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