Hi Gavin, Revisited your comments again. > From: Gavin Shan <[email protected]> > Sent: Tuesday, October 3, 2023 2:37 AM > To: Salil Mehta <[email protected]>; [email protected]; qemu- > [email protected] > Cc: [email protected]; [email protected]; Jonathan Cameron > <[email protected]>; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; zhukeqian > <[email protected]>; wangxiongfeng (C) <[email protected]>; > wangyanan (Y) <[email protected]>; [email protected]; > [email protected]; [email protected]; Linuxarm <[email protected]> > Subject: Re: [PATCH V2 08/10] physmem: Add helper function to destroy CPU > AddressSpace > > On 9/30/23 10:19, Salil Mehta wrote: > > Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also > > involves destruction of the CPU AddressSpace. Add common function to help > > destroy the CPU AddressSpace. > > > > Signed-off-by: Salil Mehta <[email protected]> > > --- > > include/exec/cpu-common.h | 8 ++++++++ > > include/hw/core/cpu.h | 1 + > > softmmu/physmem.c | 25 +++++++++++++++++++++++++ > > 3 files changed, 34 insertions(+) > > > > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > > index 41788c0bdd..eb56a228a2 100644 > > --- a/include/exec/cpu-common.h > > +++ b/include/exec/cpu-common.h > > @@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void); > > */ > > void cpu_address_space_init(CPUState *cpu, int asidx, > > const char *prefix, MemoryRegion *mr); > > +/** > > + * cpu_address_space_destroy: > > + * @cpu: CPU for which address space needs to be destroyed > > + * @asidx: integer index of this address space > > + * > > + * Note that with KVM only one address space is supported. > > + */ > > +void cpu_address_space_destroy(CPUState *cpu, int asidx); > > > > void cpu_physical_memory_rw(hwaddr addr, void *buf, > > hwaddr len, bool is_write); > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > index 648b5b3586..65d2ae4581 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -355,6 +355,7 @@ struct CPUState { > > QSIMPLEQ_HEAD(, qemu_work_item) work_list; > > > > CPUAddressSpace *cpu_ases; > > + int cpu_ases_count; > > int num_ases; > > AddressSpace *as; > > MemoryRegion *memory; > > @num_ases and @cpu_ases_count are duplicate to each other to some extent. > The > real problem is @cpu_ases is allocated at once and we need to make the > allocation > sparse. In that way, each CPU address space is independent and can be > destroyed > independently.
(revisiting this comment i.e. 'sparse') If you meant, the order of initialization and destruction might not be same then yes, AddressSpace can be *conditionally* allocated during CPU realization phase and should be *conditionally* destroyed as well during CPU un-realization phase. Later means, it is not safe to assume that indexes to the array of AddressSpace might be consecutive. Hence, their destruction at once can create problems. https://lore.kernel.org/qemu-devel/[email protected]/T/#m44b81fbbba33a346dd2292256012f86ef7c8e761 The sparse allocation for the CPU address space can be done > in > cpu_address_space_init() like below: > > #define CPU_ADDRESS_SPACE_MAX 8 > > struct CPUState { > CPUAddressSpace *cpu_ases[CPU_ADDRESS_SPACE_MAX]; > } Yes, this will also work but we will end up refactoring existing initialization interface. Do we require all of this when counter based approach also works without changing anything? Thanks Salil.
