RE: [PATCH RFC V2 25/37] arm/virt: Add/update basic hot-(un)plug framework

[Salil Mehta]

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.