Re: [PATCH QEMU HCI-8.1 v4 12/31] q35: Fix migration of SMRAM state

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] q35: SMBASE lock reachable from reset state without 0xff/0x01 negotiation
--

commit 910f528c422bd7c6087d0502ae62fa2f2c8cd852
Author: Igor Mammedov <imammedo@redhat.com>

q35: Fix migration of SMRAM state

This commit stops mch_update_smbase_smram() from clearing the SMBASE
register when wmask is still at its reset value. wmask is now derived from
the register value instead, so a locked state survives migration. The
junk-write check in test_smram_smbase_lock() is removed to match.

Link: https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c

[ ... ]

> @@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
>      }
>  
>      if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> -            MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
>          return;
>      }
>  
>      /*
> -     * default/reset state, discard written value
> -     * which will disable SMRAM balackhole at SMBASE
> +     * reg value can come from register write/reset/migration source,
> +     * update wmask to be in sync with it regardless of source
>       */
> -    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> -        *reg = 0x00;
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        return;
>      }
> -
> -    memory_region_transaction_begin();
>      if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
> -        /* disable all writes */
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
> -            ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        /* lock register at 0x2 and disable all writes */
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> -        lck = true;
> -    } else {
> -        lck = false;
>      }
> +
> +    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +    memory_region_transaction_begin();
>      memory_region_set_enabled(&mch->smbase_blackhole, lck);
>      memory_region_set_enabled(&mch->smbase_window, lck);
>      memory_region_transaction_commit();

[Severity: Low]
After mch_reset(), config[MCH_HOST_BRIDGE_F_SMBASE] is 0 and wmask is
0xff, with no 0xff/0x01 negotiation having taken place. Can a guest now
lock the register directly from this state?

For example, a guest config write of 0x02, 0x06 or 0xfe is stored by
pci_default_write_config() because wmask is 0xff:

mch_write_config()
  mch_update_smbase_smram()
    /* value is neither QUERY (0xff) nor IN_RAM (0x01) */
    if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
        *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
    }
    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
    memory_region_set_enabled(&mch->smbase_blackhole, lck);
    memory_region_set_enabled(&mch->smbase_window, lck);

The removed check used to zero such writes while wmask was 0xff, so this
path was not reachable before negotiation. Now smbase_blackhole and
smbase_window can be enabled without it, and non-lock junk values are
stored as written.

The commit message acknowledges that junk values are now accepted, but it
says the register can only be locked after reaching the reserved 0x1 or
0x2 values. Should it also say that a plain write of 0x02 locks the
register directly from the reset state?

[ ... ]

> diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
> --- a/tests/qtest/q35-test.c
> +++ b/tests/qtest/q35-test.c

[ ... ]

> @@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
>      qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
>      g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
>  
> -    /* check that writing junk to 0x9c before before negotiating is ignored */
> -    for (i = 0; i < 0xff; i++) {
> -        qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
> -        g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
> -    }
> -

[Severity: Low]
This isn't a bug, but the test that checked the old "junk is ignored before
negotiation" behaviour is removed, and nothing replaces it. Could
test_smram_smbase_lock() get coverage for the new pre-negotiation
behaviour?

The same applies to the wmask resync that the patch adds for migrated
register values. Is there a test that covers the case where the register
is 0x02 or 0x01 after post_load?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=12