Hi Gavin,

> From: Gavin Shan <gshan@redhat.com>
> Sent: Thursday, July 27, 2023 1:57 AM
> 
> Hi Xianglai,
> 
> 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!

Best wishes,
Salil.

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