Re: [PATCH 0/8] Adds CPU hot-plug support to Loongarch

[Gavin Shan]

Hi Salil,

On 7/28/23 00:58, Salil Mehta wrote:
> > From: Gavin Shan <gshan@redhat.com>
> > Sent: Thursday, July 27, 2023 1:57 AM
> > On 7/20/23 17:15, xianglai li wrote:
> > > Hello everyone, We refer to the implementation of ARM CPU
> > > Hot-Plug to add GED-based CPU Hot-Plug support to Loongarch.
> > >
> > > The first 4 patches are changes to the QEMU common code,
> > > including adding GED support for CPU Hot-Plug, updating
> > > the ACPI table creation process, and adding qdev_disconnect_gpio_out_named
> > > and cpu_address_space_destroy interfaces to release resources
> > > when CPU un-plug.
> > >
> > > The last four patches are Loongarch architecture-related,
> > > and the modifications include the definition of the hook
> > > function related to the CPU Hot-(UN)Plug, the allocation
> > > and release of CPU resources when CPU Hot-(UN)Plug,
> > > the creation process of updating the ACPI table,
> > > and finally the custom switch for the CPU Hot-Plug.
>
> [...]
>
> > It seems the design for ARM64 hotplug has been greatly referred, but the authors
> > are missed to be copied if you were referring to the following repository.
> > There will be conflicts between those two patchsets as I can see and I'm not 
> > sure
> > how it can be resolved. In theory, one patchset needs to be rebased on another
> > one since they're sharing large amount of codes.
> >
> >     https://github.com/salil-mehta/qemu.git
> >     (branch: virt-cpuhp-armv8/rfc-v1-port11052023.dev-1)
> >
> > I took a quick scan on this series. Loongarch doesn't have ARM64's constraint 
> > due
> > to vGIC3 where all vCPUs's file descriptor needs to be in place. It means the 
> > possible
> > and not yet present vCPU needs to be visible to KVM. Without this constraint, 
> > the
> > implementation is simplified a lot.
> >
> > Besides, we found the vCPU hotplug isn't working for TCG when Salil's series was
> > tested. I'm not sure if we have same issue with this series, or TCG isn't a 
> > concern
> > to us at all?
>
>
> Sorry, I should have replied this in the other mail but have been on/off in last
> few days due to some medical reasons. But jotting it here:
>
> Yes, you are correct. And there are some trivial fixes we already have to make
> it work. In case it is useful to you, we are planning to add them for the sake
> of completion. Maybe you can try that in the RFC V2 or I'll share with you the
> repository once I push the fixes.
>
> Many thanks!

No worries, thanks a lot for your followup and clarify. I think it'd better to
make TCG working so that we have consistent vCPU hotplug capability for all
accelerators eventually. However, I didn't test with HVF and not sure about it.
I just checked your repository again and it seems these TCG fixes aren't there
yet.

  https://github.com/salil-mehta/qemu.git
  (virt-cpuhp-armv8/rfc-v1-port11052023.dev-1)

  /home/gshan/sandbox/src/qemu/main/build/qemu-system-aarch64           \
  -accel tcg -machine virt,gic-version=3,nvdimm=on -icount 1            \
  -cpu max -smp maxcpus=2,cpus=1,sockets=1,clusters=1,cores=1,threads=2
   :
  (qemu) device_add driver=max-arm-cpu,id=cpu1,thread-id=1
  Error: cpu(id1=0:0:0:1) with arch-id 1 exists

As to the cause, the CPU object isn't detached from the CPU slot as said
before.

Thanks,
Gavin