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

[Salil Mehta]

>  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.

Thanks
Salil.



>  
>  Thanks,
>  Gavin
>