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

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Gavin,

>  From: Gavin Shan <gshan@redhat.com>
>  Sent: Tuesday, August 20, 2024 1:22 AM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  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);


If there is no dependency then we can completely move before   virt_cpu_post_init().
I'll get back to you on this.

Thanks
Salil.

>  
>  Thanks,
>  Gavin
>  
>  
>  
>