| Message ID | 20230926100436.28284-16-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
Hi Salil, On 9/26/23 20:04, 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 5c8a0672dc..cbb6199ec6 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2376,6 +2376,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); > + } > + I prefer the old style. Squeezing all conditions to @has_ged changes what's to be meant by @has_ged itself. if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) { : } > virt_cpu_post_init(vms, sysmem); > > fdt_add_pmu_nodes(vms); > @@ -2398,9 +2404,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); > } > Thanks, Gavin
> From: Gavin Shan <gshan@redhat.com> > Sent: Thursday, September 28, 2023 2:08 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu- > arm@nongnu.org > Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > peter.maydell@linaro.org; richard.henderson@linaro.org; > imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com; > philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; > rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org; > linux@armlinux.org.uk; darren@os.amperecomputing.com; > ilkka@os.amperecomputing.com; vishnu@os.amperecomputing.com; > karl.heubaum@oracle.com; miguel.luis@oracle.com; salil.mehta@opnsrc.net; > zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng (C) > <wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>; > jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn > Subject: Re: [PATCH RFC V2 15/37] arm/virt: Create GED dev before > *disabled* CPU Objs are destroyed > > Hi Salil, > > On 9/26/23 20:04, 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 5c8a0672dc..cbb6199ec6 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -2376,6 +2376,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); > > + } > > + > > I prefer the old style. Squeezing all conditions to @has_ged changes what's > to be meant by @has_ged itself. The check is too long and a similar piece of code has to used again down below. Hence, to reuse the result, it's better to store the first result in a variable. I can see you point though. To mitigate that I can use a new variable for this? Thanks Salil. > > if (has_ged && aarch64 && firmware_loaded && > virt_is_acpi_enabled(vms)) { > : > } > > > virt_cpu_post_init(vms, sysmem); > > > > fdt_add_pmu_nodes(vms); > > @@ -2398,9 +2404,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); > > } > > > > Thanks, > Gavin
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 5c8a0672dc..cbb6199ec6 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2376,6 +2376,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); @@ -2398,9 +2404,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(-)