Re: [PATCH QEMU HCI-8.1 v7 15/35] q35: Fix migration of SMRAM state

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] q35: mch_update_smbase_smram() skips region resync when SMBASE reg is 0x01
- [Low] q35: SMBASE register accepts junk, direct 0x01 and lock writes without 0xff negotiation
--

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

q35: Fix migration of SMRAM state

This commit stops mch_update_smbase_smram() from clearing the migrated
SMBASE register value. wmask is now derived from the register value, so a
locked state survives migration. The test hunk that checked junk writes
before negotiation is removed.

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
> -     */
> -    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> -        *reg = 0x00;
> +    /*
> +     * reg value can come from register write/reset/migration source,
> +     * update wmask to be in sync with it regardless of source
> +     */
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        return;
>      }

[Severity: Medium]
Can this early return leave the SMBASE regions enabled when the register
becomes 0x01?

The return skips the memory_region_set_enabled() calls for smbase_blackhole
and smbase_window. Before the patch, a value of 0x01 fell through and
disabled both regions.

The path is mch_post_load()->mch_update()->mch_update_smbase_smram(). On a
live COLO secondary, no reset precedes it:

migration/colo.c:colo_incoming_thread()
    ret = qemu_load_device_state(fb);

Consider this sequence:

- The secondary holds reg=0x02 with both regions enabled.
- The primary reboots, and the guest renegotiates to 0x01 within one
  checkpoint interval.
- The next checkpoint loads 0x01.

post_load then sets wmask and returns, so the regions stay enabled while
the register reads 0x01 (negotiated, not locked). Guest-visible SMRAM at
SMBASE then disagrees with the register.

Should the IN_RAM branch also disable the two regions before returning?

The snapshot-load path is not affected, because load_snapshot() calls
qemu_system_reset() before qemu_loadvm_state(). A fresh incoming migration
is also not affected, because the regions start disabled.

> -
> -    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;
>      }

[Severity: Low]
Is it intended that mch_update_smbase_smram() no longer has a "not yet
negotiated" state?

In the reset state (reg=0, wmask=0xff), a guest config write to the SMBASE
register now behaves differently, via mch_write_config()->mch_update():

- A value with bit 0x2 set, such as 0x03 or 0x06, takes the "*reg &
  MCH_HOST_BRIDGE_F_SMBASE_LCK" branch. It sets wmask to 0, stores 0x02,
  and enables smbase_blackhole and smbase_window. The guest never saw the
  0x01 reply.
- A write of exactly 0x01 takes the IN_RAM branch and sets wmask to
  MCH_HOST_BRIDGE_F_SMBASE_LCK without a prior 0xff query.
- Other values, such as 0x04, are stored and read back instead of
  reading 0.

The commit message mentions the last case ("let guest write junk values").
It does not mention the first two.

Could the commit message describe the lock-without-negotiation and direct
0x01 cases as well?

> 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);
> -    }
> -
>      /* enable SMRAM at SMBASE */
>      qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
>      g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);

[Severity: Low]
This isn't a bug, but the removed junk-write check is not replaced by any
test of the new behavior. Would it be worth adding coverage for the new
pre-negotiation write semantics and for the migrated register state
(locked and negotiated) in test_smram_smbase_lock()?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=15