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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Dec 11, 2025 at 05:54:54PM +0100, Igor Mammedov wrote:
> When migrating, dst QEMU by default has SMRAM unlocked,
> and since wmask is not migrated, the migrated value of
> MCH_HOST_BRIDGE_F_SMBASE in config space fall to prey of
> 
>   mch_update_smbase_smram()
>     ...
>     if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
>         *reg = 0x00;
> 
> and is getting cleared and leads to unlocked smram
> on dst even if on source it's been locked.
> 
> As Andrey has pointed out [1], we should derive wmask
> from config and not other way around.
> 
> Drop offending chunk and resync wmask based on MCH_HOST_BRIDGE_F_SMBASE
> register value. That would preserve the register during
> migration and set smram regions into corresponding state.
> 
> What that changes is:
> that it would let guest write junk values in register
> (with no apparent effect) until it's stumbles upon
> reserved 0x1 [|] 0x2 values, at which point it
> would be only possible to lock register and trigger
> switch to SMRAM blackhole in CPU AS.
> 
> While at it, fix up test by removing junk discard before negotiation hunk.
> 
> PS2:
> Instead of adding a dedicated post_load handler for it,
> reuse mch_update->mch_update_smbase_smram call chain
> that is called on write/reset/post_load to be consistent
> with how we handle mch registers.
> 
> PS3:
> for prosterity here is erro message Andrey got due to this bug:
>     qemu: vfio_container_dma_map(0x..., 0x0, 0xa0000, 0x....) = -22 (Invalid argument)
>     qemu: hardware error: vfio: DMA mapping failed, unable to continue
> 
> 1) https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/
> Fixes: f404220e279c ("q35: implement 128K SMRAM at default SMBASE address")
> Reported-by: Andrey Ryabinin <arbn@yandex-team.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Cc: qemu-stable@nongnu.org
> Cc: mst@redhat.com
> Cc: arbn@yandex-team.com
> Cc: michael.roth@amd.com
> ---
>  hw/pci-host/q35.c      | 25 +++++++++++--------------
>  tests/qtest/q35-test.c |  6 ------
>  2 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index a708758d36..946342ba58 100644
> --- 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;
>      }
> -
> -    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;
> +        /* 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;

why not move the if here and then simply:

	if (lck) {
		/* lock register at 0x2 and disable all writes */
		pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
		*reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
	}

can be a patch on top.

> +    memory_region_transaction_begin();
>      memory_region_set_enabled(&mch->smbase_blackhole, lck);
>      memory_region_set_enabled(&mch->smbase_window, lck);
>      memory_region_transaction_commit();
> diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
> index 62fff49fc8..4e3a4457f6 100644
> --- a/tests/qtest/q35-test.c
> +++ b/tests/qtest/q35-test.c
> @@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
>      qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
>      g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
>  
> -    /* check that writing junk to 0x9c before before negotiating is ignored */
> -    for (i = 0; i < 0xff; i++) {
> -        qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
> -        g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
> -    }
> -
>      /* enable SMRAM at SMBASE */
>      qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
>      g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);
> -- 
> 2.47.3
> 
>