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

[Igor Mammedov <imammedo@redhat.com>]

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  
>