| Message ID | 20240613233639.202896-12-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
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. Thanks, Gavin
> 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 >
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
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 > > > >
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); }
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(-)