RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Peter,

Thanks for the review.

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Saturday, May 4, 2024 2:41 PM
>  
>  On Tue, 12 Mar 2024 at 02:02, Salil Mehta <salil.mehta@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.mehta@huawei.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@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.mehta@huawei.com/

Above change now would be part of Arch specific patch-set RFC V3 being prepared.


Thanks
Salil.