| Message ID | 20230926100436.28284-26-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
Hi Salil, On 9/26/23 20:04, Salil Mehta wrote: > Add CPU hot-unplug hooks and update hotplug hooks with additional sanity checks > for use in hotplug paths. > > Note, Functional contents of the hooks(now left with TODO comment) shall be > gradually filled in the subsequent patches in an incremental approach to patch > and logic building which would be roughly as follows: > 1. (Un-)wiring of interrupts between vCPU<->GIC > 2. Sending events to Guest for hot-(un)plug so that guest can take appropriate > actions. > 3. Notifying GIC about hot-(un)plug action so that vCPU could be (un-)stitched > to the GIC CPU interface. > 4. Updating the Guest with Next boot info for this vCPU in the firmware. > > Co-developed-by: Salil Mehta <salil.mehta@huawei.com> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > hw/arm/virt.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 3b068534a8..dce02136cb 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -81,6 +81,7 @@ > #include "hw/virtio/virtio-iommu.h" > #include "hw/char/pl011.h" > #include "qemu/guest-random.h" > +#include "qapi/qmp/qdict.h" > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ > @@ -2985,12 +2986,23 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > { > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > MachineState *ms = MACHINE(hotplug_dev); > + MachineClass *mc = MACHINE_GET_CLASS(ms); > ARMCPU *cpu = ARM_CPU(dev); > CPUState *cs = CPU(dev); > CPUArchId *cpu_slot; > int32_t min_cpuid = 0; > int32_t max_cpuid; > > + if (dev->hotplugged && !vms->acpi_dev) { > + error_setg(errp, "GED acpi device does not exists"); > + return; > + } > + > + if (dev->hotplugged && !mc->has_hotpluggable_cpus) { > + error_setg(errp, "CPU hotplug not supported on this machine"); > + return; > + } > + I guess these can be combined to: if (dev->hotplugged && (!mc->has_hotpluggable_cpus || !vms->acpi_dev)) { error_setg(errp, "CPU hotplug not supported or GED ACPI device not exist"); } Besides, need we check (vms->gic_version == VIRT_GIC_VERSION_3)? > /* sanity check the cpu */ > if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) { > error_setg(errp, "Invalid CPU type, expected cpu type: '%s'", > @@ -3039,6 +3051,22 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > } > virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); > > + /* > + * Fix the GIC for this new vCPU being plugged. The QOM CPU object for the > + * new vCPU need to be updated in the corresponding QOM GICv3CPUState object > + * We also need to re-wire the IRQs for this new CPU object. This update > + * is limited to the QOM only and does not affects the KVM. Later has > + * already been pre-sized with possible CPU at VM init time. This is a > + * workaround to the constraints posed by ARM architecture w.r.t supporting > + * CPU Hotplug. Specification does not exist for the later. > + * This patch-up is required both for {cold,hot}-plugged vCPUs. Cold-inited > + * vCPUs have their GIC state initialized during machvit_init(). > + */ > + if (vms->acpi_dev) { > + /* TODO: update GIC about this hotplug change here */ > + /* TODO: wire the GIC<->CPU irqs */ > + } > + When looking at these 'TODO', it seems you need order the patches to make those preparatory patches ahead of this one. In this way, the 'TODO' can be avoided. > /* > * To give persistent presence view of vCPUs to the guest, ACPI might need > * to fake the presence of the vCPUs to the guest but keep them disabled. > @@ -3050,6 +3078,7 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > MachineState *ms = MACHINE(hotplug_dev); > CPUState *cs = CPU(dev); > CPUArchId *cpu_slot; > @@ -3058,10 +3087,81 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); > cpu_slot->cpu = OBJECT(dev); > > + /* > + * Update the ACPI Hotplug state both for vCPUs being {hot,cold}-plugged. > + * vCPUs can be cold-plugged using '-device' option. For vCPUs being hot > + * plugged, guest is also notified. > + */ > + if (vms->acpi_dev) { > + /* TODO: update acpi hotplug state. Send cpu hotplug event to guest */ > + /* TODO: register cpu for reset & update F/W info for the next boot */ > + } > + We needn't validate vms->acpi_dev again since it has been done in pre_plug(). > cs->disabled = false; > return; > } > > +static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > + ARMCPU *cpu = ARM_CPU(dev); > + CPUState *cs = CPU(dev); > + > + if (!vms->acpi_dev || !dev->realized) { > + error_setg(errp, "GED does not exists or device is not realized!"); > + return; > + } > + > + if (!mc->has_hotpluggable_cpus) { > + error_setg(errp, "CPU hot(un)plug not supported on this machine"); > + return; > + } > + > + if (cs->cpu_index == first_cpu->cpu_index) { > + error_setg(errp, "Boot CPU(id%d=%d:%d:%d:%d) hot-unplug not supported", > + first_cpu->cpu_index, cpu->socket_id, cpu->cluster_id, > + cpu->core_id, cpu->thread_id); > + return; > + } > + > + /* TODO: request cpu hotplug from guest */ > + > + return; > +} > + > +static void virt_cpu_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > + MachineState *ms = MACHINE(hotplug_dev); > + CPUState *cs = CPU(dev); > + CPUArchId *cpu_slot; > + > + if (!vms->acpi_dev || !dev->realized) { > + error_setg(errp, "GED does not exists or device is not realized!"); > + return; > + } > + > + cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); > + > + /* TODO: update the acpi cpu hotplug state for cpu hot-unplug */ > + > + /* TODO: unwire the gic-cpu irqs here */ > + /* TODO: update the GIC about this hot unplug change */ > + > + /* TODO: unregister cpu for reset & update F/W info for the next boot */ > + Same as above. > + qobject_unref(dev->opts); > + dev->opts = NULL; > + > + cpu_slot->cpu = NULL; > + cs->disabled = true; > + > + return; > +} > + > static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > @@ -3185,6 +3285,8 @@ static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_unplug_request(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), > errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + virt_cpu_unplug_request(hotplug_dev, dev, errp); > } else { > error_setg(errp, "device unplug request for unsupported device" > " type: %s", object_get_typename(OBJECT(dev))); > @@ -3198,6 +3300,8 @@ static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev, > virt_dimm_unplug(hotplug_dev, dev, errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_unplug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + virt_cpu_unplug(hotplug_dev, dev, errp); > } else { > error_setg(errp, "virt: device unplug for unsupported device" > " type: %s", object_get_typename(OBJECT(dev))); Thanks, Gavin
Hi Gavin, > From: Gavin Shan <gshan@redhat.com> > Sent: Friday, September 29, 2023 1:21 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org > Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > peter.maydell@linaro.org; richard.henderson@linaro.org; > imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com; > philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; > rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org; > linux@armlinux.org.uk; darren@os.amperecomputing.com; > ilkka@os.amperecomputing.com; vishnu@os.amperecomputing.com; > karl.heubaum@oracle.com; miguel.luis@oracle.com; salil.mehta@opnsrc.net; > zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng (C) > <wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>; > jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn > Subject: Re: [PATCH RFC V2 25/37] arm/virt: Add/update basic hot-(un)plug > framework > > Hi Salil, > > On 9/26/23 20:04, Salil Mehta wrote: > > Add CPU hot-unplug hooks and update hotplug hooks with additional sanity checks > > for use in hotplug paths. > > > > Note, Functional contents of the hooks(now left with TODO comment) shall be > > gradually filled in the subsequent patches in an incremental approach to patch > > and logic building which would be roughly as follows: > > 1. (Un-)wiring of interrupts between vCPU<->GIC > > 2. Sending events to Guest for hot-(un)plug so that guest can take appropriate > > actions. > > 3. Notifying GIC about hot-(un)plug action so that vCPU could be (un-)stitched > > to the GIC CPU interface. > > 4. Updating the Guest with Next boot info for this vCPU in the firmware. > > > > Co-developed-by: Salil Mehta <salil.mehta@huawei.com> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com> > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> [...] > > @@ -2985,12 +2986,23 @@ static void virt_cpu_pre_plug(HotplugHandler > *hotplug_dev, DeviceState *dev, > > { > > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > > MachineState *ms = MACHINE(hotplug_dev); > > + MachineClass *mc = MACHINE_GET_CLASS(ms); > > ARMCPU *cpu = ARM_CPU(dev); > > CPUState *cs = CPU(dev); > > CPUArchId *cpu_slot; > > int32_t min_cpuid = 0; > > int32_t max_cpuid; > > > > + if (dev->hotplugged && !vms->acpi_dev) { > > + error_setg(errp, "GED acpi device does not exists"); > > + return; > > + } > > + > > + if (dev->hotplugged && !mc->has_hotpluggable_cpus) { > > + error_setg(errp, "CPU hotplug not supported on this machine"); > > + return; > > + } > > + > > I guess these can be combined to: > > if (dev->hotplugged && (!mc->has_hotpluggable_cpus || !vms->acpi_dev)) { > error_setg(errp, "CPU hotplug not supported or GED ACPI device not exist"); > } Above checks exists because I wanted different error strings for each. > > Besides, need we check (vms->gic_version == VIRT_GIC_VERSION_3)? Flag ' mc->has_hotpluggable_cpus' takes care all of that. > > > /* sanity check the cpu */ > > if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) { > > error_setg(errp, "Invalid CPU type, expected cpu type: '%s'", > > @@ -3039,6 +3051,22 @@ static void virt_cpu_pre_plug(HotplugHandler > *hotplug_dev, DeviceState *dev, > > } > > virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); > > > > + /* > > + * Fix the GIC for this new vCPU being plugged. The QOM CPU object for the > > + * new vCPU need to be updated in the corresponding QOM GICv3CPUState object > > + * We also need to re-wire the IRQs for this new CPU object. This update > > + * is limited to the QOM only and does not affects the KVM. Later has > > + * already been pre-sized with possible CPU at VM init time. This is a > > + * workaround to the constraints posed by ARM architecture w.r.t supporting > > + * CPU Hotplug. Specification does not exist for the later. > > + * This patch-up is required both for {cold,hot}-plugged vCPUs. Cold-inited > > + * vCPUs have their GIC state initialized during machvit_init(). > > + */ > > + if (vms->acpi_dev) { > > + /* TODO: update GIC about this hotplug change here */ > > + /* TODO: wire the GIC<->CPU irqs */ > > + } > > + > > When looking at these 'TODO', it seems you need order the patches to make those > preparatory patches ahead of this one. In this way, the 'TODO' can be avoided. Maybe but it will break step wise flow of the patch-set [...] > > @@ -3058,10 +3087,81 @@ static void virt_cpu_plug(HotplugHandler > *hotplug_dev, DeviceState *dev, > > cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); > > cpu_slot->cpu = OBJECT(dev); > > > > + /* > > + * Update the ACPI Hotplug state both for vCPUs being {hot,cold}-plugged. > > + * vCPUs can be cold-plugged using '-device' option. For vCPUs being hot > > + * plugged, guest is also notified. > > + */ > > + if (vms->acpi_dev) { > > + /* TODO: update acpi hotplug state. Send cpu hotplug event to guest */ > > + /* TODO: register cpu for reset & update F/W info for the next boot */ > > + } > > + > > We needn't validate vms->acpi_dev again since it has been done in pre_plug(). We want this leg to be conditional for cold inited CPUs. [...] > > +static void virt_cpu_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > > + Error **errp) > > +{ > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > > + MachineState *ms = MACHINE(hotplug_dev); > > + CPUState *cs = CPU(dev); > > + CPUArchId *cpu_slot; > > + > > + if (!vms->acpi_dev || !dev->realized) { > > + error_setg(errp, "GED does not exists or device is not realized!"); > > + return; > > + } > > + > > + cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); > > + > > + /* TODO: update the acpi cpu hotplug state for cpu hot-unplug */ > > + > > + /* TODO: unwire the gic-cpu irqs here */ > > + /* TODO: update the GIC about this hot unplug change */ > > + > > + /* TODO: unregister cpu for reset & update F/W info for the next boot */ > > + > > Same as above. I understand your point. But will spoil the flow of path-set Thanks Salil.
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 3b068534a8..dce02136cb 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -81,6 +81,7 @@ #include "hw/virtio/virtio-iommu.h" #include "hw/char/pl011.h" #include "qemu/guest-random.h" +#include "qapi/qmp/qdict.h" #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ @@ -2985,12 +2986,23 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, { VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); MachineState *ms = MACHINE(hotplug_dev); + MachineClass *mc = MACHINE_GET_CLASS(ms); ARMCPU *cpu = ARM_CPU(dev); CPUState *cs = CPU(dev); CPUArchId *cpu_slot; int32_t min_cpuid = 0; int32_t max_cpuid; + if (dev->hotplugged && !vms->acpi_dev) { + error_setg(errp, "GED acpi device does not exists"); + return; + } + + if (dev->hotplugged && !mc->has_hotpluggable_cpus) { + error_setg(errp, "CPU hotplug not supported on this machine"); + return; + } + /* sanity check the cpu */ if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) { error_setg(errp, "Invalid CPU type, expected cpu type: '%s'", @@ -3039,6 +3051,22 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, } virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); + /* + * Fix the GIC for this new vCPU being plugged. The QOM CPU object for the + * new vCPU need to be updated in the corresponding QOM GICv3CPUState object + * We also need to re-wire the IRQs for this new CPU object. This update + * is limited to the QOM only and does not affects the KVM. Later has + * already been pre-sized with possible CPU at VM init time. This is a + * workaround to the constraints posed by ARM architecture w.r.t supporting + * CPU Hotplug. Specification does not exist for the later. + * This patch-up is required both for {cold,hot}-plugged vCPUs. Cold-inited + * vCPUs have their GIC state initialized during machvit_init(). + */ + if (vms->acpi_dev) { + /* TODO: update GIC about this hotplug change here */ + /* TODO: wire the GIC<->CPU irqs */ + } + /* * To give persistent presence view of vCPUs to the guest, ACPI might need * to fake the presence of the vCPUs to the guest but keep them disabled. @@ -3050,6 +3078,7 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); MachineState *ms = MACHINE(hotplug_dev); CPUState *cs = CPU(dev); CPUArchId *cpu_slot; @@ -3058,10 +3087,81 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); cpu_slot->cpu = OBJECT(dev); + /* + * Update the ACPI Hotplug state both for vCPUs being {hot,cold}-plugged. + * vCPUs can be cold-plugged using '-device' option. For vCPUs being hot + * plugged, guest is also notified. + */ + if (vms->acpi_dev) { + /* TODO: update acpi hotplug state. Send cpu hotplug event to guest */ + /* TODO: register cpu for reset & update F/W info for the next boot */ + } + cs->disabled = false; return; } +static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); + ARMCPU *cpu = ARM_CPU(dev); + CPUState *cs = CPU(dev); + + if (!vms->acpi_dev || !dev->realized) { + error_setg(errp, "GED does not exists or device is not realized!"); + return; + } + + if (!mc->has_hotpluggable_cpus) { + error_setg(errp, "CPU hot(un)plug not supported on this machine"); + return; + } + + if (cs->cpu_index == first_cpu->cpu_index) { + error_setg(errp, "Boot CPU(id%d=%d:%d:%d:%d) hot-unplug not supported", + first_cpu->cpu_index, cpu->socket_id, cpu->cluster_id, + cpu->core_id, cpu->thread_id); + return; + } + + /* TODO: request cpu hotplug from guest */ + + return; +} + +static void virt_cpu_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); + MachineState *ms = MACHINE(hotplug_dev); + CPUState *cs = CPU(dev); + CPUArchId *cpu_slot; + + if (!vms->acpi_dev || !dev->realized) { + error_setg(errp, "GED does not exists or device is not realized!"); + return; + } + + cpu_slot = virt_find_cpu_slot(ms, cs->cpu_index); + + /* TODO: update the acpi cpu hotplug state for cpu hot-unplug */ + + /* TODO: unwire the gic-cpu irqs here */ + /* TODO: update the GIC about this hot unplug change */ + + /* TODO: unregister cpu for reset & update F/W info for the next boot */ + + qobject_unref(dev->opts); + dev->opts = NULL; + + cpu_slot->cpu = NULL; + cs->disabled = true; + + return; +} + static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { @@ -3185,6 +3285,8 @@ static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { virtio_md_pci_unplug_request(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + virt_cpu_unplug_request(hotplug_dev, dev, errp); } else { error_setg(errp, "device unplug request for unsupported device" " type: %s", object_get_typename(OBJECT(dev))); @@ -3198,6 +3300,8 @@ static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev, virt_dimm_unplug(hotplug_dev, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { virtio_md_pci_unplug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + virt_cpu_unplug(hotplug_dev, dev, errp); } else { error_setg(errp, "virt: device unplug for unsupported device" " type: %s", object_get_typename(OBJECT(dev)));