From: Isaku Yamahata <isaku.yamahata@linux.intel.com>

Add a q35 property to check whether or not SMM ranges, e.g. SMRAM, TSEG,
etc... exist for the target platform.  TDX doesn't support SMM and doesn't
play nice with QEMU modifying related guest memory ranges.

Signed-off-by: Isaku Yamahata <isaku.yamahata@linux.intel.com>
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
Message-ID: <20240320083945.991426-19-michael.roth@amd.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/i386/pc.h      |  1 +
 include/hw/pci-host/q35.h |  1 +
 hw/i386/pc_q35.c          |  2 ++
 hw/pci-host/q35.c         | 42 +++++++++++++++++++++++++++------------
 4 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 349f79df086..e52290916cb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -161,6 +161,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
 #define PCI_HOST_BELOW_4G_MEM_SIZE     "below-4g-mem-size"
 #define PCI_HOST_ABOVE_4G_MEM_SIZE     "above-4g-mem-size"
+#define PCI_HOST_PROP_SMM_RANGES       "smm-ranges"
 
 
 void pc_pci_as_mapping_init(MemoryRegion *system_memory,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index bafcbe67521..22fadfa3ed7 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -50,6 +50,7 @@ struct MCHPCIState {
     MemoryRegion tseg_blackhole, tseg_window;
     MemoryRegion smbase_blackhole, smbase_window;
     bool has_smram_at_smbase;
+    bool has_smm_ranges;
     Range pci_hole;
     uint64_t below_4g_mem_size;
     uint64_t above_4g_mem_size;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6e1180d4b60..bb53a51ac18 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -219,6 +219,8 @@ static void pc_q35_init(MachineState *machine)
                             x86ms->above_4g_mem_size, NULL);
     object_property_set_bool(phb, PCI_HOST_BYPASS_IOMMU,
                              pcms->default_bus_bypass_iommu, NULL);
+    object_property_set_bool(phb, PCI_HOST_PROP_SMM_RANGES,
+                             x86_machine_is_smm_enabled(x86ms), NULL);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
 
     /* pci */
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 98d4a7c253a..0b6cbaed7ed 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -179,6 +179,8 @@ static Property q35_host_props[] = {
                      mch.below_4g_mem_size, 0),
     DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
                      mch.above_4g_mem_size, 0),
+    DEFINE_PROP_BOOL(PCI_HOST_PROP_SMM_RANGES, Q35PCIHost,
+                     mch.has_smm_ranges, true),
     DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -214,6 +216,7 @@ static void q35_host_initfn(Object *obj)
     /* mch's object_initialize resets the default value, set it again */
     qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
                          Q35_PCI_HOST_HOLE64_SIZE_DEFAULT);
+
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
                         q35_host_get_pci_hole_start,
                         NULL, NULL, NULL);
@@ -476,6 +479,10 @@ static void mch_write_config(PCIDevice *d,
         mch_update_pciexbar(mch);
     }
 
