| Message ID | cover.1689837093.git.lixianglai@loongson.cn |
|---|---|
| Headers | show |
| Series | Adds CPU hot-plug support to Loongarch | expand |
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? Thanks, Gavin
Hi Gavin and Salil, On 7/27/23 8:57 AM, Gavin Shan wrote: > 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. We have great respect and gratitude to Salil and his team for their work and contributions to CPU HotPlug, which greatly reduced the difficulty of developing CPU HotPlug in Loongarch. So, We plan to rebase the next version of patch based on their code. Hi Salil, I estimate that it will take quite some time for your patch series to merge in, if allowed, can you merge your patch series changes to the common code section into the community first, so that our code can be rebase and merged. > > 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? At present, QEMU only supports TCG under Loongarch, and we test CPU HotPlug is also carried out under QEMU TCG, and CPU HotPlug can work normally when testing. Of course, we are also very happy to see you testing CPU hotplug under Loongarch, which can find more problems and help us improve our patch. > > Thanks, > Gavin Thanks, xianglai li
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
Hi, Salil On 2023/7/27 pm 10:51, Salil Mehta wrote: > Hello, > >> From: lixianglai <lixianglai@loongson.cn> >> Sent: Thursday, July 27, 2023 3:14 AM >> To: Gavin Shan <gshan@redhat.com>; qemu-devel@nongnu.org; Salil Mehta >> <salil.mehta@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Bibo Mao >> <maobibo@loongson.cn> >> Subject: Re: [PATCH 0/8] Adds CPU hot-plug support to Loongarch >> >> Hi Gavin and Salil, >> >> On 7/27/23 8:57 AM, Gavin Shan wrote: >>> 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. >> We have great respect and gratitude to Salil and his team for their work >> and contributions to CPU HotPlug, > > Many thanks! Really appreciate this. > > >> which greatly reduced the difficulty of developing CPU HotPlug in >> Loongarch. > > We are glad that this work is useful to other companies as well this > was one of our goal. > > >> So, We plan to rebase the next version of patch based on their code. > > Great. Thanks for clarifying this as accountability is important > even though we are working in a collaborative environment. > > As such, I am planning to send the RFC V2 in 2 weeks of time and > will make sure that the patches which are required by your patch-set > are formed in such a way that they can be independently accepted > w.r.t rest of the ARM64 architecture specific patch-set. > > >> Hi Salil, >> >> I estimate that it will take quite some time for your patch series to >> merge in, >> >> if allowed, can you merge your patch series changes to the common code >> section into the community first, >> >> so that our code can be rebase and merged. > > Sure, as clarified above, something similar, will create a patch-set > which will have patches which can be independently accepted/Ack'ed > and consumed even before the complete patch-set. > > Although I think, even in current form most patches have been arranged > in such a way. But I will doubly check again before I float RFC V2. I am sorry for the late reply. Thanks very much,We look forward to joining the community with your patch series. > >>> 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? >> At present, QEMU only supports TCG under Loongarch, >> >> and we test CPU HotPlug is also carried out under QEMU TCG, >> >> and CPU HotPlug can work normally when testing. >> >> Of course, we are also very happy to see you testing CPU hotplug under >> Loongarch, >> >> which can find more problems and help us improve our patch. > > We know that. There are some trivial fixes we already have, I will push > them as well for the completion. We were not sure if anybody needs them > as usage of vCPU Hotplug under 'accel=tcg' is highly unlikely except for > testing cases. Please let us know if you have any? No, we don't have it yet. > > Many thanks! > Salil. >