| Message ID | 20220516152610.1963435-29-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc/q35: refactor ISA and SMBUS AML generation | expand |
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 <imammedo@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
On Mon, May 16, 2022 at 04:46:29PM -0400, Michael S. Tsirkin wrote: > 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 <imammedo@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. That problem isn't new and we already have a bunch of aml_* stubs because of that. I expect it'll work just fine, at worst we'll have to add a stub or two in case some calls are not covered yet. take care, Gerd
On Mon, 16 May 2022 16:46:29 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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 <imammedo@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. it compiles/passes tests with current code, but otherwise as Gerd already mentioned, current pattern for such issues is stub functions. Also we are currently pulling in aml_build library for ISA devices (this series spreads it to some more devices). We can isolate aml device builder into separate files (aka set of foo-device-acpi.c) to compile it out completely, but I'd prefer to do it separately from this refactoring, if you'd like to go this direction. It's just another pre-existing issue, and out of scope of this refactoring (which is already too big for my taste). As you can notice this series mostly moves ad-hoc AML to respective devices without rewriting it, to keep it simple and review-able, the other issues can be solved on top to keep it manageable. > 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. lets see how PCI conversion will end up, if stubs become to much of a burden, I for sure will try inline function idea. Or another a bit worse idea would be to use similar macro keyed of ACPI define. > > > @@ -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 >
On Tue, May 17, 2022 at 10:13:51AM +0200, Gerd Hoffmann wrote: > That problem isn't new and we already have a bunch of aml_* stubs > because of that. I expect it'll work just fine, at worst we'll > have to add a stub or two in case some calls are not covered yet. Right but adding these stubs is a bother, we keep missing some. If possible I'd like the solution to be cleaner than the status quo. Is adding a wrapper instead of setting a method directly such a big problem really?
On Wed, 18 May 2022 12:29:25 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 17, 2022 at 10:13:51AM +0200, Gerd Hoffmann wrote: > > That problem isn't new and we already have a bunch of aml_* stubs > > because of that. I expect it'll work just fine, at worst we'll > > have to add a stub or two in case some calls are not covered yet. > > Right but adding these stubs is a bother, we keep missing some. > If possible I'd like the solution to be cleaner than the status quo. > Is adding a wrapper instead of setting a method directly such > a big problem really? Stubs are the bother but not much compared to alternatives. I can't recall missing stubs recently (it's hard to miss them as it's build time failure that won't pass CI). However wrapper would introduce ifdeffenry instead of a stub. And my understanding was that it's not acceptable and stubs are what consensus approach is/was to eliminate/minimize ifdefs in the code. Also adding wrapper won't help anything, we also need to decouple AML code into separate source files to avoid dependency on AML routines and that is a bigger crunch that includes not only new source files but spreading CONFIG_APCI all over the tree, so I'm not sure if end result won't be worse compared to stubs. Stubs are not the cleanest ways around the issue but they would be simpler to maintain in the end.
On Mon, 16 May 2022 16:46:29 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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 <imammedo@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? it turns out meson somehow figures out dependency and pulls in CONFIG_ACPI implicitly for mips (well and the same for a bunch of other targets where acpi doesn't have any stake). > 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, ....) current docs/devel/build-system.rst suggests using stubs for conditional code (it even uses CONFIG_ACPI as example of such usage) > the build function itself is static, so compiler will > remove it if unused. what you are essentially suggesting is to make target in-depended file (that is built once. ex: fdc-isa) into several target dependent builds (due to dependency on build_aml_foo() symbol). While stubs allow to keep file independent by substituting build_aml_foo() with dummy stub. So I'm not sure we would like go that route. Anyways, I did try to give it a shot and failed, or maybe I misunderstood your idea completely resistance I've met: - CONFIG_ACPI define is poisoned (hacked around it, pretty sure in wrong way) - couldn't figure out meson.build rule that depends on several symbols yet (meson docs suggest it's possble) (i.e. add/build new fdc-isa-acpi.c file if both CONFIG_FDC_ISA & CONFIG_ACPI defined) we can use only CONFIG_ACPI and hope that linker will discard object file as unused if target has ACPI but does not have CONFIG_FDC_ISA (it's still lost build time on use-less object). - CONFIG_ACPI is not visible for target in-depended devices gave up here ... my ugly attempt is here: https://gitlab.com/imammedo/qemu/-/commit/9cb126c903a72582ac2f1643664b06414e25e0af > > @@ -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 >
On Wed, 18 May 2022 12:29:25 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 17, 2022 at 10:13:51AM +0200, Gerd Hoffmann wrote: > > That problem isn't new and we already have a bunch of aml_* stubs > > because of that. I expect it'll work just fine, at worst we'll > > have to add a stub or two in case some calls are not covered yet. > > Right but adding these stubs is a bother, we keep missing some. > If possible I'd like the solution to be cleaner than the status quo. > Is adding a wrapper instead of setting a method directly such > a big problem really? > here is stub based ACPI decoupling for isa devices we currently have in the tree: https://gitlab.com/imammedo/qemu/-/commits/decouple_build_aml_v1/ If it looks acceptable to you, I can prep/post it first and then rebase this series on top to reduce unnecessary churning.
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 = { @@ -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)
.. 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 <imammedo@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(-)