diff mbox series

[28/35] acpi: pvpanic-isa: use AcpiDevAmlIfClass:build_dev_aml to provide device's AML

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

Commit Message

Igor Mammedov May 16, 2022, 3:26 p.m. UTC
.. 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(-)

Comments

Michael S. Tsirkin May 16, 2022, 8:46 p.m. UTC | #1
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
Gerd Hoffmann May 17, 2022, 8:13 a.m. UTC | #2
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
Igor Mammedov May 17, 2022, 4:01 p.m. UTC | #3
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  
>
Michael S. Tsirkin May 18, 2022, 4:29 p.m. UTC | #4
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?
Igor Mammedov May 19, 2022, 11:52 a.m. UTC | #5
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.
Igor Mammedov May 19, 2022, 5:56 p.m. UTC | #6
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  
>
Igor Mammedov May 26, 2022, 1:57 p.m. UTC | #7
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 mbox series

Patch

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)