Re: [PATCH QEMU HCI-8.1 07/21] 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: mch_update_smbase_smram() locks SMBASE on a pre-handshake write with bit 1 set
--

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

q35: Fix migration of SMRAM state

This patch stops mch_update_smbase_smram() from clearing the migrated
MCH_HOST_BRIDGE_F_SMBASE value. It re-derives wmask from the register
value instead, so a locked SMRAM state survives migration. It also
removes the qtest check that junk written before the handshake reads
back as 0.

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

[ ... ]

>      /*
> -     * 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]
Can a guest now lock SMBASE without doing the 0xff handshake? After
mch_reset(), config[0x9c] is 0 and wmask[0x9c] is 0xff. A guest config
write of 0x02, 0x03 or 0xfe is stored by pci_default_write_config(),
because wmask is 0xff.

mch_write_config() then calls mch_update_smbase_smram(). The value is
neither 0xff nor 0x01, so it reaches this branch:

    if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
        *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
    }

wmask becomes 0 and *reg becomes 0x02. smbase_blackhole and
smbase_window are then enabled, so the 128K SMRAM window at SMBASE is
black-holed until machine reset.

The old wmask == 0xff check discarded such a write and set *reg to 0.
Other junk values are now stored as written, with no effect.

Only the writing guest is affected, and a reset clears it. Firmware such
as SeaBIOS and OVMF uses the 0xff query handshake first, so it is
unaffected. The commit message describes this trade-off, and the change
matches upstream QEMU 66cf169e29b0. Is this guest-visible change
intended?

The migration fix itself looks correct. mch_post_load() calls
mch_update(), which calls mch_update_smbase_smram() and re-derives wmask
from the migrated register value. A value of 0x01 gives wmask 0x02 with
no black hole. A value of 0x02 gives wmask 0 with the black hole
enabled. A value of 0 keeps the reset wmask of 0xff.

> 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 removed check is not replaced by anything.
test_smram_smbase_lock() no longer covers a junk write before the
handshake, or the new lock-on-junk behaviour where a value with bit 1
set locks the register. No test covers migration of the SMRAM state
either.

Could a test be added for these cases?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=7