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

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

Hi Xianglai,

> From: lixianglai <lixianglai@loongson.cn>
> Sent: Friday, September 15, 2023 3:48 AM
> To: David Hildenbrand <david@redhat.com>; 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
> 
> 
> 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"
> 
> >
> > What happens if you re-add that CPU? Will we reuse the previous
> > address space?
> 
> 
> Here is the memory layout where I inserted cpu1 again. It does not
> appear that the original address space was reused, and the address space
> is now duplicated
> 
> info mtree
> 
> address-space: cpu-memory-0
> address-space: cpu-memory-1
> address-space: cpu-memory-1
> 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-00000000000affff (prio 2, ram): alias vga.chain4
> @vga.vram 0000000000000000-000000000000ffff
>        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>        00000000000e0000-00000000000fffff (prio 1, rom): alias isa-bios
> @pc.bios 0000000000020000-000000000003ffff
>        00000000fd000000-00000000fdffffff (prio 1, ram): vga.vram
> 
> 
> In addition, I do not find the corresponding resource release action for
> cpu->cpu_ases requested in function cpu_address_space_init.
> 
> I wonder if there is a leak in the memory space requested here. Maybe
> qemu automatically reclaims memory space
> 
> or frees resources somewhere else I didn't find? I thought I'd try
> running the following valgrind to see if I could verify my suspicions.
> 
> void cpu_address_space_init(CPUState *cpu, int asidx,
>                              const char *prefix, MemoryRegion *mr)
> {
> 
> ...
> 
>      if (!cpu->cpu_ases) {
>          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
>      }
> 
> ...
> 
> }
> 
> >
> >> info mtree
> >>
> >> address-space: cpu-memory-0
> >> address-space: cpu-memory-1
> >> 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
> >>
> >>
> >>   From the above test, you can see whether the address space of cpu1 is
> >> residual after a cpu hot swap, and whether it is reasonable?
> >
> >
> > Probably we should teach other archs to destroy that address space as
> > well.
> >
> > Can we do that from the core, instead of having to do that in each CPU
> > unrealize function?
> >
> I think it can also be done in the public code flow. Since I refer to
> arm's scheme
> 
> (https://lore.kernel.org/all/20200613213629.21984-1-
> salil.mehta@huawei.com/),
> 
> 
> and arm's patch will be issued soon, I will conduct rebase based on arm
> patch in the future.


Here it is:
https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/T/#m523b37819c4811c7827333982004e07a1ef03879



> 
> Therefore, I would like to see if arm has any good suggestions. If there
> are no good suggestions at this stage,
> 
> I think we can shelve this problem for the first time, and I can
> consider not referencing this function for the first time,
> 
> and we can submit another patch to solve this problem.
> 
> Hi Salil Mehta:
> 
> Is the cpu_address_space_destroy function still present in the new patch
> version of arm?

Yes, this is present in the RFC V2. Please find it here

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



> 
> Can we put this function on the public path of cpu destroy?

Yes, AddressSpace destruction is already part of the Architecture
agnostic patches. Please rebase you patch-set and you will
see your bugs disappearing :)


Thanks
Salil.