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

[Zhao Liu <zhao1.liu@intel.com>]

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