[PATCH 0/2] i386/kvm: Enable SMM addrss space for i386 cpu

[PATCH 0/2] i386/kvm: Enable SMM addrss space for i386 cpu

[Xiaoyao Li]

Patch 1 enables the SMM address space i386 cpu under KVM.

Patch 2 gives name for each address space index.

Xiaoyao Li (2):
  i386/cpu: Enable SMM cpu addressspace
  target/i386: Define enum X86ASIdx for x86's address spaces

 accel/kvm/kvm-all.c              |  2 +-
 system/physmem.c                 |  5 -----
 target/i386/cpu.h                |  5 +++++
 target/i386/kvm/kvm-cpu.c        | 10 ++++++++++
 target/i386/kvm/kvm.c            |  7 ++++++-
 target/i386/tcg/system/tcg-cpu.c |  4 ++--
 6 files changed, 24 insertions(+), 9 deletions(-)

-- 
2.43.0

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

[Xiaoyao Li]

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

[PATCH 2/2] target/i386: Define enum X86ASIdx for x86's address spaces

[Xiaoyao Li]

Like ARM defines ARMASIdx, do the same to define X86ASIdx as enum. So
that it's more clear what index 0 is for memory and index 1 is for SMM.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 accel/kvm/kvm-all.c              | 2 +-
 target/i386/cpu.h                | 5 +++++
 target/i386/kvm/kvm-cpu.c        | 2 +-
 target/i386/kvm/kvm.c            | 4 ++--
 target/i386/tcg/system/tcg-cpu.c | 4 ++--
 5 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 890d5ea9f865..e56c217a5a0d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2797,7 +2797,7 @@ static int kvm_init(AccelState *as, MachineState *ms)
     s->memory_listener.listener.coalesced_io_del = kvm_uncoalesce_mmio_region;
 
     kvm_memory_listener_register(s, &s->memory_listener,
-                                 &address_space_memory, 0, "kvm-memory");
+                                 &address_space_memory, X86ASIdx_MEM, "kvm-memory");
     memory_listener_register(&kvm_io_listener,
                              &address_space_io);
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f977fc49a774..e0be7a740685 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2574,6 +2574,11 @@ static inline bool x86_has_cpuid_0x1f(X86CPU *cpu)
 void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
 void cpu_sync_avx_hflag(CPUX86State *env);
 
+typedef enum X86ASIdx {
+    X86ASIdx_MEM = 0,
+    X86ASIdx_SMM = 1,
+} X86ASIdx;
+
 #ifndef CONFIG_USER_ONLY
 static inline int x86_asidx_from_attrs(CPUState *cs, MemTxAttrs attrs)
 {
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index aa657c2a4627..36f5892d330e 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -99,7 +99,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      * 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);
+    cpu_address_space_init(cs, X86ASIdx_MEM, "cpu-mmeory", cs->memory);
 
     return true;
 }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 47fb5c673c8e..5621200be0f0 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2728,10 +2728,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");
+                                 &smram_address_space, X86ASIdx_SMM, "kvm-smram");
 
     CPU_FOREACH(cpu) {
-        cpu_address_space_init(cpu, 1, "cpu-smm", &smram_as_root);
+        cpu_address_space_init(cpu, X86ASIdx_SMM, "cpu-smm", &smram_as_root);
     }
 }
 
diff --git a/target/i386/tcg/system/tcg-cpu.c b/target/i386/tcg/system/tcg-cpu.c
index 0538a4fd51a3..7255862c2449 100644
--- a/target/i386/tcg/system/tcg-cpu.c
+++ b/target/i386/tcg/system/tcg-cpu.c
@@ -74,8 +74,8 @@ bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
     memory_region_set_enabled(cpu->cpu_as_mem, true);
 
     cs->num_ases = 2;
-    cpu_address_space_init(cs, 0, "cpu-memory", cs->memory);
-    cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_as_root);
+    cpu_address_space_init(cs, X86ASIdx_MEM, "cpu-memory", cs->memory);
+    cpu_address_space_init(cs, X86ASIdx_SMM, "cpu-smm", cpu->cpu_as_root);
 
     /* ... SMRAM with higher priority, linked from /machine/smram.  */
     cpu->machine_done.notify = tcg_cpu_machine_done;
-- 
2.43.0

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

[Philippe Mathieu-Daudé]

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)

Re: [PATCH 2/2] target/i386: Define enum X86ASIdx for x86's address spaces

[Philippe Mathieu-Daudé]

On 29/7/25 07:40, Xiaoyao Li wrote:
> Like ARM defines ARMASIdx, do the same to define X86ASIdx as enum. So
> that it's more clear what index 0 is for memory and index 1 is for SMM.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   accel/kvm/kvm-all.c              | 2 +-
>   target/i386/cpu.h                | 5 +++++
>   target/i386/kvm/kvm-cpu.c        | 2 +-
>   target/i386/kvm/kvm.c            | 4 ++--
>   target/i386/tcg/system/tcg-cpu.c | 4 ++--
>   5 files changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] target/i386: Define enum X86ASIdx for x86's address spaces

[Kirill Martynov]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

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

> On 29 Jul 2025, at 10:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> On 29/7/25 07:40, Xiaoyao Li wrote:
>> Like ARM defines ARMASIdx, do the same to define X86ASIdx as enum. So
>> that it's more clear what index 0 is for memory and index 1 is for SMM.
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>  accel/kvm/kvm-all.c              | 2 +-
>>  target/i386/cpu.h                | 5 +++++
>>  target/i386/kvm/kvm-cpu.c        | 2 +-
>>  target/i386/kvm/kvm.c            | 4 ++--
>>  target/i386/tcg/system/tcg-cpu.c | 4 ++--
>>  5 files changed, 11 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 


[-- Attachment #2: Type: text/html, Size: 1465 bytes --]

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

[Kirill Martynov]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]



> 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>>

[-- Attachment #2: Type: text/html, Size: 3430 bytes --]

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

[Zhao Liu]

> @@ -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 2/2] target/i386: Define enum X86ASIdx for x86's address spaces

[Zhao Liu]

On Tue, Jul 29, 2025 at 01:40:23PM +0800, Xiaoyao Li wrote:
> Date: Tue, 29 Jul 2025 13:40:23 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH 2/2] target/i386: Define enum X86ASIdx for x86's address
>  spaces
> X-Mailer: git-send-email 2.43.0
> 
> Like ARM defines ARMASIdx, do the same to define X86ASIdx as enum. So
> that it's more clear what index 0 is for memory and index 1 is for SMM.

Maybe:

Define X86ASIdx as the enum, like ARM's ARMASIdx, so that index 0 is
clearly for memory and index 1 for SMM.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  accel/kvm/kvm-all.c              | 2 +-
>  target/i386/cpu.h                | 5 +++++
>  target/i386/kvm/kvm-cpu.c        | 2 +-
>  target/i386/kvm/kvm.c            | 4 ++--
>  target/i386/tcg/system/tcg-cpu.c | 4 ++--
>  5 files changed, 11 insertions(+), 6 deletions(-)

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

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

[Xiaoyao Li]

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

[Zhao Liu]

> > > +        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

[Xiaoyao Li]

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

[Zhao Liu]

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

[Kirill Martynov]

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

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.


[-- Attachment #2: Type: text/html, Size: 15629 bytes --]