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