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

[Igor Mammedov <imammedo@redhat.com>]

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