Re: [PATCH] q35: Fix migration of SMRAM state

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  3 Dec 2025 19:08:51 +0100
Andrey Ryabinin <arbn@yandex-team.com> wrote:

> mch_update_smbase_smram() essentially uses wmask[MCH_HOST_BRIDGE_F_SMBASE]
> to track SMBASE area state. Since 'wmask' state is not migrated is not
> migrated, the destination QEMU always sees
>  wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff
> 
> As a result, when mch_update() calls mch_update_smbase_smram() on the
> destination, it resets ->config[MCH_HOST_BRIDGE_F_SMBASE] and disables
> the smbase-window memory region—even if it was enabled on the source.

[...]

> +static void mch_smbase_smram_post_load(MCHPCIState *mch)
> +{
> +    PCIDevice *pd = PCI_DEVICE(mch);
> +    uint8_t *reg = pd->config + MCH_HOST_BRIDGE_F_SMBASE;
> +
> +    if (!mch->has_smram_at_smbase) {
> +        return;
> +    }
> +
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> +            MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +    } else if (*reg == MCH_HOST_BRIDGE_F_SMBASE_LCK) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
> +    }
> +}
You are correctly pointing to the issue about non-migratable wmask controlling
config[], it should be other way around.

given reset already sets
  wmask[MCH_HOST_BRIDGE_F_SMBASE] && config[MCH_HOST_BRIDGE_F_SMBASE]
to default values, we don't need to do the same in mch_update_smbase_smram()
so we can just drop it.

Also I wouldn't introduce a dedicated mch_smbase_smram_post_load() though,
since mch_post_load() already calls mch_update_smbase_smram() indirectly,
I'd rather fix the later.

Would following work for you:

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index a708758d36..7a85a349bd 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -431,31 +431,25 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
         return;
     }
 
-    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
-        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 either 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;
+    switch (*reg) {
+    case MCH_HOST_BRIDGE_F_SMBASE_QUERY:
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+        *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
+        return;
+    case MCH_HOST_BRIDGE_F_SMBASE_LCK:
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
+        break;
+    case MCH_HOST_BRIDGE_F_SMBASE_IN_RAM:
+        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+        break;
     }
 
     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;
-        *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
-        lck = true;
-    } else {
-        lck = false;
-    }
+    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
     memory_region_set_enabled(&mch->smbase_blackhole, lck);
     memory_region_set_enabled(&mch->smbase_window, lck);
     memory_region_transaction_commit();

>  static int mch_post_load(void *opaque, int version_id)
>  {
>      MCHPCIState *mch = opaque;
> +
> +    mch_smbase_smram_post_load(mch);
>      mch_update(mch);
>      return 0;
>  }