On Tue, 7 May 2024 at 01:11, Salil Mehta <salil.me...@huawei.com> wrote: > > > From: Peter Maydell <peter.mayd...@linaro.org> > > Sent: Monday, May 6, 2024 10:29 AM > > To: Salil Mehta <salil.me...@huawei.com> > > > > On Mon, 6 May 2024 at 10:06, Salil Mehta <salil.me...@huawei.com> > > wrote: > > > > > > Hi Peter, > > > > > > Thanks for the review. > > > > > > > From: Peter Maydell <peter.mayd...@linaro.org> 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(). > > > > How can you be taking care of *x86* CPU types in the Arm unrealize? > > > Sorry, yes, I missed to reply that clearly. There was indeed a leak with x86 > reported > by Phillipe/David last year. In fact, Phillipe floated a patch last year for > this. > I thought it was fixed already as part of cpu_common_unrealize() but I just > checked and realized that the below proposed changed still isn’t part of the > mainline > > https://lore.kernel.org/qemu-devel/20230918160257.30127-9-phi...@linaro.org/
That seems like the right way to clean these up -- Philippe, do you want to fish that bugfix out of your big patchset and submit it separately ? > I can definitely add a common CPU AddressSpace destruction leg as part of this > patch if in case arch specific CPU unrealize does not cleans up its CPU > AddressSpace? Arch-specific CPU unrealize shouldn't need to do anything -- if we fix this similarly to Philippe's patch above then that will do the cleanup required. Handling this kind of cleanup in common code is more reliable because it doesn't require every target-arch maintainer to remember it needs to be done, plus it's less code. thanks -- PMM
