Re: [PATCH RFC V3 11/29] arm/virt: Create GED dev before *disabled* CPU Objs are destroyed

[Gavin Shan]

Hi Salil,

On 8/19/24 10:10 PM, Salil Mehta wrote:
> >   From: Gavin Shan <gshan@redhat.com>
> >   Sent: Tuesday, August 13, 2024 2:05 AM
> >   To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
> >   qemu-arm@nongnu.org; mst@redhat.com
> >   
> >   On 6/14/24 9:36 AM, Salil Mehta wrote:
> >   > ACPI CPU hotplug state (is_present=_STA.PRESENT,
> >   > is_enabled=_STA.ENABLED) for all the possible vCPUs MUST be
> >   > initialized during machine init. This is done during the creation of
> >   > the GED device. VMM/Qemu MUST expose/fake the ACPI state of the
> >   > disabled vCPUs to the Guest kernel as 'present' (_STA.PRESENT) always
> >   > i.e. ACPI persistent. if the 'disabled' vCPU objectes are destroyed
> >   > before the GED device has been created then their ACPI hotplug state
> >   > might not get initialized correctly as acpi_persistent flag is part of the
> >   CPUState. This will expose wrong status of the unplugged vCPUs to the
> >   Guest kernel.
> >   >
> >   > Hence, moving the GED device creation before disabled vCPU objects get
> >   > destroyed as part of the post CPU init routine.
> >   >
> >   > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> >   > ---
> >   >   hw/arm/virt.c | 10 +++++++---
> >   >   1 file changed, 7 insertions(+), 3 deletions(-)
> >   >
> >   > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index
> >   > 918bcb9a1b..5f98162587 100644
> >   > --- a/hw/arm/virt.c
> >   > +++ b/hw/arm/virt.c
> >   > @@ -2467,6 +2467,12 @@ static void machvirt_init(MachineState
> >   > *machine)
> >   >
> >   >       create_gic(vms, sysmem);
> >   >
> >   > +    has_ged = has_ged && aarch64 && firmware_loaded &&
> >   > +              virt_is_acpi_enabled(vms);
> >   > +    if (has_ged) {
> >   > +        vms->acpi_dev = create_acpi_ged(vms);
> >   > +    }
> >   > +
> >   >       virt_cpu_post_init(vms, sysmem);
> >   >
> >   >       fdt_add_pmu_nodes(vms);
> >   > @@ -2489,9 +2495,7 @@ static void machvirt_init(MachineState
> >   *machine)
> >   >
> >   >       create_pcie(vms);
> >   >
> >   > -    if (has_ged && aarch64 && firmware_loaded &&
> >   virt_is_acpi_enabled(vms)) {
> >   > -        vms->acpi_dev = create_acpi_ged(vms);
> >   > -    } else {
> >   > +    if (!has_ged) {
> >   >           create_gpio_devices(vms, VIRT_GPIO, sysmem);
> >   >       }
> >   >
> >   
> >   It's likely the GPIO device can be created before those disabled CPU objects
> >   are destroyed. It means the whole chunk of code can be moved together, I
> >   think.
>
> I was not totally sure of this. Hence, kept the order of the rest like that. I 
> can
> definitely check again if we can do that to reduce the change.

@has_ged is the equivalent to '!vmc->no_ged' initially and then it's overrided 
by
the following changes in this patch. The syntax of @has_ged has been changed and
it's not the best name to match the changes. There are two solutions: (1) Rename
@has_ged to something meaningful and matching with the changes; (2) Move the 
whole
chunk of codes, which I preferred. The GPIO device and GED device are 
supplementing
to each other, meaning GPIO device will be created when GED device has been 
disallowed.

    has_ged = has_ged && aarch64 && firmware_loaded && 
virt_is_acpi_enabled(vms);

The code to be moved together in virt.c since they're correlated:

    if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
        vms->acpi_dev = create_acpi_ged(vms);
    } else {
        create_gpio_devices(vms, VIRT_GPIO, sysmem);
    }
    
    if (vms->secure && !vmc->no_secure_gpio) {
        create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
    }

     /* connect powerdown request */
     vms->powerdown_notifier.notify = virt_powerdown_req;
     qemu_register_powerdown_notifier(&vms->powerdown_notifier);

Thanks,
Gavin