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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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