Re: [PATCH QEMU HCI-8.1 v2 07/24] 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 disable on IN_RAM post_load
- [Low] q35: mch_update_smbase_smram() does not restore wmask for loaded 0x00 SMBASE
--

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

q35: Fix migration of SMRAM state

This change drops the check in mch_update_smbase_smram() that cleared the
SMBASE register while wmask was 0xff. It resyncs wmask from the register
value instead, so a locked SMRAM state is preserved across migration. It
also removes the test hunk that wrote junk to the register before
negotiation.

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

[Severity: Medium]
Can this early return leave the SMBASE regions enabled?  Before this patch,
a register value of 0x01 fell through to lck = false, which disabled both
smbase_blackhole and smbase_window.  Now only wmask is updated, and
memory_region_set_enabled(false) is never called for 0x01.

Consider mch_post_load() -> mch_update() -> mch_update_smbase_smram() on a
live MCHPCIState that is currently locked (regions enabled) and has not been
through mch_reset().  If the incoming SMBASE value is 0x01, the regions stay
enabled while config[SMBASE] reads 0x01 (unlocked).  The guest then sees a
black-holed SMBASE although the register says the range is unlocked.  This
contradicts the comment "update wmask to be in sync with it regardless of
source".

The snapshot-load path calls qemu_system_reset() and a fresh migration
destination has the regions disabled, so neither is affected.  The path that
looks reachable is COLO, where colo_process_incoming_thread() calls
qemu_loadvm_state_main() repeatedly on a running secondary VM without
qemu_system_reset().  If the primary locks SMRAM, reboots, and negotiates
0x01 within one checkpoint interval, the secondary loads 0x01 while its
regions are still enabled from the earlier lock.  This needs COLO and a
machine with has_smram_at_smbase.

The final series state does not change hw/pci-host/q35.c, so this is not
addressed later in the series.  Should the IN_RAM branch also disable
smbase_blackhole and smbase_window before returning?

>      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]
Does this code restore wmask when a loaded SMBASE value is 0x00 or a junk
value without bit 1 set?  wmask is resynced only for QUERY, IN_RAM and
values with the LCK bit.  For other values wmask is left unchanged.

mch_reset() is the only place that sets wmask back to 0xff:

    d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
    d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;

On the COLO secondary, colo_incoming_process_checkpoint() calls
qemu_load_device_state() -> qemu_loadvm_state_main() -> mch_post_load() ->
mch_update() -> mch_update_smbase_smram() without a device reset.  If the
secondary's MCH is locked (wmask 0) and a later checkpoint carries
SMBASE=0x00, wmask stays 0.  The register is then read-only and the guest can
no longer negotiate or lock SMRAM at SMBASE.  The primary could carry 0x00
after a guest reset, before firmware negotiation.

This only shows up if the secondary takes over after such a checkpoint
without a guest reset, and the final series state does not change this file.
Is the comment "update wmask to be in sync with it regardless of source"
fully accurate here, and should wmask be set to 0xff for values that do not
match the QUERY, IN_RAM or LCK cases?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=7