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

[Gavin Shan <gshan@redhat.com>]

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