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

[David Hildenbrand]

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.

[...]

> > 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.
> >
> > 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?
> >
> > 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.

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

--
Cheers,

David / dhildenb