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/[email protected]/
> Fixes: f404220e279c ("q35: implement 128K SMRAM at default SMBASE address")
> Reported-by: Andrey Ryabinin <[email protected]>
> Signed-off-by: Igor Mammedov <[email protected]>
> ---
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
>
>