+    if (!mch->has_smm_ranges) {
+        return;
+    }
+
     if (ranges_overlap(address, len, MCH_HOST_BRIDGE_SMRAM,
                        MCH_HOST_BRIDGE_SMRAM_SIZE)) {
         mch_update_smram(mch);
@@ -494,10 +501,13 @@ static void mch_write_config(PCIDevice *d,
 static void mch_update(MCHPCIState *mch)
 {
     mch_update_pciexbar(mch);
+
     mch_update_pam(mch);
-    mch_update_smram(mch);
-    mch_update_ext_tseg_mbytes(mch);
-    mch_update_smbase_smram(mch);
+    if (mch->has_smm_ranges) {
+        mch_update_smram(mch);
+        mch_update_ext_tseg_mbytes(mch);
+        mch_update_smbase_smram(mch);
+    }
 
     /*
      * pci hole goes from end-of-low-ram to io-apic.
@@ -538,19 +548,21 @@ static void mch_reset(DeviceState *qdev)
     pci_set_quad(d->config + MCH_HOST_BRIDGE_PCIEXBAR,
                  MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
 
-    d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
-    d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
-    d->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK;
-    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK;
+    if (mch->has_smm_ranges) {
+        d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
+        d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
+        d->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK;
+        d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK;
 
-    if (mch->ext_tseg_mbytes > 0) {
-        pci_set_word(d->config + MCH_HOST_BRIDGE_EXT_TSEG_MBYTES,
-                     MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY);
+        if (mch->ext_tseg_mbytes > 0) {
+            pci_set_word(d->config + MCH_HOST_BRIDGE_EXT_TSEG_MBYTES,
+                        MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY);
+        }
+
+        d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
+        d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
     }
 
-    d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
-    d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
-
     mch_update(mch);
 }
 
@@ -578,6 +590,10 @@ static void mch_realize(PCIDevice *d, Error **errp)
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 
+    if (!mch->has_smm_ranges) {
+        return;
+    }
+
     /* if *disabled* show SMRAM to all CPUs */
     memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram-region",
                              mch->pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,
-- 
2.44.0

On 23.04.2024 18:09, Paolo Bonzini wrote:
> From: Isaku Yamahata <isaku.yamahata@linux.intel.com>
> 
> Add a q35 property to check whether or not SMM ranges, e.g. SMRAM, TSEG,
> etc... exist for the target platform.  TDX doesn't support SMM and doesn't
> play nice with QEMU modifying related guest memory ranges.

So, as I wrote in another email, this broke video (screen is blank) for

  qemu-system-x86_64 -machine q35,accel=kvm,smm=off

before this commit, there are usual seabios messages (and even messages
from qemu before it loads seabios), and after this commit, the screen
stays blank.


> @@ -578,6 +590,10 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
>   
> +    if (!mch->has_smm_ranges) {
> +        return;
> +    }
> +
>       /* if *disabled* show SMRAM to all CPUs */
>       memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram-region",
>                                mch->pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,

Moving this if..return block right below this smram-region init fixes
the problem with the video:

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 0b6cbaed7e..aa8c4a273a 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -589,20 +589,20 @@ static void mch_realize(PCIDevice *d, Error **errp)
                   mch->system_memory, mch->pci_address_space,
                   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
      }

-    if (!mch->has_smm_ranges) {
-        return;
-    }
-
      /* if *disabled* show SMRAM to all CPUs */
      memory_region_init_alias(&mch->smram_region, OBJECT(mch), 
"smram-region",
                               mch->pci_address_space, 
MCH_HOST_BRIDGE_SMRAM_C_BASE,
                               MCH_HOST_BRIDGE_SMRAM_C_SIZE);
      memory_region_add_subregion_overlap(mch->system_memory, 
MCH_HOST_BRIDGE_SMRAM_C_BASE,
                                          &mch->smram_region, 1);
      memory_region_set_enabled(&mch->smram_region, true);

+    if (!mch->has_smm_ranges) {
+        return;
+    }
+
      memory_region_init_alias(&mch->open_high_smram, OBJECT(mch), 
"smram-open-high",
                               mch->ram_memory, 
MCH_HOST_BRIDGE_SMRAM_C_BASE,
                               MCH_HOST_BRIDGE_SMRAM_C_SIZE);
      memory_region_add_subregion_overlap(mch->system_memory, 0xfeda0000,


I've no idea what else might be missing here.

Overall, adding an early return this way is a recipe for disaster
to happen - if not now then later.  Here, and in mch_write_config().

Thanks,

/mjt

Ping, also adding kraxel@.

Should I send formal patch moving one line of code up?

Thanks,

/mjt

On 12.08.2025 18:27, Michael Tokarev wrote:
> On 23.04.2024 18:09, Paolo Bonzini wrote:
>> From: Isaku Yamahata <isaku.yamahata@linux.intel.com>
>>
>> Add a q35 property to check whether or not SMM ranges, e.g. SMRAM, TSEG,
>> etc... exist for the target platform.  TDX doesn't support SMM and 
>> doesn't
>> play nice with QEMU modifying related guest memory ranges.
> 
> So, as I wrote in another email, this broke video (screen is blank) for
> 
>   qemu-system-x86_64 -machine q35,accel=kvm,smm=off
> 
> before this commit, there are usual seabios messages (and even messages
> from qemu before it loads seabios), and after this commit, the screen
> stays blank.
> 
> 
>> @@ -578,6 +590,10 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>>       }
>> +    if (!mch->has_smm_ranges) {
>> +        return;
>> +    }
>> +
>>       /* if *disabled* show SMRAM to all CPUs */
>>       memory_region_init_alias(&mch->smram_region, OBJECT(mch), 
>> "smram-region",
>>                                mch->pci_address_space, 
>> MCH_HOST_BRIDGE_SMRAM_C_BASE,
> 
> Moving this if..return block right below this smram-region init fixes
> the problem with the video:
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 0b6cbaed7e..aa8c4a273a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -589,20 +589,20 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                    mch->system_memory, mch->pci_address_space,
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
> 
> -    if (!mch->has_smm_ranges) {
> -        return;
> -    }
> -
>       /* if *disabled* show SMRAM to all CPUs */
>       memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram- 
> region",
>                                mch->pci_address_space, 
> MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                MCH_HOST_BRIDGE_SMRAM_C_SIZE);
>       memory_region_add_subregion_overlap(mch->system_memory, 
> MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                           &mch->smram_region, 1);
>       memory_region_set_enabled(&mch->smram_region, true);
> 
> +    if (!mch->has_smm_ranges) {
> +        return;
> +    }
> +
>       memory_region_init_alias(&mch->open_high_smram, OBJECT(mch), 
> "smram-open-high",
>                                mch->ram_memory, 
> MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                MCH_HOST_BRIDGE_SMRAM_C_SIZE);
>       memory_region_add_subregion_overlap(mch->system_memory, 0xfeda0000,
> 
> 
> I've no idea what else might be missing here.
> 
> Overall, adding an early return this way is a recipe for disaster
> to happen - if not now then later.  Here, and in mch_write_config().
> 
> Thanks,
> 
> /mjt
> 
> 

On Wed, Sep 17, 2025 at 05:23:55PM +0300, Michael Tokarev wrote:
> Ping, also adding kraxel@.
> 
> Should I send formal patch moving one line of code up?

I'm wondering why this is needed in the first place?

We already have a smm machine property, and with smm
disabled all the smm-related memory regions should
stay disabled ...

take care,
  Gerd

On 18.09.2025 12:20, Gerd Hoffmann wrote:
> On Wed, Sep 17, 2025 at 05:23:55PM +0300, Michael Tokarev wrote:
>> Ping, also adding kraxel@.
>>
>> Should I send formal patch moving one line of code up?
> 
> I'm wondering why this is needed in the first place?
> 
> We already have a smm machine property, and with smm
> disabled all the smm-related memory regions should
> stay disabled ...

I've a bug report stating that vga output does not work
with -machine q35,smm=off.  It is trivial to observe -
just run `qemu-system-x86_64 -machine q35,smm=off` --
you'll see an empty screen instead of seabios output
(after the commit in question).  Moving the "if..return"
line down one statement makes the output visible again.

I dunno if it actually *shuld* work (ie, there should be
an output on the guest screen).  If there shoudln't be,
okay, let's say it's an operator error.

I just don't know all these different cpu/machine features
and all their nuances.

Thanks,

/mjt

On Thu, Sep 18, 2025 at 12:27:16PM +0300, Michael Tokarev wrote:
> On 18.09.2025 12:20, Gerd Hoffmann wrote:
> > On Wed, Sep 17, 2025 at 05:23:55PM +0300, Michael Tokarev wrote:
> > > Ping, also adding kraxel@.
> > > 
> > > Should I send formal patch moving one line of code up?
> > 
> > I'm wondering why this is needed in the first place?
> > 
> > We already have a smm machine property, and with smm
> > disabled all the smm-related memory regions should
> > stay disabled ...
> 
> I've a bug report stating that vga output does not work
> with -machine q35,smm=off.

Sorry for the confusion, I was wondering why we need the original
patch which introduces the problem ...

take care,
  Gerd

On 23/4/24 17:09, Paolo Bonzini wrote:
> From: Isaku Yamahata <isaku.yamahata@linux.intel.com>
> 
> Add a q35 property to check whether or not SMM ranges, e.g. SMRAM, TSEG,
> etc... exist for the target platform.  TDX doesn't support SMM and doesn't
> play nice with QEMU modifying related guest memory ranges.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@linux.intel.com>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Message-ID: <20240320083945.991426-19-michael.roth@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/hw/i386/pc.h      |  1 +
>   include/hw/pci-host/q35.h |  1 +
>   hw/i386/pc_q35.c          |  2 ++
>   hw/pci-host/q35.c         | 42 +++++++++++++++++++++++++++------------
>   4 files changed, 33 insertions(+), 13 deletions(-)


> @@ -578,6 +590,10 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
>   
> +    if (!mch->has_smm_ranges) {
> +        return;

Are we sure we are not skipping something unrelated to SMM like the
"QEMU specific hack" with the tseg-blackhole region?

> +    }
> +
>       /* if *disabled* show SMRAM to all CPUs */
>       memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram-region",
>                                mch->pci_address_space, MCH_HOST_BRIDGE_SMRAM_C_BASE,