[PATCH] q35: Fix migration of SMRAM state

[PATCH] q35: Fix migration of SMRAM state

[Andrey Ryabinin]

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.

'info mtree' on source:
  address-space: KVM-SMRAM
    0000000000000000-ffffffffffffffff (prio 0, i/o): mem-container-smram
      0000000000000000-00000000ffffffff (prio 10, i/o): smram
        0000000000030000-000000000004ffff (prio 0, i/o): alias smbase-window @pc.ram
        00000000000a0000-00000000000bffff (prio 0, i/o): alias smram-low @pc.ram
        000000007f000000-000000007fffffff (prio 0, i/o): alias tseg-window @pc.ram
      0000000000000000-ffffffffffffffff (prio 0, i/o): alias mem-smram @system

'info mtree' on dest after migration:
  address-space: KVM-SMRAM
    0000000000000000-ffffffffffffffff (prio 0, i/o): mem-container-smram
      0000000000000000-00000000ffffffff (prio 10, i/o): smram
        00000000000a0000-00000000000bffff (prio 0, i/o): alias smram-low @pc.ram
        000000007f000000-000000007fffffff (prio 0, i/o): alias tseg-window @pc.ram
      0000000000000000-ffffffffffffffff (prio 0, i/o): alias mem-smram @system

This mismatch in memory regions breaks CPR-transfer migration with VFIO:
the DMA mappings sent via VFIO_IOMMU_MAP_DMA on the destination must
match the source. Otherwise, the destination QEMU aborts:

    qemu: vfio_container_dma_map(0x..., 0x0, 0xa0000, 0x....) = -22 (Invalid argument)
    qemu: hardware error: vfio: DMA mapping failed, unable to continue

According to mch_update_smbase_smram() the valid combinations of
->config and ->wmask for MCH_HOST_BRIDGE_F_SMBASE are:

  1) ->config[] == 0x0 && ->wmask[] == 0xff
  2) ->config[] == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM &&
          ->wmask[] == MCH_HOST_BRIDGE_F_SMBASE_LCK
  3) ->config[] == MCH_HOST_BRIDGE_F_SMBASE_LCK &&
          ->wmask[] == 0

Add mch_smbase_smram_post_load() to restore ->wmask of
MCH_HOST_BRIDGE_F_SMBASE based on ->config[] value, ensuring that the
follow-up call to mch_update_smbase_smram() correctly reinstates the
smbase-window region if it was enabled on the source.

Cc: qemu-stable@nongnu.org
Fixes: f404220e279c ("q35: implement 128K SMRAM at default SMBASE address")
Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
---
 hw/pci-host/q35.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index a708758d36..2466a1aded 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -517,9 +517,27 @@ static void mch_update(MCHPCIState *mch)
                      IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
+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;
+    }
+}
 static int mch_post_load(void *opaque, int version_id)
 {
     MCHPCIState *mch = opaque;
+
+    mch_smbase_smram_post_load(mch);
     mch_update(mch);
     return 0;
 }
-- 
2.51.2

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

[Igor Mammedov]

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

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

[Andrey Ryabinin]

On 12/10/25 10:40 PM, Igor Mammedov wrote:
> 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;


This change makes strict adherence to the negotiation protocol unnecessary. As a result:
 - Writes performed before MCH_HOST_BRIDGE_F_SMBASE_QUERY are no longer ignored.
 - The guest can set MCH_HOST_BRIDGE_F_SMBASE_LCK immediately.
 - The guest can now set MCH_HOST_BRIDGE_F_SMBASE_IN_RAM, which was previously impossible.

Perhaps this is acceptable — it may simply expose misbehaving guest behavior, I’m not sure,
I'm no expert here. But it does raise the question of why these restrictions existed
in the first place.

Also, if we are lifting these restrictions, tests/qtest/q35-test.c will need to be updated.

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

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

[Igor Mammedov]

On Thu, 11 Dec 2025 16:51:44 +0900
Andrey Ryabinin <arbn@yandex-team.com> wrote:

> On 12/10/25 10:40 PM, Igor Mammedov wrote:
> > 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;  
> 
> 
> This change makes strict adherence to the negotiation protocol unnecessary. As a result:
>  - Writes performed before MCH_HOST_BRIDGE_F_SMBASE_QUERY are no longer ignored.
>  - The guest can set MCH_HOST_BRIDGE_F_SMBASE_LCK immediately.
>  - The guest can now set MCH_HOST_BRIDGE_F_SMBASE_IN_RAM, which was previously impossible.
> 
> Perhaps this is acceptable — it may simply expose misbehaving guest behavior, I’m not sure,
> I'm no expert here. But it does raise the question of why these restrictions existed
> in the first place.

main point of this is discovery of the feature and enabling/locking backhole.
and only the last (#3 in original commit says anything wrt further writes).

so yes, above change will open up ability to write MCH_HOST_BRIDGE_F_SMBASE_IN_RAM
with out the same outcome as MCH_HOST_BRIDGE_F_SMBASE_QUERY, which ain't breaking
anything. But it's pointless as it doesn't really tell guest if the feature is available.
There must be asymmetric write 0xff with following read 0x1 read to occur, otherwise
nothing is guarantied.

the same applies to MCH_HOST_BRIDGE_F_SMBASE_LCK write, without negotiation.
One might get backhole enabled or not, it shouldn't affect existing code that
is written according to described protocol.
Drawback of it would be possibility to guest write new code that could use
backhole effect to check if it's enabled, but then it won't work on older
QEMU and onus is on them to make it follow protocol as described.

I think it should be ok to relax existing restrictions a bit as long as
detection/locking flow works the same as was initially described.

And while I'm not opposed to a dedicated post_load routine, it is ok too.
But considering that we do use mch_update() on migration and write path
anyway and not only for smbase lock, I'm in favor of fixing up
mch_update_smbase_smram() for consistency.

> 
> Also, if we are lifting these restrictions, tests/qtest/q35-test.c will need to be updated.
thanks,
I'll fix it up and post a proper patch.

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