Hi Peter, > From: Peter Maydell <peter.mayd...@linaro.org> > Sent: Tuesday, May 7, 2024 10:03 AM > > 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- > philmd@linar > > o.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.
Agreed but It is a trade-off between 'lines of code' and 'clarity of the code'. Ideally, if someone is adding a initialization part one must consciously verify the locations where these will get deallocated/deinited as well so I do not think there is an escape from that kind of maintenance part. For arch like x86, where we are doing default AddressSpace allocation from common realize, it is more obvious to symmetrically destroy from common leg but for any other architecture including ARM where many types of CPU AddressSpace are getting allocated it would not be obvious to find its destruction leg buried somewhere in the common code. Hence, I would suggest to make release of the AddressSpace from common unrealization part optional which by default it is in the code excerpt from the patch: https://lore.kernel.org/qemu-devel/20230918160257.30127-9-phi...@linaro.org/ + /* Destroy CPU address space */ + for (unsigned idx = 0; idx < cpu->num_ases; idx++) { + cpu_address_space_destroy(cpu, idx); + } cpu->num_ases will be 0 in case Arch specific code has already destroyed it. That said we will still need some modification in the address space destruction function to gracefully return in case that CPU AddressSpace was already destroyed or does not exist. This will keep both options open. Thanks Salil. > > thanks > -- PMM
