Hi Peter,
Thanks for the review.
> From: Peter Maydell <peter.mayd...@linaro.org>
> Sent: Saturday, May 4, 2024 2:41 PM
>
> On Tue, 12 Mar 2024 at 02:02, Salil Mehta <salil.me...@huawei.com>
> 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 <salil.me...@huawei.com>
> > Tested-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
> > Reviewed-by: Gavin Shan <gs...@redhat.com>
> > Tested-by: Xianglai Li <lixiang...@loongson.cn>
> > Tested-by: Miguel Luis <miguel.l...@oracle.com>
> > Reviewed-by: Shaoqin Huang <shahu...@redhat.com>
>
> > diff --git a/system/physmem.c b/system/physmem.c index
> > 6e9ed97597..61b32ac4f2 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int
> > asidx,
> >
> > if (!cpu->cpu_ases) {
> > cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> > + cpu->cpu_ases_count = cpu->num_ases;
> > }
> >
> > newas = &cpu->cpu_ases[asidx];
> > @@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int
> asidx,
> > }
> > }
> >
> > +void cpu_address_space_destroy(CPUState *cpu, int asidx) {
> > + CPUAddressSpace *cpuas;
> > +
> > + assert(cpu->cpu_ases);
> > + assert(asidx >= 0 && asidx < cpu->num_ases);
> > + /* KVM cannot currently support multiple address spaces. */
> > + assert(asidx == 0 || !kvm_enabled());
> > +
> > + cpuas = &cpu->cpu_ases[asidx];
> > + if (tcg_enabled()) {
> > + memory_listener_unregister(&cpuas->tcg_as_listener);
> > + }
> > +
> > + address_space_destroy(cpuas->as);
> > + g_free_rcu(cpuas->as, rcu);
> > +
> > + if (asidx == 0) {
> > + /* reset the convenience alias for address space 0 */
> > + cpu->as = NULL;
> > + }
> > +
> > + if (--cpu->cpu_ases_count == 0) {
> > + g_free(cpu->cpu_ases);
> > + cpu->cpu_ases = NULL;
> > + }
> > +}
>
> When do we need to destroy a single address space in this way that means
> we need to keep a count of how many ASes the CPU currently has? The
> commit message talks about the case when we unrealize the whole CPU
> object, but in that situation you can just throw away all the ASes at once
> (eg
> by calling some
> cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).
Yes, maybe, we can destroy all at once from common leg as well. I'd prefer this
to be done from the arch specific function for ARM to maintain the clarity &
symmetry of initialization and un-initialization legs. For now, all of these
address
space destruction is happening in context to the arm_cpu_unrealizefn().
It’s a kind of trade-off between little more code and clarity but I'm open to
further suggestions.
>
> Also, if we're leaking stuff here by failing to destroy it, is that a
> problem for
> existing CPU types like x86 that we can already hotplug?
No we are not. We are taking care of these in the ARM arch specific legs
within functions arm_cpu_(un)realizefn().
https://lore.kernel.org/all/20230926103654.34424-1-salil.me...@huawei.com/
Above change now would be part of Arch specific patch-set RFC V3 being prepared.
Thanks
Salil.
