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

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

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.