| Message ID | 20240613233639.202896-14-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
On Fri Jun 14, 2024 at 9:36 AM AEST, Salil Mehta wrote: > ARM arch does not allow CPUs presence to be changed [1] after kernel has booted. > Hence, firmware/ACPI/Qemu must ensure persistent view of the vCPUs to the Guest > kernel even when they are not present in the QoM i.e. are unplugged or are > yet-to-be-plugged Do you need arch-independent state for this? If ARM always requires it then can it be implemented between arm and acpi interface? If not, then perhaps could it be done in the patch that introduces all the other state? > References: > [1] Check comment 5 in the bugzilla entry > Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4481#c5 If I understand correctly (and I don't know ACPI, so it's likely I don't), that is and update to ACPI spec to say some bit in ACPI table must remain set regardless of CPU hotplug state. Reference links are good, I think it would be nice to add a small summary in the changelog too. Thanks, Nick > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > cpu-common.c | 6 ++++++ > hw/arm/virt.c | 7 +++++++ > include/hw/core/cpu.h | 21 +++++++++++++++++++++ > 3 files changed, 34 insertions(+) > > diff --git a/cpu-common.c b/cpu-common.c > index 49d2a50835..e4b4dee99a 100644 > --- a/cpu-common.c > +++ b/cpu-common.c > @@ -128,6 +128,12 @@ bool qemu_enabled_cpu(CPUState *cpu) > return cpu && !cpu->disabled; > } > > +bool qemu_persistent_cpu(CPUState *cpu) > +{ > + /* cpu state can be faked to the guest via acpi */ > + return cpu && cpu->acpi_persistent; > +} > + > uint64_t qemu_get_cpu_archid(int cpu_index) > { > MachineState *ms = MACHINE(qdev_get_machine()); > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 5f98162587..9d33f30a6a 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -3016,6 +3016,13 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > return; > } > virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); > + > + /* > + * To give persistent presence view of vCPUs to the guest, ACPI might need > + * to fake the presence of the vCPUs to the guest but keep them disabled. > + * This shall be used during the init of ACPI Hotplug state and hot-unplug > + */ > + cs->acpi_persistent = true; > } > > static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 62e68611c0..e13e542177 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -540,6 +540,14 @@ struct CPUState { > * every CPUState is enabled across all architectures. > */ > bool disabled; > + /* > + * On certain architectures, to provide a persistent view of the 'presence' > + * of vCPUs to the guest, ACPI might need to fake the 'presence' of the > + * vCPUs but keep them ACPI-disabled for the guest. This is achieved by > + * returning `_STA.PRES=True` and `_STA.Ena=False` for the unplugged vCPUs > + * in QEMU QoM. > + */ > + bool acpi_persistent; > > /* TODO Move common fields from CPUArchState here. */ > int cpu_index; > @@ -959,6 +967,19 @@ bool qemu_present_cpu(CPUState *cpu); > */ > bool qemu_enabled_cpu(CPUState *cpu); > > +/** > + * qemu_persistent_cpu: > + * @cpu: The vCPU to check > + * > + * Checks if the vCPU state should always be reflected as *present* via ACPI > + * to the Guest. By default, this is False on all architectures and has to be > + * explicity set during initialization. > + * > + * Returns: True if it is ACPI 'persistent' CPU > + * > + */ > +bool qemu_persistent_cpu(CPUState *cpu); > + > /** > * qemu_get_cpu_archid: > * @cpu_index: possible vCPU for which arch-id needs to be retreived
HI Nick, Thanks for taking time to review. Please find my replies inline. > From: Nicholas Piggin <npiggin@gmail.com> > Sent: Thursday, July 4, 2024 3:49 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > qemu-arm@nongnu.org; mst@redhat.com > > On Fri Jun 14, 2024 at 9:36 AM AEST, Salil Mehta wrote: > > ARM arch does not allow CPUs presence to be changed [1] after kernel > has booted. > > Hence, firmware/ACPI/Qemu must ensure persistent view of the vCPUs to > > the Guest kernel even when they are not present in the QoM i.e. are > > unplugged or are yet-to-be-plugged > > Do you need arch-independent state for this? If ARM always requires it > then can it be implemented between arm and acpi interface? Yes, we do need as we cannot say if the same constraint applies to other architectures as well. Above stated constraint affects how the architecture common ACPI CPU code is initialized. > > If not, then perhaps could it be done in the patch that introduces all the > other state? > > > References: > > [1] Check comment 5 in the bugzilla entry > > Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4481#c5 > > If I understand correctly (and I don't know ACPI, so it's likely I don't), that is > and update to ACPI spec to say some bit in ACPI table must remain set > regardless of CPU hotplug state. ARM does not claims anything related to CPU hotplug right now. It simply does not exists. The ACPI update is simply reinforcing the existing fact that _STA.Present bit in the ACPI spec cannot be changed after system has booted. This is because for ARM arch there are many other initializations which depend upon the exact availability of CPU count during boot and they do not expect that to change after boot. For example, there are so many per-CPU features and the GIC CPU interface etc. which all expect this to be fixed at boot time. This is immutable requirement from ARM. > > Reference links are good, I think it would be nice to add a small summary in > the changelog too. sure. I will do. Thanks Salil. > > Thanks, > Nick > > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > cpu-common.c | 6 ++++++ > > hw/arm/virt.c | 7 +++++++ > > include/hw/core/cpu.h | 21 +++++++++++++++++++++ > > 3 files changed, 34 insertions(+) > > > > diff --git a/cpu-common.c b/cpu-common.c index > 49d2a50835..e4b4dee99a > > 100644 > > --- a/cpu-common.c > > +++ b/cpu-common.c > > @@ -128,6 +128,12 @@ bool qemu_enabled_cpu(CPUState *cpu) > > return cpu && !cpu->disabled; > > } > > > > +bool qemu_persistent_cpu(CPUState *cpu) { > > + /* cpu state can be faked to the guest via acpi */ > > + return cpu && cpu->acpi_persistent; } > > + > > uint64_t qemu_get_cpu_archid(int cpu_index) { > > MachineState *ms = MACHINE(qdev_get_machine()); diff --git > > a/hw/arm/virt.c b/hw/arm/virt.c index 5f98162587..9d33f30a6a 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -3016,6 +3016,13 @@ static void virt_cpu_pre_plug(HotplugHandler > *hotplug_dev, DeviceState *dev, > > return; > > } > > virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); > > + > > + /* > > + * To give persistent presence view of vCPUs to the guest, ACPI might > need > > + * to fake the presence of the vCPUs to the guest but keep them > disabled. > > + * This shall be used during the init of ACPI Hotplug state and hot- > unplug > > + */ > > + cs->acpi_persistent = true; > > } > > > > static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState > > *dev, diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index > > 62e68611c0..e13e542177 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -540,6 +540,14 @@ struct CPUState { > > * every CPUState is enabled across all architectures. > > */ > > bool disabled; > > + /* > > + * On certain architectures, to provide a persistent view of the > 'presence' > > + * of vCPUs to the guest, ACPI might need to fake the 'presence' of the > > + * vCPUs but keep them ACPI-disabled for the guest. This is achieved > by > > + * returning `_STA.PRES=True` and `_STA.Ena=False` for the unplugged > vCPUs > > + * in QEMU QoM. > > + */ > > + bool acpi_persistent; > > > > /* TODO Move common fields from CPUArchState here. */ > > int cpu_index; > > @@ -959,6 +967,19 @@ bool qemu_present_cpu(CPUState *cpu); > > */ > > bool qemu_enabled_cpu(CPUState *cpu); > > > > +/** > > + * qemu_persistent_cpu: > > + * @cpu: The vCPU to check > > + * > > + * Checks if the vCPU state should always be reflected as *present* > > +via ACPI > > + * to the Guest. By default, this is False on all architectures and > > +has to be > > + * explicity set during initialization. > > + * > > + * Returns: True if it is ACPI 'persistent' CPU > > + * > > + */ > > +bool qemu_persistent_cpu(CPUState *cpu); > > + > > /** > > * qemu_get_cpu_archid: > > * @cpu_index: possible vCPU for which arch-id needs to be retreived
On Thu Jul 4, 2024 at 9:23 PM AEST, Salil Mehta wrote: > HI Nick, > > Thanks for taking time to review. Please find my replies inline. > > > From: Nicholas Piggin <npiggin@gmail.com> > > Sent: Thursday, July 4, 2024 3:49 AM > > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > > qemu-arm@nongnu.org; mst@redhat.com > > > > On Fri Jun 14, 2024 at 9:36 AM AEST, Salil Mehta wrote: > > > ARM arch does not allow CPUs presence to be changed [1] after kernel > > has booted. > > > Hence, firmware/ACPI/Qemu must ensure persistent view of the vCPUs to > > > the Guest kernel even when they are not present in the QoM i.e. are > > > unplugged or are yet-to-be-plugged > > > > Do you need arch-independent state for this? If ARM always requires it > > then can it be implemented between arm and acpi interface? > > > Yes, we do need as we cannot say if the same constraint applies to other > architectures as well. Above stated constraint affects how the architecture > common ACPI CPU code is initialized. Right, but could it be done with an ACPI property that the arch can change, or an argument from arch code to an ACPI init routine? Or even a machine property that ACPI could query. > > If not, then perhaps could it be done in the patch that introduces all the > > other state? > > > > > References: > > > [1] Check comment 5 in the bugzilla entry > > > Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4481#c5 > > > > If I understand correctly (and I don't know ACPI, so it's likely I don't), that is > > and update to ACPI spec to say some bit in ACPI table must remain set > > regardless of CPU hotplug state. > > > ARM does not claims anything related to CPU hotplug right now. It simply > does not exists. The ACPI update is simply reinforcing the existing fact that > _STA.Present bit in the ACPI spec cannot be changed after system has booted. > > This is because for ARM arch there are many other initializations which depend > upon the exact availability of CPU count during boot and they do not expect > that to change after boot. For example, there are so many per-CPU features > and the GIC CPU interface etc. which all expect this to be fixed at boot time. > This is immutable requirement from ARM. > > > > > > Reference links are good, I think it would be nice to add a small summary in > > the changelog too. > > sure. I will do. Thanks. Something like what you wrote above would work. Thanks, Nick
diff --git a/cpu-common.c b/cpu-common.c index 49d2a50835..e4b4dee99a 100644 --- a/cpu-common.c +++ b/cpu-common.c @@ -128,6 +128,12 @@ bool qemu_enabled_cpu(CPUState *cpu) return cpu && !cpu->disabled; } +bool qemu_persistent_cpu(CPUState *cpu) +{ + /* cpu state can be faked to the guest via acpi */ + return cpu && cpu->acpi_persistent; +} + uint64_t qemu_get_cpu_archid(int cpu_index) { MachineState *ms = MACHINE(qdev_get_machine()); diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 5f98162587..9d33f30a6a 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -3016,6 +3016,13 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, return; } virt_cpu_set_properties(OBJECT(cs), cpu_slot, errp); + + /* + * To give persistent presence view of vCPUs to the guest, ACPI might need + * to fake the presence of the vCPUs to the guest but keep them disabled. + * This shall be used during the init of ACPI Hotplug state and hot-unplug + */ + cs->acpi_persistent = true; } static void virt_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 62e68611c0..e13e542177 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -540,6 +540,14 @@ struct CPUState { * every CPUState is enabled across all architectures. */ bool disabled; + /* + * On certain architectures, to provide a persistent view of the 'presence' + * of vCPUs to the guest, ACPI might need to fake the 'presence' of the + * vCPUs but keep them ACPI-disabled for the guest. This is achieved by + * returning `_STA.PRES=True` and `_STA.Ena=False` for the unplugged vCPUs + * in QEMU QoM. + */ + bool acpi_persistent; /* TODO Move common fields from CPUArchState here. */ int cpu_index; @@ -959,6 +967,19 @@ bool qemu_present_cpu(CPUState *cpu); */ bool qemu_enabled_cpu(CPUState *cpu); +/** + * qemu_persistent_cpu: + * @cpu: The vCPU to check + * + * Checks if the vCPU state should always be reflected as *present* via ACPI + * to the Guest. By default, this is False on all architectures and has to be + * explicity set during initialization. + * + * Returns: True if it is ACPI 'persistent' CPU + * + */ +bool qemu_persistent_cpu(CPUState *cpu); + /** * qemu_get_cpu_archid: * @cpu_index: possible vCPU for which arch-id needs to be retreived
ARM arch does not allow CPUs presence to be changed [1] after kernel has booted. Hence, firmware/ACPI/Qemu must ensure persistent view of the vCPUs to the Guest kernel even when they are not present in the QoM i.e. are unplugged or are yet-to-be-plugged References: [1] Check comment 5 in the bugzilla entry Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4481#c5 Signed-off-by: Salil Mehta <salil.mehta@huawei.com> --- cpu-common.c | 6 ++++++ hw/arm/virt.c | 7 +++++++ include/hw/core/cpu.h | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+)