| Message ID | 20230202045223.2594627-3-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add basic ACPI support for risc-v virt | expand |
On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> wrote: > > ACPI is optional. So, add a switch to toggle. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > hw/riscv/virt.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/hw/riscv/virt.h | 2 ++ > 2 files changed, 40 insertions(+) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 7ad9fda20c..84962962ff 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -50,6 +50,7 @@ > #include "hw/pci-host/gpex.h" > #include "hw/display/ramfb.h" > #include "hw/acpi/aml-build.h" > +#include "qapi/qapi-visit-common.h" > > /* > * The virt machine physical address space used by some of the devices > @@ -1525,6 +1526,10 @@ static void virt_machine_init(MachineState *machine) > > static void virt_machine_instance_init(Object *obj) > { > + MachineState *ms = MACHINE(obj); Drop this > + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); > + > + s->acpi = ON_OFF_AUTO_OFF; Is this needed? I believe the purpose of an auto/on/off property is to have an "auto" value, which is ON_OFF_AUTO_AUTO. > } > > static char *virt_get_aia_guests(Object *obj, Error **errp) > @@ -1601,6 +1606,34 @@ static void virt_set_aclint(Object *obj, bool value, Error **errp) > s->have_aclint = value; > } > > +bool virt_is_acpi_enabled(RISCVVirtState *s) > +{ > + if (s->acpi == ON_OFF_AUTO_OFF) { > + return false; > + } > + return true; > +} > + > +static void virt_get_acpi(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); ditto > + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); > + > + OnOffAuto acpi = s->acpi; > + > + visit_type_OnOffAuto(v, name, &acpi, errp); > +} > + > +static void virt_set_acpi(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); ditto > + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); > + > + visit_type_OnOffAuto(v, name, &s->acpi, errp); > +} > + > static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine, > DeviceState *dev) > { > @@ -1672,6 +1705,11 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) > sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value " > "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS); > object_class_property_set_description(oc, "aia-guests", str); > + object_class_property_add(oc, "acpi", "OnOffAuto", > + virt_get_acpi, virt_set_acpi, > + NULL, NULL); I am not sure about "OnOffAuto" vs. "bool" type. It seems the only difference is that with "OnOffAuto" type we may silently change the interpretation of "auto" value across different QEMU versions? > + object_class_property_set_description(oc, "acpi", > + "Enable ACPI"); > } > > static const TypeInfo virt_machine_typeinfo = { > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h > index 6c7885bf89..62efebaa32 100644 > --- a/include/hw/riscv/virt.h > +++ b/include/hw/riscv/virt.h > @@ -58,6 +58,7 @@ struct RISCVVirtState { > int aia_guests; > char *oem_id; > char *oem_table_id; > + OnOffAuto acpi; > }; > > enum { > @@ -123,4 +124,5 @@ enum { > #define FDT_APLIC_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \ > 1 + FDT_APLIC_INT_CELLS) > > +bool virt_is_acpi_enabled(RISCVVirtState *s); > #endif > -- Regards, Bin
On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > + object_class_property_add(oc, "acpi", "OnOffAuto", > + virt_get_acpi, virt_set_acpi, > + NULL, NULL); > + object_class_property_set_description(oc, "acpi", > + "Enable ACPI"); The way this works on other architectures (x86_64, aarch64) is that you get ACPI by default and can use -no-acpi to disable it if desired. Can we have the same on RISC-V, for consistency?
On 6/2/23 11:54, Andrea Bolognani wrote: > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: >> + object_class_property_add(oc, "acpi", "OnOffAuto", >> + virt_get_acpi, virt_set_acpi, >> + NULL, NULL); >> + object_class_property_set_description(oc, "acpi", >> + "Enable ACPI"); > > The way this works on other architectures (x86_64, aarch64) is that > you get ACPI by default and can use -no-acpi to disable it if > desired. Can we have the same on RISC-V, for consistency? -no-acpi rather seems a x86-specific hack for the ISA PC machine, and has a high maintenance cost / burden. If hardware provides ACPI support, QEMU should expose it to the guest. Actually, what is the value added by '-no-acpi'?
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/23 11:54, Andrea Bolognani wrote: > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > + virt_get_acpi, virt_set_acpi, > > > + NULL, NULL); > > > + object_class_property_set_description(oc, "acpi", > > > + "Enable ACPI"); > > > > The way this works on other architectures (x86_64, aarch64) is that > > you get ACPI by default and can use -no-acpi to disable it if > > desired. Can we have the same on RISC-V, for consistency? Default on, with a user control to turn off, can be done with a boolean. I'm not sure why/if Auto is needed for acpi. Auto is useful when a configuration doesn't support a default setting for a feature. If the user hasn't explicitly requested the feature to be on or off, then the configuration can silently select what works. If, however, the user explicitly chooses what doesn't work, then qemu will fail with an error instead. > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > has a high maintenance cost / burden. > > If hardware provides ACPI support, QEMU should expose it to the guest. > > Actually, what is the value added by '-no-acpi'? IIRC, when booting, at least arm guests, with edk2 and ACPI tables, then edk2 will provide the guest ACPI tables instead of DT. To ensure we can boot with edk2, but still allow the guest to boot with DT, we need a way to disable the generation of ACPI tables. Thanks, drew
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/23 11:54, Andrea Bolognani wrote: > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > + virt_get_acpi, virt_set_acpi, > > > + NULL, NULL); > > > + object_class_property_set_description(oc, "acpi", > > > + "Enable ACPI"); > > > > The way this works on other architectures (x86_64, aarch64) is that > > you get ACPI by default and can use -no-acpi to disable it if > > desired. Can we have the same on RISC-V, for consistency? > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > has a high maintenance cost / burden. Under the hood it is actually a OnOffAuto machine property and -no-acpi is just a shortcut to set that. > Actually, what is the value added by '-no-acpi'? On arm(64) the linux kernel can use either device trees or ACPI to find the hardware. Historical reasons mostly, when the platform started there was no ACPI support. Also the edk2 firmware uses Device Trees for hardware discovery, likewise for historical reasons. When ACPI is available for a platform right from the start I see little reason to offer an option to turn it off though ... take care, Gerd
On Mon, Feb 06, 2023 at 01:35:20PM +0100, Andrew Jones wrote: > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > + virt_get_acpi, virt_set_acpi, > > > > + NULL, NULL); > > > > + object_class_property_set_description(oc, "acpi", > > > > + "Enable ACPI"); > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > you get ACPI by default and can use -no-acpi to disable it if > > > desired. Can we have the same on RISC-V, for consistency? > > Default on, with a user control to turn off, can be done with a boolean. > I'm not sure why/if Auto is needed for acpi. Auto is useful when a > configuration doesn't support a default setting for a feature. If the > user hasn't explicitly requested the feature to be on or off, then the > configuration can silently select what works. If, however, the user > explicitly chooses what doesn't work, then qemu will fail with an error > instead. > Since all other architectures use Auto instead of a simple bool, I opted for the same to keep it consistent. However, default AUTO looked ambiguous to me. Since we still need to support external interrupt controllers (IMSIC/APLIC/PLIC), I chose to keep it OFF by default for now. Thanks Sunil > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > has a high maintenance cost / burden. > > > > If hardware provides ACPI support, QEMU should expose it to the guest. > > > > Actually, what is the value added by '-no-acpi'? > > IIRC, when booting, at least arm guests, with edk2 and ACPI tables, > then edk2 will provide the guest ACPI tables instead of DT. To ensure > we can boot with edk2, but still allow the guest to boot with DT, we > need a way to disable the generation of ACPI tables. > > Thanks, > drew
Am 6. Februar 2023 11:18:06 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 6/2/23 11:54, Andrea Bolognani wrote: >> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: >>> + object_class_property_add(oc, "acpi", "OnOffAuto", >>> + virt_get_acpi, virt_set_acpi, >>> + NULL, NULL); >>> + object_class_property_set_description(oc, "acpi", >>> + "Enable ACPI"); >> >> The way this works on other architectures (x86_64, aarch64) is that >> you get ACPI by default and can use -no-acpi to disable it if >> desired. Can we have the same on RISC-V, for consistency? > >-no-acpi rather seems a x86-specific hack for the ISA PC machine, ... for the i440FX/PIIX machine, to be precise. There it controls the presence of the PIIX ACPI controller and surely also the generation of ACPI tables. ACPI wasn't a thing in pure ISA times, so the switch doesn't make much sense for the ISA machine. Here, for RISC-V, the "acpi" switch seems to just control the generation of ACPI tables which has quite different semantics than -no-acpi. >and >has a high maintenance cost / burden. > >If hardware provides ACPI support, QEMU should expose it to the guest. Yes, I fully agree with both points. > >Actually, what is the value added by '-no-acpi'? IIUC, it allows the PC machine to emulate a PCI PC without an ACPI bios. Unfortunately, it also omits the instantiation of the... erm... Frankenstein PIIX4_ACPI device which seems quite unnecessary to achieve that goal. Just always instantiating it seems much simpler.
On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > + virt_get_acpi, virt_set_acpi, > > > > + NULL, NULL); > > > > + object_class_property_set_description(oc, "acpi", > > > > + "Enable ACPI"); > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > you get ACPI by default and can use -no-acpi to disable it if > > > desired. Can we have the same on RISC-V, for consistency? > > Default on, with a user control to turn off, can be done with a boolean. > I'm not sure why/if Auto is needed for acpi. Auto is useful when a > configuration doesn't support a default setting for a feature. If the > user hasn't explicitly requested the feature to be on or off, then the > configuration can silently select what works. If, however, the user > explicitly chooses what doesn't work, then qemu will fail with an error > instead. I have a confusion about "OnOffAuto" vs. "bool" type. Both "OnOffAuto" vs. "bool" type property can have a default value if user does not assign a value to it from command line. The default value is: - ON_OFF_AUTO_AUTO for "OnOffAuto" - false for "bool" But the default value can be overridden in the machine's init function, like in this patch. So I am not really sure how these 2 types of properties are different. Why did we introduce a "OnOffAuto" type, and how is that type supposed to be used in which scenario? > > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > has a high maintenance cost / burden. > > > > If hardware provides ACPI support, QEMU should expose it to the guest. > > > > Actually, what is the value added by '-no-acpi'? > > IIRC, when booting, at least arm guests, with edk2 and ACPI tables, > then edk2 will provide the guest ACPI tables instead of DT. To ensure > we can boot with edk2, but still allow the guest to boot with DT, we > need a way to disable the generation of ACPI tables. > Regards, Bin
On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote: > On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > > + virt_get_acpi, virt_set_acpi, > > > > > + NULL, NULL); > > > > > + object_class_property_set_description(oc, "acpi", > > > > > + "Enable ACPI"); > > > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > > you get ACPI by default and can use -no-acpi to disable it if > > > > desired. Can we have the same on RISC-V, for consistency? > > > > Default on, with a user control to turn off, can be done with a boolean. > > I'm not sure why/if Auto is needed for acpi. Auto is useful when a > > configuration doesn't support a default setting for a feature. If the > > user hasn't explicitly requested the feature to be on or off, then the > > configuration can silently select what works. If, however, the user > > explicitly chooses what doesn't work, then qemu will fail with an error > > instead. > > I have a confusion about "OnOffAuto" vs. "bool" type. > > Both "OnOffAuto" vs. "bool" type property can have a default value if > user does not assign a value to it from command line. The default > value is: > > - ON_OFF_AUTO_AUTO for "OnOffAuto" > - false for "bool" > > But the default value can be overridden in the machine's init > function, like in this patch. > > So I am not really sure how these 2 types of properties are different. > Why did we introduce a "OnOffAuto" type, and how is that type supposed > to be used in which scenario? Auto makes sense for options which have dependencies on other options. For example, if we have two options, A and B, where A is an Auto and B is a boolean, then, when A is initialized to AUTO and has a dependency on B being X, then have the following B=X A=AUTO -> T (works) B=!X A=AUTO -> F (works) (This is the same whether B was set to X by default or explicitly by the user.) Now, if the user explicitly sets A, we have B=X A=T (works) B=X A=F (works) B=!X A=T (emit error about B!=X with A=T and fail) B=!X A=F (works) We can't have that behavior with A just being a boolean, defaulting to true, because we don't know if it's true because of the default or because it was explicitly set B=X A=T (works, by default or by user) B=X A=F (works) B=!X A=T (doesn't work, but if the user didn't select A=T, then we could have silently flipped it to F and continued) B=!X A=F (works) IOW, Auto just adds one more bit of info, allowing default vs. user selection to be determined, which can then be used for different behaviors. Back to the "acpi" property, I'm not sure what it depends on that requires it to be an Auto vs. a boolean. Thanks, drew
On 6/2/23 13:56, Gerd Hoffmann wrote: > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: >> On 6/2/23 11:54, Andrea Bolognani wrote: >>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: >>>> + object_class_property_add(oc, "acpi", "OnOffAuto", >>>> + virt_get_acpi, virt_set_acpi, >>>> + NULL, NULL); >>>> + object_class_property_set_description(oc, "acpi", >>>> + "Enable ACPI"); >>> >>> The way this works on other architectures (x86_64, aarch64) is that >>> you get ACPI by default and can use -no-acpi to disable it if >>> desired. Can we have the same on RISC-V, for consistency? >> >> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and >> has a high maintenance cost / burden. > > Under the hood it is actually a OnOffAuto machine property and -no-acpi > is just a shortcut to set that. > >> Actually, what is the value added by '-no-acpi'? > > On arm(64) the linux kernel can use either device trees or ACPI to find > the hardware. Historical reasons mostly, when the platform started > there was no ACPI support. Also the edk2 firmware uses Device Trees > for hardware discovery, likewise for historical reasons. > > When ACPI is available for a platform right from the start I see little > reason to offer an option to turn it off though ... Yeah I concur. There is no point in disabling ACPI on the RISCV virt machine IMO.
On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/23 13:56, Gerd Hoffmann wrote: > > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > > + virt_get_acpi, virt_set_acpi, > > > > > + NULL, NULL); > > > > > + object_class_property_set_description(oc, "acpi", > > > > > + "Enable ACPI"); > > > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > > you get ACPI by default and can use -no-acpi to disable it if > > > > desired. Can we have the same on RISC-V, for consistency? > > > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > > has a high maintenance cost / burden. > > > > Under the hood it is actually a OnOffAuto machine property and -no-acpi > > is just a shortcut to set that. > > > > > Actually, what is the value added by '-no-acpi'? > > > > On arm(64) the linux kernel can use either device trees or ACPI to find > > the hardware. Historical reasons mostly, when the platform started > > there was no ACPI support. Also the edk2 firmware uses Device Trees > > for hardware discovery, likewise for historical reasons. > > > > When ACPI is available for a platform right from the start I see little > > reason to offer an option to turn it off though ... > > Yeah I concur. There is no point in disabling ACPI on the RISCV virt > machine IMO. Thank you all for the inputs. I am fine to keep it enabled by default. Do you mean we don't need the switch at all even for some testing/debugging purpose? Thanks, Sunil
On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/23 13:56, Gerd Hoffmann wrote: > > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > > + virt_get_acpi, virt_set_acpi, > > > > > + NULL, NULL); > > > > > + object_class_property_set_description(oc, "acpi", > > > > > + "Enable ACPI"); > > > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > > you get ACPI by default and can use -no-acpi to disable it if > > > > desired. Can we have the same on RISC-V, for consistency? > > > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > > has a high maintenance cost / burden. > > > > Under the hood it is actually a OnOffAuto machine property and -no-acpi > > is just a shortcut to set that. > > > > > Actually, what is the value added by '-no-acpi'? > > > > On arm(64) the linux kernel can use either device trees or ACPI to find > > the hardware. Historical reasons mostly, when the platform started > > there was no ACPI support. Also the edk2 firmware uses Device Trees > > for hardware discovery, likewise for historical reasons. > > > > When ACPI is available for a platform right from the start I see little > > reason to offer an option to turn it off though ... > > Yeah I concur. There is no point in disabling ACPI on the RISCV virt > machine IMO. edk2 will only present DT or ACPI to the guest, not both. However, RISCV Linux supports both. If we don't offer '-no-acpi' as a way to switch to DT, then edk2+DT users will need to configure the varstore to select it. And, since testing needs to be done with both, that varstore change will need to be added to all the testcases which need DT (or a varstore with DT already selected will need to be maintained and used by the testsuites) IMO, the generation of the ACPI tables should be 'on' by default, but then the, already present, '-no-acpi' command line option should be made available in order to easily inform edk2 to present DT instead of ACPI. Thanks, drew
On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/23 11:54, Andrea Bolognani wrote: > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > + virt_get_acpi, virt_set_acpi, > > > + NULL, NULL); > > > + object_class_property_set_description(oc, "acpi", > > > + "Enable ACPI"); > > > > The way this works on other architectures (x86_64, aarch64) is that > > you get ACPI by default and can use -no-acpi to disable it if > > desired. Can we have the same on RISC-V, for consistency? > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > has a high maintenance cost / burden. Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically. If -no-acpi is problematic for some reason, then something like '-machine virt,acpi=off' would be sufficient for switching to DT too. Thanks, drew
On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote: > On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > > + virt_get_acpi, virt_set_acpi, > > > > > + NULL, NULL); > > > > > + object_class_property_set_description(oc, "acpi", > > > > > + "Enable ACPI"); > > > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > > you get ACPI by default and can use -no-acpi to disable it if > > > > desired. Can we have the same on RISC-V, for consistency? > > > > Default on, with a user control to turn off, can be done with a boolean. > > I'm not sure why/if Auto is needed for acpi. Auto is useful when a > > configuration doesn't support a default setting for a feature. If the > > user hasn't explicitly requested the feature to be on or off, then the > > configuration can silently select what works. If, however, the user > > explicitly chooses what doesn't work, then qemu will fail with an error > > instead. > > I have a confusion about "OnOffAuto" vs. "bool" type. > > Both "OnOffAuto" vs. "bool" type property can have a default value if > user does not assign a value to it from command line. The default > value is: > > - ON_OFF_AUTO_AUTO for "OnOffAuto" > - false for "bool" > > But the default value can be overridden in the machine's init > function, like in this patch. > > So I am not really sure how these 2 types of properties are different. > Why did we introduce a "OnOffAuto" type, and how is that type supposed > to be used in which scenario? > I don't know either. Since it is the same property across architectures, I used the OnOffAuto instead of a bool. May be Gerd and other qemu experts can help us to understand better. https://github.com/qemu/qemu/commit/17e89077b7e3bc1d96540e21ddc7451c3e077049 Thanks, Sunil
On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote: > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > + object_class_property_add(oc, "acpi", "OnOffAuto", > > > > + virt_get_acpi, virt_set_acpi, > > > > + NULL, NULL); > > > > + object_class_property_set_description(oc, "acpi", > > > > + "Enable ACPI"); > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > you get ACPI by default and can use -no-acpi to disable it if > > > desired. Can we have the same on RISC-V, for consistency? > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > has a high maintenance cost / burden. > > Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically. > If -no-acpi is problematic for some reason, then something like > '-machine virt,acpi=off' would be sufficient for switching to DT too. I would greatly prefer it if the command line interface could be kept consistent across architectures. It looks like i440fx and q35 both have an 'acpi' machine property. Is -no-acpi just sugar for acpi=off? Is it considered desirable to get rid of -no-acpi? If so, we should follow the usual deprecation process and get rid of it after libvirt has had a chance to adapt. In the scenario described above, it would make sense for RISC-V to only offer the machine type option (assuming that -no-acpi doesn't come for free with that) instead of putting additional effort into implementing an interface that is already on its way out.
On 07/02/2023 14.56, Andrea Bolognani wrote: > On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote: >> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: >>> On 6/2/23 11:54, Andrea Bolognani wrote: >>>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: >>>>> + object_class_property_add(oc, "acpi", "OnOffAuto", >>>>> + virt_get_acpi, virt_set_acpi, >>>>> + NULL, NULL); >>>>> + object_class_property_set_description(oc, "acpi", >>>>> + "Enable ACPI"); >>>> >>>> The way this works on other architectures (x86_64, aarch64) is that >>>> you get ACPI by default and can use -no-acpi to disable it if >>>> desired. Can we have the same on RISC-V, for consistency? >>> >>> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and >>> has a high maintenance cost / burden. >> >> Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically. >> If -no-acpi is problematic for some reason, then something like >> '-machine virt,acpi=off' would be sufficient for switching to DT too. > > I would greatly prefer it if the command line interface could be kept > consistent across architectures. > > It looks like i440fx and q35 both have an 'acpi' machine property. Is > -no-acpi just sugar for acpi=off? Yes, it is, see softmmu/vl.c: case QEMU_OPTION_no_acpi: qdict_put_str(machine_opts_dict, "acpi", "off"); break; > Is it considered desirable to get rid of -no-acpi? Sounds like a good idea, indeed! > If so, we should follow the usual deprecation > process and get rid of it after libvirt has had a chance to adapt. > > In the scenario described above, it would make sense for RISC-V to > only offer the machine type option (assuming that -no-acpi doesn't > come for free with that) instead of putting additional effort into > implementing an interface that is already on its way out. I agree. IMHO the machine parameter should be enough, no need to introduce "-no-acpi" here. Thomas
On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote: > On 07/02/2023 14.56, Andrea Bolognani wrote: > > It looks like i440fx and q35 both have an 'acpi' machine property. Is > > -no-acpi just sugar for acpi=off? > > Yes, it is, see softmmu/vl.c: > > case QEMU_OPTION_no_acpi: > qdict_put_str(machine_opts_dict, "acpi", "off"); > break; > > > Is it considered desirable to get rid of -no-acpi? > > Sounds like a good idea, indeed! > > > If so, we should follow the usual deprecation > > process and get rid of it after libvirt has had a chance to adapt. > > > > In the scenario described above, it would make sense for RISC-V to > > only offer the machine type option (assuming that -no-acpi doesn't > > come for free with that) instead of putting additional effort into > > implementing an interface that is already on its way out. > > I agree. IMHO the machine parameter should be enough, no need to introduce > "-no-acpi" here. Well, it looks like -no-acpi will come for free after all, thanks to the code you pasted above, as long as the machine property follows the convention established by x86, arm and (I just noticed this one) loongarch. So I guess the -no-acpi deprecation can be handled separately, and the only thing that needs changing in the current patch is making sure that ACPI is opt-out rather than opt-in, as is the case for other architectures :)
On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote: > On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote: > > On 07/02/2023 14.56, Andrea Bolognani wrote: > > > It looks like i440fx and q35 both have an 'acpi' machine property. Is > > > -no-acpi just sugar for acpi=off? > > > > Yes, it is, see softmmu/vl.c: > > > > case QEMU_OPTION_no_acpi: > > qdict_put_str(machine_opts_dict, "acpi", "off"); > > break; > > > > > Is it considered desirable to get rid of -no-acpi? > > > > Sounds like a good idea, indeed! > > > > > If so, we should follow the usual deprecation > > > process and get rid of it after libvirt has had a chance to adapt. > > > > > > In the scenario described above, it would make sense for RISC-V to > > > only offer the machine type option (assuming that -no-acpi doesn't > > > come for free with that) instead of putting additional effort into > > > implementing an interface that is already on its way out. > > > > I agree. IMHO the machine parameter should be enough, no need to introduce > > "-no-acpi" here. > > Well, it looks like -no-acpi will come for free after all, thanks to > the code you pasted above, as long as the machine property follows > the convention established by x86, arm and (I just noticed this one) > loongarch. Not quite, because we also have $ grep -A1 '"no-acpi"' qemu-options.hx DEF("no-acpi", 0, QEMU_OPTION_no_acpi, "-no-acpi disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM) So that means neither riscv nor loongarch get -no-acpi just by adding the "acpi" machine property. If the plan is to deprecate -no-acpi, then riscv can be like loongarch and only have the "acpi" property from the start. Thanks, drew
On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote: > On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote: > > Well, it looks like -no-acpi will come for free after all, thanks to > > the code you pasted above, as long as the machine property follows > > the convention established by x86, arm and (I just noticed this one) > > loongarch. > > Not quite, because we also have > > $ grep -A1 '"no-acpi"' qemu-options.hx > DEF("no-acpi", 0, QEMU_OPTION_no_acpi, > "-no-acpi disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM) > > So that means neither riscv nor loongarch get -no-acpi just by adding > the "acpi" machine property. > > If the plan is to deprecate -no-acpi, then riscv can be like loongarch > and only have the "acpi" property from the start. Makes sense. For the libvirt integration, do I understand correctly that the machine property was added by commit commit 17e89077b7e3bc1d96540e21ddc7451c3e077049 Author: Gerd Hoffmann <kraxel@redhat.com> Date: Fri Mar 20 11:01:36 2020 +0100 acpi: add acpi=OnOffAuto machine property to x86 and arm virt Remove the global acpi_enabled bool and replace it with an acpi OnOffAuto machine property. qemu throws an error now if you use -no-acpi while the machine type you are using doesn't support acpi in the first place. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Message-Id: <20200320100136.11717-1-kraxel@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Acked-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so we'll have to make the use of the machine property conditional.
On 08/02/2023 17.48, Andrea Bolognani wrote: > On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote: >> On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote: >>> Well, it looks like -no-acpi will come for free after all, thanks to >>> the code you pasted above, as long as the machine property follows >>> the convention established by x86, arm and (I just noticed this one) >>> loongarch. >> >> Not quite, because we also have >> >> $ grep -A1 '"no-acpi"' qemu-options.hx >> DEF("no-acpi", 0, QEMU_OPTION_no_acpi, >> "-no-acpi disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM) >> >> So that means neither riscv nor loongarch get -no-acpi just by adding >> the "acpi" machine property. >> >> If the plan is to deprecate -no-acpi, then riscv can be like loongarch >> and only have the "acpi" property from the start. > > Makes sense. > > > For the libvirt integration, do I understand correctly that the > machine property was added by commit > > commit 17e89077b7e3bc1d96540e21ddc7451c3e077049 > Author: Gerd Hoffmann <kraxel@redhat.com> > Date: Fri Mar 20 11:01:36 2020 +0100 > > acpi: add acpi=OnOffAuto machine property to x86 and arm virt > > Remove the global acpi_enabled bool and replace it with an > acpi OnOffAuto machine property. > > qemu throws an error now if you use -no-acpi while the machine > type you are using doesn't support acpi in the first place. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Message-Id: <20200320100136.11717-1-kraxel@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so > we'll have to make the use of the machine property conditional. Sounds right, and testing for the machine option should be possible with the upcoming QEMU 8.0. FWIW, I assume this is similar to the -no-hpet option which has been turned into a machine property, too: https://gitlab.com/libvirt/libvirt/-/commit/3c508e7d4310aeb8 Thomas
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 7ad9fda20c..84962962ff 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -50,6 +50,7 @@ #include "hw/pci-host/gpex.h" #include "hw/display/ramfb.h" #include "hw/acpi/aml-build.h" +#include "qapi/qapi-visit-common.h" /* * The virt machine physical address space used by some of the devices @@ -1525,6 +1526,10 @@ static void virt_machine_init(MachineState *machine) static void virt_machine_instance_init(Object *obj) { + MachineState *ms = MACHINE(obj); + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); + + s->acpi = ON_OFF_AUTO_OFF; } static char *virt_get_aia_guests(Object *obj, Error **errp) @@ -1601,6 +1606,34 @@ static void virt_set_aclint(Object *obj, bool value, Error **errp) s->have_aclint = value; } +bool virt_is_acpi_enabled(RISCVVirtState *s) +{ + if (s->acpi == ON_OFF_AUTO_OFF) { + return false; + } + return true; +} + +static void virt_get_acpi(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + MachineState *ms = MACHINE(obj); + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); + + OnOffAuto acpi = s->acpi; + + visit_type_OnOffAuto(v, name, &acpi, errp); +} + +static void virt_set_acpi(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + MachineState *ms = MACHINE(obj); + RISCVVirtState *s = RISCV_VIRT_MACHINE(ms); + + visit_type_OnOffAuto(v, name, &s->acpi, errp); +} + static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine, DeviceState *dev) { @@ -1672,6 +1705,11 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value " "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS); object_class_property_set_description(oc, "aia-guests", str); + object_class_property_add(oc, "acpi", "OnOffAuto", + virt_get_acpi, virt_set_acpi, + NULL, NULL); + object_class_property_set_description(oc, "acpi", + "Enable ACPI"); } static const TypeInfo virt_machine_typeinfo = { diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h index 6c7885bf89..62efebaa32 100644 --- a/include/hw/riscv/virt.h +++ b/include/hw/riscv/virt.h @@ -58,6 +58,7 @@ struct RISCVVirtState { int aia_guests; char *oem_id; char *oem_table_id; + OnOffAuto acpi; }; enum { @@ -123,4 +124,5 @@ enum { #define FDT_APLIC_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \ 1 + FDT_APLIC_INT_CELLS) +bool virt_is_acpi_enabled(RISCVVirtState *s); #endif
ACPI is optional. So, add a switch to toggle. Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- hw/riscv/virt.c | 38 ++++++++++++++++++++++++++++++++++++++ include/hw/riscv/virt.h | 2 ++ 2 files changed, 40 insertions(+)