On 12/10/25 10:40 PM, Igor Mammedov wrote:
> On Wed, 3 Dec 2025 19:08:51 +0100
> Andrey Ryabinin <[email protected]> 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;
>> }
>
