On Mon, May 16, 2022 at 11:26:03AM -0400, Igor Mammedov wrote:
> .. and clean up not longer needed conditionals in DSTD build code
> pvpanic-isa AML will be fetched and included when ISA bridge will
> build its own AML code (including attached devices).
>
> Expected AML change:
> the device under separate _SB.PCI0.ISA scope is moved directly
> under Device(ISA) node.
>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> include/hw/misc/pvpanic.h | 9 ---------
> hw/i386/acpi-build.c | 37 ----------------------------------
> hw/misc/pvpanic-isa.c | 42 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 46 deletions(-)
>
> diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
> index 7f16cc9b16..e520566ab0 100644
> --- a/include/hw/misc/pvpanic.h
> +++ b/include/hw/misc/pvpanic.h
> @@ -33,13 +33,4 @@ struct PVPanicState {
>
> void pvpanic_setup_io(PVPanicState *s, DeviceState *dev, unsigned size);
>
> -static inline uint16_t pvpanic_port(void)
> -{
> - Object *o = object_resolve_path_type("", TYPE_PVPANIC_ISA_DEVICE, NULL);
> - if (!o) {
> - return 0;
> - }
> - return object_property_get_uint(o, PVPANIC_IOPORT_PROP, NULL);
> -}
> -
> #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 517818cd9f..a42f41f373 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -30,7 +30,6 @@
> #include "hw/pci/pci.h"
> #include "hw/core/cpu.h"
> #include "target/i386/cpu.h"
> -#include "hw/misc/pvpanic.h"
> #include "hw/timer/hpet.h"
> #include "hw/acpi/acpi-defs.h"
> #include "hw/acpi/acpi.h"
> @@ -117,7 +116,6 @@ typedef struct AcpiMiscInfo {
> #endif
> const unsigned char *dsdt_code;
> unsigned dsdt_size;
> - uint16_t pvpanic_port;
> } AcpiMiscInfo;
>
> typedef struct AcpiBuildPciBusHotplugState {
> @@ -302,7 +300,6 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> #ifdef CONFIG_TPM
> info->tpm_version = tpm_get_version(tpm_find());
> #endif
> - info->pvpanic_port = pvpanic_port();
> }
>
> /*
> @@ -1749,40 +1746,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(dsdt, scope);
> }
>
> - if (misc->pvpanic_port) {
> - scope = aml_scope("\\_SB.PCI0.ISA");
> -
> - dev = aml_device("PEVT");
> - aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
> -
> - crs = aml_resource_template();
> - aml_append(crs,
> - aml_io(AML_DECODE16, misc->pvpanic_port, misc->pvpanic_port, 1,
> 1)
> - );
> - aml_append(dev, aml_name_decl("_CRS", crs));
> -
> - aml_append(dev, aml_operation_region("PEOR", AML_SYSTEM_IO,
> - aml_int(misc->pvpanic_port),
> 1));
> - field = aml_field("PEOR", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> - aml_append(field, aml_named_field("PEPT", 8));
> - aml_append(dev, field);
> -
> - /* device present, functioning, decoding, shown in UI */
> - aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> -
> - method = aml_method("RDPT", 0, AML_NOTSERIALIZED);
> - aml_append(method, aml_store(aml_name("PEPT"), aml_local(0)));
> - aml_append(method, aml_return(aml_local(0)));
> - aml_append(dev, method);
> -
> - method = aml_method("WRPT", 1, AML_NOTSERIALIZED);
> - aml_append(method, aml_store(aml_arg(0), aml_name("PEPT")));
> - aml_append(dev, method);
> -
> - aml_append(scope, dev);
> - aml_append(dsdt, scope);
> - }
> -
> sb_scope = aml_scope("\\_SB");
> {
> Object *pci_host;
> diff --git a/hw/misc/pvpanic-isa.c b/hw/misc/pvpanic-isa.c
> index b84d4d458d..ccec50f61b 100644
> --- a/hw/misc/pvpanic-isa.c
> +++ b/hw/misc/pvpanic-isa.c
> @@ -22,6 +22,7 @@
> #include "qom/object.h"
> #include "hw/isa/isa.h"
> #include "standard-headers/linux/pvpanic.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>
> OBJECT_DECLARE_SIMPLE_TYPE(PVPanicISAState, PVPANIC_ISA_DEVICE)
>
> @@ -63,6 +64,41 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error
> **errp)
> isa_register_ioport(d, &ps->mr, s->ioport);
> }
>
> +static void build_pvpanic_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
> +{
> + Aml *crs, *field, *method;
> + PVPanicISAState *s = PVPANIC_ISA_DEVICE(adev);
> + Aml *dev = aml_device("PEVT");
> +
> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
> +
> + crs = aml_resource_template();
> + aml_append(crs,
> + aml_io(AML_DECODE16, s->ioport, s->ioport, 1, 1)
> + );
> + aml_append(dev, aml_name_decl("_CRS", crs));
> +
> + aml_append(dev, aml_operation_region("PEOR", AML_SYSTEM_IO,
> + aml_int(s->ioport), 1));
> + field = aml_field("PEOR", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> + aml_append(field, aml_named_field("PEPT", 8));
> + aml_append(dev, field);
> +
> + /* device present, functioning, decoding, shown in UI */
> + aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> +
> + method = aml_method("RDPT", 0, AML_NOTSERIALIZED);
> + aml_append(method, aml_store(aml_name("PEPT"), aml_local(0)));
> + aml_append(method, aml_return(aml_local(0)));
> + aml_append(dev, method);
> +
> + method = aml_method("WRPT", 1, AML_NOTSERIALIZED);
> + aml_append(method, aml_store(aml_arg(0), aml_name("PEPT")));
> + aml_append(dev, method);
> +
> + aml_append(scope, dev);
> +}
> +
> static Property pvpanic_isa_properties[] = {
> DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505),
> DEFINE_PROP_UINT8("events", PVPanicISAState, pvpanic.events,
> @@ -73,10 +109,12 @@ static Property pvpanic_isa_properties[] = {
> static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> + AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
>
> dc->realize = pvpanic_isa_realizefn;
> device_class_set_props(dc, pvpanic_isa_properties);
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + adevc->build_dev_aml = build_pvpanic_isa_aml;
> }
>
> static const TypeInfo pvpanic_isa_info = {
So this really makes the device depend on ACPI.
What if the device is also built for a platform without ACPI?
Looks like it will fail to build.
E.g. mips has ISA too. What if one was to add pvpanic there?
I am not sure how important this is at the moment, but
I think the APIs should support this cleanly.
How about an inline function along the lines of:
acpi_set_build_dev_aml(DeviceClass *dc, ....)
the build function itself is static, so compiler will
remove it if unused.
> @@ -85,6 +123,10 @@ static const TypeInfo pvpanic_isa_info = {
> .instance_size = sizeof(PVPanicISAState),
> .instance_init = pvpanic_isa_initfn,
> .class_init = pvpanic_isa_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_ACPI_DEV_AML_IF },
> + { },
> + },
> };
>
> static void pvpanic_register_types(void)
> --
> 2.31.1
