RE: [PATCH v2 04/10] Introduce the CPU address space destruction function

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

Hi David,

> From: David Hildenbrand <david@redhat.com>
> Sent: Friday, September 15, 2023 9:07 AM
> To: lixianglai <lixianglai@loongson.cn>; qemu-devel@nongnu.org; Salil Mehta
> <salil.mehta@huawei.com>
> Cc: Salil Mehta <salil.mehta@opnsrc.net>; Xiaojuan Yang
> <yangxiaojuan@loongson.cn>; Song Gao <gaosong@loongson.cn>; Michael S.
> Tsirkin <mst@redhat.com>; Igor Mammedov <imammedo@redhat.com>; Ani Sinha
> <anisinha@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard
> Henderson <richard.henderson@linaro.org>; Eduardo Habkost
> <eduardo@habkost.net>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>;
> Philippe Mathieu-Daudé <philmd@linaro.org>; wangyanan (Y)
> <wangyanan55@huawei.com>; Daniel P. Berrangé <berrange@redhat.com>; Peter
> Xu <peterx@redhat.com>; Bibo Mao <maobibo@loongson.cn>
> Subject: Re: [PATCH v2 04/10] Introduce the CPU address space destruction
> function
> 
> On 15.09.23 04:53, lixianglai wrote:
> > Hi David Hildenbrand:
> >
> >>
> >> Hi David Hildenbrand:
> >>> On 14.09.23 15:00, lixianglai wrote:
> >>>> Hi David:
> >>>
> >>> Hi!
> >>>
> >>>>
> >>>>> On 12.09.23 04:11, xianglai li wrote:
> >>>>>> Introduce new function to destroy CPU address space resources
> >>>>>> for cpu hot-(un)plug.
> >>>>>>
> >>>>> How do other archs handle that? Or how are they able to get away
> >>>>> without destroying?
> >>>>>
> >>>> They do not remove the cpu address space, taking the X86
> >>>> architecture as
> >>>> an example:
> >>>>
> >>>> 1.Start the x86 VM:
> >>>>
> >>>> ./qemu-system-x86_64 \
> >>>> -machine q35  \
> >>>> -cpu Broadwell-IBRS \
> >>>> -smp 1,maxcpus=100,sockets=100,cores=1,threads=1 \
> >>>> -m 4G \
> >>>> -drive file=~/anolis-8.8.qcow2  \
> >>>> -serial stdio   \
> >>>> -monitor telnet:localhost:4498,server,nowait   \
> >>>> -nographic
> >>>>
> >>>> 2.Connect the qemu monitor
> >>>>
> >>>> telnet 127.0.0.1 4498
> >>>>
> >>>> info mtree
> >>>>
> >>>> address-space: cpu-memory-0
> >>>> address-space: memory
> >>>>      0000000000000000-ffffffffffffffff (prio 0, i/o): system
> >>>>        0000000000000000-000000007fffffff (prio 0, ram): alias
> >>>> ram-below-4g
> >>>> @pc.ram 0000000000000000-000000007fffffff
> >>>>        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
> >>>>          00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
> >>>>
> >>>> 3.Perform cpu hot swap int qemu monitor
> >>>>
> >>>> device_add
> >>>> Broadwell-IBRS-x86_64-cpu,socket-id=1,core-id=0,thread-id=0,id=cpu1
> >>>> device_del cpu1
> >>>>
> >>>
> >>> Hm, doesn't seem to work for me on upstream QEMU for some reason:
> >>> "Error: acpi: device unplug request for not supported device type:
> >>> Broadwell-IBRS-x86_64-cpu"
> >>
> > First I use qemu tcg, and then the cpu needs to be removed after the
> > operating system is booted.
> 
> Ah, the last thing is the important bit. I can reproduce this with KVM
> easily.
> 
> Doing it a couple of times
> 
> address-space: cpu-memory-0
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> 
> Looks like a resource/memory leak.

Yes, there was. Thanks for identifying it. I have fixed in the
latest RFC V2. Please check here:

https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/T/#m5f5ae40b091d69d01012880d7500d96874a9d39c

I have tested and AddressSpace comes and goes away cleanly
on CPU hot(un)plug action.



[...]


> >> Hi Salil Mehta:
> >>
> >> Is the cpu_address_space_destroy function still present in the new
> >> patch version of arm?
> >>
> >> Can we put this function on the public path of cpu destroy?
> 
> Looks like this has to be fixed for all archs that support VCPU unplug.
> 
> The CPU implementation end up call qemu_init_vcpu() in their realize
> function; there should be something like qemu_destroy_vcpu() on the
> unrealize path that takes care of undoing any cpu_address_space_init().
> 
> We seem to have cpu_common_unrealizefn()->cpu_exec_unrealizefn() but
> that doesn't take care of address spaces.

Yes, the current concept can be extended

> 
> Also, in qemu_init_vcpu() we do a cpus_accel->create_vcpu_thread(cpu).
> I'm, curious if we destroy that thread somehow.


Yes we do. In ARM RFC, this happens after CPU has been unplugged and
CPU is being destroyed and unparented (all of this happens in context
ACPI _EJx method evaluated by OSPM). This eventually leads to
architecture specific CPU unrealize and call to cpu_remove_sync().


Thanks
Salil.