i386/kvm: Enable SMM addrss space for i386 cpu

[PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Xiaoyao Li 3 months, 2 weeks ago

Kirill Martynov reported assertation in cpu_asidx_from_attrs() being hit
when x86_cpu_dump_state() is called to dump the CPU state[*]. It happens
when the CPU is in SMM and KVM emulation failure due to misbehaving
guest.

The root cause is that QEMU i386 never enables the SMM addressspace for cpu
since kvm SMM support has been added.

Enable the SMM cpu address space under KVM when the SMM is enabled for
the x86machine.

[*] https://lore.kernel.org/qemu-devel/20250523154431.506993-1-stdcalllevi@yandex-team.ru/

Reported-by: Kirill Martynov <stdcalllevi@yandex-team.ru>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 system/physmem.c          |  5 -----
 target/i386/kvm/kvm-cpu.c | 10 ++++++++++
 target/i386/kvm/kvm.c     |  5 +++++
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 130c148ffb5c..76e1c33aab5c 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -795,9 +795,6 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
         cpu->as = as;
     }
 
-    /* KVM cannot currently support multiple address spaces. */
-    assert(asidx == 0 || !kvm_enabled());
-
     if (!cpu->cpu_ases) {
         cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
         cpu->cpu_ases_count = cpu->num_ases;
@@ -820,8 +817,6 @@ void cpu_address_space_destroy(CPUState *cpu, int asidx)
 
     assert(cpu->cpu_ases);
     assert(asidx >= 0 && asidx < cpu->num_ases);
-    /* KVM cannot currently support multiple address spaces. */
-    assert(asidx == 0 || !kvm_enabled());
 
     cpuas = &cpu->cpu_ases[asidx];
     if (tcg_enabled()) {
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 89a795365945..aa657c2a4627 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -13,6 +13,7 @@
 #include "qapi/error.h"
 #include "system/system.h"
 #include "hw/boards.h"
+#include "hw/i386/x86.h"
 
 #include "kvm_i386.h"
 #include "accel/accel-cpu-target.h"
@@ -91,6 +92,15 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
         kvm_set_guest_phys_bits(cs);
     }
 
+    /*
+     * When SMM is enabled, there is 2 address spaces. Otherwise only 1.
+     *
+     * Only init address space 0 here, the second one for SMM is initialized at
+     * register_smram_listener() after machine init done.
+     */
+    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
+    cpu_address_space_init(cs, 0, "cpu-mmeory", cs->memory);
+
     return true;
 }
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 369626f8c8d7..47fb5c673c8e 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2704,6 +2704,7 @@ static MemoryRegion smram_as_mem;
 
 static void register_smram_listener(Notifier *n, void *unused)
 {
+    CPUState *cpu;
     MemoryRegion *smram =
         (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
 
@@ -2728,6 +2729,10 @@ static void register_smram_listener(Notifier *n, void *unused)
     address_space_init(&smram_address_space, &smram_as_root, "KVM-SMRAM");
     kvm_memory_listener_register(kvm_state, &smram_listener,
                                  &smram_address_space, 1, "kvm-smram");
+
+    CPU_FOREACH(cpu) {
+        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
+    }
 }
 
 static void *kvm_msr_energy_thread(void *data)
-- 
2.43.0

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Zhao Liu 3 months, 2 weeks ago

> @@ -91,6 +92,15 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>          kvm_set_guest_phys_bits(cs);
>      }
>  
> +    /*
> +     * When SMM is enabled, there is 2 address spaces. Otherwise only 1.
> +     *
> +     * Only init address space 0 here, the second one for SMM is initialized at
               ^^^^
	       initialize

> +     * register_smram_listener() after machine init done.
> +     */
> +    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
> +    cpu_address_space_init(cs, 0, "cpu-mmeory", cs->memory);
> +
>      return true;
>  }
>  
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 369626f8c8d7..47fb5c673c8e 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2704,6 +2704,7 @@ static MemoryRegion smram_as_mem;
>  
>  static void register_smram_listener(Notifier *n, void *unused)
>  {
> +    CPUState *cpu;
>      MemoryRegion *smram =
>          (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>  
> @@ -2728,6 +2729,10 @@ static void register_smram_listener(Notifier *n, void *unused)
>      address_space_init(&smram_address_space, &smram_as_root, "KVM-SMRAM");
>      kvm_memory_listener_register(kvm_state, &smram_listener,
>                                   &smram_address_space, 1, "kvm-smram");
> +
> +    CPU_FOREACH(cpu) {
> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);

It is worth mentioning in the commit message that directly sharing
MemoryRegion in CPUAddressSpace is safe.

> +    }

I still think such CPU_FOREACH in machine_done callback is not the
best approach - it's better to initialize all the address spaces in
kvm_cpu_realizefn(), and not to go far away from cs->num_ases, as I
said in the previous discussion.

But it's still good to fix this bug. So, with other comments
addressed,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Xiaoyao Li 3 months, 2 weeks ago

On 7/30/2025 4:11 PM, Zhao Liu wrote:
>> @@ -91,6 +92,15 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>>           kvm_set_guest_phys_bits(cs);
>>       }
>>   
>> +    /*
>> +     * When SMM is enabled, there is 2 address spaces. Otherwise only 1.
>> +     *
>> +     * Only init address space 0 here, the second one for SMM is initialized at
>                 ^^^^
> 	       initialize
> 
>> +     * register_smram_listener() after machine init done.
>> +     */
>> +    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
>> +    cpu_address_space_init(cs, 0, "cpu-mmeory", cs->memory);
>> +
>>       return true;
>>   }
>>   
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 369626f8c8d7..47fb5c673c8e 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2704,6 +2704,7 @@ static MemoryRegion smram_as_mem;
>>   
>>   static void register_smram_listener(Notifier *n, void *unused)
>>   {
>> +    CPUState *cpu;
>>       MemoryRegion *smram =
>>           (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>>   
>> @@ -2728,6 +2729,10 @@ static void register_smram_listener(Notifier *n, void *unused)
>>       address_space_init(&smram_address_space, &smram_as_root, "KVM-SMRAM");
>>       kvm_memory_listener_register(kvm_state, &smram_listener,
>>                                    &smram_address_space, 1, "kvm-smram");
>> +
>> +    CPU_FOREACH(cpu) {
>> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
> 
> It is worth mentioning in the commit message that directly sharing
> MemoryRegion in CPUAddressSpace is safe.

It's unnecessary to me. It's common that different Address space share 
the same (root) memory region. e.g., for address space 0 for the cpu, 
though what passed in is cpu->memory, they all point to system_memory.

>> +    }
> 
> I still think such CPU_FOREACH in machine_done callback is not the
> best approach - it's better to initialize all the address spaces in
> kvm_cpu_realizefn(), and not to go far away from cs->num_ases, as I
> said in the previous discussion.
> 
> But it's still good to fix this bug. So, with other comments
> addressed,
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Zhao Liu 3 months, 2 weeks ago

> > > +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
> > 
> > It is worth mentioning in the commit message that directly sharing
> > MemoryRegion in CPUAddressSpace is safe.
> 
> It's unnecessary to me. It's common that different Address space share the
> same (root) memory region. e.g., for address space 0 for the cpu, though
> what passed in is cpu->memory, they all point to system_memory.

For cpu->memory, there's the "object_ref(OBJECT(cpu->memory))" in
cpu_exec_initfn().

But this case doesn't need to increase ref count like cpu->memory, since
memory_region_ref() provides protection and it's enough.

This is the difference.

So it sounds like now it's more necessary to clarify this, no?

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Xiaoyao Li 3 months, 2 weeks ago

On 7/30/2025 11:20 PM, Zhao Liu wrote:
>>>> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
>>>
>>> It is worth mentioning in the commit message that directly sharing
>>> MemoryRegion in CPUAddressSpace is safe.
>>
>> It's unnecessary to me. It's common that different Address space share the
>> same (root) memory region. e.g., for address space 0 for the cpu, though
>> what passed in is cpu->memory, they all point to system_memory.
> 
> For cpu->memory, there's the "object_ref(OBJECT(cpu->memory))" in
> cpu_exec_initfn().
> 
> But this case doesn't need to increase ref count like cpu->memory, since
> memory_region_ref() provides protection and it's enough.
> 
> This is the difference.
> 
> So it sounds like now it's more necessary to clarify this, no?
> 

clarify why smram_as_root doesn't need to be object_ref()'ed explicitly 
like what cpu_exec_initfn() does for cpu->memory?

As you saide,

cpu_address_space_init()
   -> address_space_init()
      -> memory_region_ref()

it already ensures the ref count is increased.

Why cpu_exec_initfn() increases the refcount of cpu->memory, is totally 
unrelated to cpu_address_space_init().

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Zhao Liu 3 months, 2 weeks ago

On Thu, Jul 31, 2025 at 12:11:41AM +0800, Xiaoyao Li wrote:
> Date: Thu, 31 Jul 2025 00:11:41 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace
> 
> On 7/30/2025 11:20 PM, Zhao Liu wrote:
> > > > > +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
> > > > 
> > > > It is worth mentioning in the commit message that directly sharing
> > > > MemoryRegion in CPUAddressSpace is safe.
> > > 
> > > It's unnecessary to me. It's common that different Address space share the
> > > same (root) memory region. e.g., for address space 0 for the cpu, though
> > > what passed in is cpu->memory, they all point to system_memory.
> > 
> > For cpu->memory, there's the "object_ref(OBJECT(cpu->memory))" in
> > cpu_exec_initfn().
> > 
> > But this case doesn't need to increase ref count like cpu->memory, since
> > memory_region_ref() provides protection and it's enough.
> > 
> > This is the difference.
> > 
> > So it sounds like now it's more necessary to clarify this, no?
> > 
> 
> clarify why smram_as_root doesn't need to be object_ref()'ed explicitly like
> what cpu_exec_initfn() does for cpu->memory?

When you're referring cpu->memory, you should aware where's the
difference and why you don't need do the same thing.

This is necessary for a clear commit message, and it also allows more
eyes to check whether it is correct and whether there are any potential
problems. Providing details is always beneficial.

> As you saide,
> 
> cpu_address_space_init()
>   -> address_space_init()
>      -> memory_region_ref()
> 
> it already ensures the ref count is increased.

Yes, this is what I mean.

> Why cpu_exec_initfn() increases the refcount of cpu->memory, is
> totally unrelated to cpu_address_space_init().
  ^^^^^^^^^^^^^^^^^

The validity of cpu->memory must be guaranteed, as this is a prerequisite
for everything else. And isn't it mainly related with the CPU address
space??

It can be said that because the pointer to system_memory needs to be
cached in CPUState, such a ref is useful, thereby ensuring the safety
of the next address space stuff.

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Kirill Martynov 2 months, 4 weeks ago

Hi Paolo,
Would you have time to share your thoughts about this set of patches?

> On 31 Jul 2025, at 06:53, Zhao Liu <zhao1.liu@intel.com> wrote:
> 
> On Thu, Jul 31, 2025 at 12:11:41AM +0800, Xiaoyao Li wrote:
>> Date: Thu, 31 Jul 2025 00:11:41 +0800
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace
>> 
>> On 7/30/2025 11:20 PM, Zhao Liu wrote:
>>>>>> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
>>>>> 
>>>>> It is worth mentioning in the commit message that directly sharing
>>>>> MemoryRegion in CPUAddressSpace is safe.
>>>> 
>>>> It's unnecessary to me. It's common that different Address space share the
>>>> same (root) memory region. e.g., for address space 0 for the cpu, though
>>>> what passed in is cpu->memory, they all point to system_memory.
>>> 
>>> For cpu->memory, there's the "object_ref(OBJECT(cpu->memory))" in
>>> cpu_exec_initfn().
>>> 
>>> But this case doesn't need to increase ref count like cpu->memory, since
>>> memory_region_ref() provides protection and it's enough.
>>> 
>>> This is the difference.
>>> 
>>> So it sounds like now it's more necessary to clarify this, no?
>>> 
>> 
>> clarify why smram_as_root doesn't need to be object_ref()'ed explicitly like
>> what cpu_exec_initfn() does for cpu->memory?
> 
> When you're referring cpu->memory, you should aware where's the
> difference and why you don't need do the same thing.
> 
> This is necessary for a clear commit message, and it also allows more
> eyes to check whether it is correct and whether there are any potential
> problems. Providing details is always beneficial.
> 
>> As you saide,
>> 
>> cpu_address_space_init()
>>  -> address_space_init()
>>     -> memory_region_ref()
>> 
>> it already ensures the ref count is increased.
> 
> Yes, this is what I mean.
> 
>> Why cpu_exec_initfn() increases the refcount of cpu->memory, is
>> totally unrelated to cpu_address_space_init().
>  ^^^^^^^^^^^^^^^^^
> 
> The validity of cpu->memory must be guaranteed, as this is a prerequisite
> for everything else. And isn't it mainly related with the CPU address
> space??
> 
> It can be said that because the pointer to system_memory needs to be
> cached in CPUState, such a ref is useful, thereby ensuring the safety
> of the next address space stuff.

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Kirill Martynov 3 months, 2 weeks ago


> On 30 Jul 2025, at 11:11, Zhao Liu <zhao1.liu@intel.com> wrote:
> 
>> @@ -91,6 +92,15 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>>         kvm_set_guest_phys_bits(cs);
>>     }
>> 
>> +    /*
>> +     * When SMM is enabled, there is 2 address spaces. Otherwise only 1.
>> +     *
>> +     * Only init address space 0 here, the second one for SMM is initialized at
>               ^^^^
> 	       initialize
> 
>> +     * register_smram_listener() after machine init done.
>> +     */
>> +    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
>> +    cpu_address_space_init(cs, 0, "cpu-mmeory", cs->memory);
>> +
>>     return true;
>> }
>> 
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 369626f8c8d7..47fb5c673c8e 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2704,6 +2704,7 @@ static MemoryRegion smram_as_mem;
>> 
>> static void register_smram_listener(Notifier *n, void *unused)
>> {
>> +    CPUState *cpu;
>>     MemoryRegion *smram =
>>         (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>> 
>> @@ -2728,6 +2729,10 @@ static void register_smram_listener(Notifier *n, void *unused)
>>     address_space_init(&smram_address_space, &smram_as_root, "KVM-SMRAM");
>>     kvm_memory_listener_register(kvm_state, &smram_listener,
>>                                  &smram_address_space, 1, "kvm-smram");
>> +
>> +    CPU_FOREACH(cpu) {
>> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
> 
> It is worth mentioning in the commit message that directly sharing
> MemoryRegion in CPUAddressSpace is safe.
> 
>> +    }
> 
> I still think such CPU_FOREACH in machine_done callback is not the
> best approach - it's better to initialize all the address spaces in
> kvm_cpu_realizefn(), and not to go far away from cs->num_ases, as I
> said in the previous discussion.
> 
> But it's still good to fix this bug. So, with other comments
> addressed,
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> 

Tested-by: Kirill Martynov <stdcalllevi@yandex-team.ru <mailto:stdcalllevi@yandex-team.ru>>

Re: [PATCH 1/2] i386/cpu: Enable SMM cpu addressspace

Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago

On 29/7/25 07:40, Xiaoyao Li wrote:
> Kirill Martynov reported assertation in cpu_asidx_from_attrs() being hit
> when x86_cpu_dump_state() is called to dump the CPU state[*]. It happens
> when the CPU is in SMM and KVM emulation failure due to misbehaving
> guest.
> 
> The root cause is that QEMU i386 never enables the SMM addressspace for cpu

"address space"

> since kvm SMM support has been added.
> 
> Enable the SMM cpu address space under KVM when the SMM is enabled for
> the x86machine.
> 
> [*] https://lore.kernel.org/qemu-devel/20250523154431.506993-1-stdcalllevi@yandex-team.ru/
> 
> Reported-by: Kirill Martynov <stdcalllevi@yandex-team.ru>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   system/physmem.c          |  5 -----
>   target/i386/kvm/kvm-cpu.c | 10 ++++++++++
>   target/i386/kvm/kvm.c     |  5 +++++
>   3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 130c148ffb5c..76e1c33aab5c 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -795,9 +795,6 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>           cpu->as = as;
>       }
>   
> -    /* KVM cannot currently support multiple address spaces. */
> -    assert(asidx == 0 || !kvm_enabled());
> -
>       if (!cpu->cpu_ases) {
>           cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
>           cpu->cpu_ases_count = cpu->num_ases;
> @@ -820,8 +817,6 @@ void cpu_address_space_destroy(CPUState *cpu, int asidx)
>   
>       assert(cpu->cpu_ases);
>       assert(asidx >= 0 && asidx < cpu->num_ases);
> -    /* KVM cannot currently support multiple address spaces. */
> -    assert(asidx == 0 || !kvm_enabled());
>   
>       cpuas = &cpu->cpu_ases[asidx];
>       if (tcg_enabled()) {
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 89a795365945..aa657c2a4627 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -13,6 +13,7 @@
>   #include "qapi/error.h"
>   #include "system/system.h"
>   #include "hw/boards.h"
> +#include "hw/i386/x86.h"
>   
>   #include "kvm_i386.h"
>   #include "accel/accel-cpu-target.h"
> @@ -91,6 +92,15 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>           kvm_set_guest_phys_bits(cs);
>       }
>   
> +    /*
> +     * When SMM is enabled, there is 2 address spaces. Otherwise only 1.
> +     *
> +     * Only init address space 0 here, the second one for SMM is initialized at
> +     * register_smram_listener() after machine init done.
> +     */
> +    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
> +    cpu_address_space_init(cs, 0, "cpu-mmeory", cs->memory);

Typo "memory".

> +
>       return true;
>   }
>   
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 369626f8c8d7..47fb5c673c8e 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2704,6 +2704,7 @@ static MemoryRegion smram_as_mem;
>   
>   static void register_smram_listener(Notifier *n, void *unused)
>   {
> +    CPUState *cpu;
>       MemoryRegion *smram =
>           (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>   
> @@ -2728,6 +2729,10 @@ static void register_smram_listener(Notifier *n, void *unused)
>       address_space_init(&smram_address_space, &smram_as_root, "KVM-SMRAM");
>       kvm_memory_listener_register(kvm_state, &smram_listener,
>                                    &smram_address_space, 1, "kvm-smram");
> +
> +    CPU_FOREACH(cpu) {
> +        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
> +    }
>   }
>   
>   static void *kvm_msr_energy_thread(void *data)