Re: [PATCH 16/35] acpi: ipmi: use AcpiDevAmlIf interface to build IPMI device descriptors

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 16, 2022 at 11:25:51AM -0400, Igor Mammedov wrote:
> convert ad-hoc way we use to generate AML for ISA/SMB IPMI devices
> to a generic approach (i.e. make devices provide its own AML blobs
> like it is done with other ISA devices (ex. KBD))
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

could not apply this. rebase and repost?

> ---
>  include/hw/acpi/ipmi.h |  9 ++------
>  hw/acpi/ipmi-stub.c    |  2 +-
>  hw/acpi/ipmi.c         | 49 +++++++++++++-----------------------------
>  hw/i386/acpi-build.c   | 17 ++++++++++-----
>  hw/ipmi/isa_ipmi_bt.c  |  4 ++++
>  hw/ipmi/isa_ipmi_kcs.c |  4 ++++
>  hw/ipmi/smbus_ipmi.c   |  4 ++++
>  7 files changed, 42 insertions(+), 47 deletions(-)
> 
> diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h
> index c38483565c..6c8079c97a 100644
> --- a/include/hw/acpi/ipmi.h
> +++ b/include/hw/acpi/ipmi.h
> @@ -9,13 +9,8 @@
>  #ifndef HW_ACPI_IPMI_H
>  #define HW_ACPI_IPMI_H
>  
> -#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>  
> -/*
> - * Add ACPI IPMI entries for all registered IPMI devices whose parent
> - * bus matches the given bus.  The resource is the ACPI resource that
> - * contains the IPMI device, this is required for the I2C CRS.
> - */
> -void build_acpi_ipmi_devices(Aml *table, BusState *bus);
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope);
>  
>  #endif /* HW_ACPI_IPMI_H */
> diff --git a/hw/acpi/ipmi-stub.c b/hw/acpi/ipmi-stub.c
> index f525f71c2d..befaf0a882 100644
> --- a/hw/acpi/ipmi-stub.c
> +++ b/hw/acpi/ipmi-stub.c
> @@ -10,6 +10,6 @@
>  #include "qemu/osdep.h"
>  #include "hw/acpi/ipmi.h"
>  
> -void build_acpi_ipmi_devices(Aml *table, BusState *bus)
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope)
>  {
>  }
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index c30b44fcf5..a20e57d465 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -62,46 +62,27 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>      return crs;
>  }
>  
> -static Aml *aml_ipmi_device(IPMIFwInfo *info)
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope)
>  {
>      Aml *dev;
> -    uint16_t version = ((info->ipmi_spec_major_revision << 8)
> -                        | (info->ipmi_spec_minor_revision << 4));
> +    IPMIFwInfo info = {};
> +    IPMIInterface *ii = IPMI_INTERFACE(adev);
> +    IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
> +    uint16_t version;
>  
> -    assert(info->ipmi_spec_minor_revision <= 15);
> +    iic->get_fwinfo(ii, &info);
> +    assert(info.ipmi_spec_minor_revision <= 15);
> +    version = ((info.ipmi_spec_major_revision << 8)
> +              | (info.ipmi_spec_minor_revision << 4));
>  
> -    dev = aml_device("MI%d", info->uuid);
> +    dev = aml_device("MI%d", info.uuid);
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid("IPI0001")));
>      aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s",
> -                                                     info->interface_name)));
> -    aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid)));
> -    aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info)));
> -    aml_append(dev, aml_name_decl("_IFT", aml_int(info->interface_type)));
> +                                                     info.interface_name)));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(info.uuid)));
> +    aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(&info)));
> +    aml_append(dev, aml_name_decl("_IFT", aml_int(info.interface_type)));
>      aml_append(dev, aml_name_decl("_SRV", aml_int(version)));
>  
> -    return dev;
> -}
> -
> -void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
> -{
> -
> -    BusChild *kid;
> -
> -    QTAILQ_FOREACH(kid, &bus->children,  sibling) {
> -        IPMIInterface *ii;
> -        IPMIInterfaceClass *iic;
> -        IPMIFwInfo info;
> -        Object *obj = object_dynamic_cast(OBJECT(kid->child),
> -                                          TYPE_IPMI_INTERFACE);
> -
> -        if (!obj) {
> -            continue;
> -        }
> -
> -        ii = IPMI_INTERFACE(obj);
> -        iic = IPMI_INTERFACE_GET_CLASS(obj);
> -        memset(&info, 0, sizeof(info));
> -        iic->get_fwinfo(ii, &info);
> -        aml_append(scope, aml_ipmi_device(&info));
> -    }
> +    aml_append(scope, dev);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6dce8354cc..ca5cab87ba 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/isa/isa.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>  #include "hw/input/i8042.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
> @@ -71,7 +72,6 @@
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/virtio/virtio-iommu.h"
>  
> -#include "hw/acpi/ipmi.h"
>  #include "hw/acpi/hmat.h"
>  #include "hw/acpi/viot.h"
>  
> @@ -870,7 +870,6 @@ static void build_isa_devices_aml(Aml *table)
>      assert(obj && !ambiguous);
>  
>      scope = aml_scope("_SB.PCI0.ISA");
> -    build_acpi_ipmi_devices(scope, BUS(obj));
>      isa_build_aml(ISA_BUS(obj), scope);
>  
>      aml_append(table, scope);
> @@ -1397,13 +1396,21 @@ static Aml *build_q35_osc_method(bool enable_native_pcie_hotplug)
>      return method;
>  }
>  
> -static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
> +static void build_smb0(Aml *table, int devnr, int func)
>  {
>      Aml *scope = aml_scope("_SB.PCI0");
>      Aml *dev = aml_device("SMB0");
> +    bool ambiguous;
> +    Object *obj;
> +    /*
> +     * temporarily fish out device hosting SMBUS, build_smb0 will be gone once
> +     * PCI enumeration will be switched to call_dev_aml_func()
> +     */
> +    obj = object_resolve_path_type("", TYPE_ICH9_SMB_DEVICE, &ambiguous);
> +    assert(obj && !ambiguous);
>  
>      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
> -    build_acpi_ipmi_devices(dev, BUS(smbus));
> +    call_dev_aml_func(DEVICE(obj), dev);
>      aml_append(scope, dev);
>      aml_append(table, scope);
>  }
> @@ -1504,7 +1511,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          }
>          build_q35_pci0_int(dsdt);
>          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> -            build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> +            build_smb0(dsdt, ICH9_SMB_DEV, ICH9_SMB_FUNC);
>          }
>      }
>  
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index 88aa734e9e..a83e7243d6 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -31,6 +31,7 @@
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>  #include "qom/object.h"
> +#include "hw/acpi/ipmi.h"
>  
>  #define TYPE_ISA_IPMI_BT "isa-ipmi-bt"
>  OBJECT_DECLARE_SIMPLE_TYPE(ISAIPMIBTDevice, ISA_IPMI_BT)
> @@ -144,6 +145,7 @@ static void isa_ipmi_bt_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      IPMIInterfaceClass *iic = IPMI_INTERFACE_CLASS(oc);
> +    AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(oc);
>  
>      dc->realize = isa_ipmi_bt_realize;
>      device_class_set_props(dc, ipmi_isa_properties);
> @@ -151,6 +153,7 @@ static void isa_ipmi_bt_class_init(ObjectClass *oc, void *data)
>      iic->get_backend_data = isa_ipmi_bt_get_backend_data;
>      ipmi_bt_class_init(iic);
>      iic->get_fwinfo = isa_ipmi_bt_get_fwinfo;
> +    adevc->build_dev_aml = build_ipmi_dev_aml;
>  }
>  
>  static const TypeInfo isa_ipmi_bt_info = {
> @@ -161,6 +164,7 @@ static const TypeInfo isa_ipmi_bt_info = {
>      .class_init    = isa_ipmi_bt_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_IPMI_INTERFACE },
> +        { TYPE_ACPI_DEV_AML_IF },
>          { }
>      }
>  };
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index afabb95ebe..b2ed70b9da 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -31,6 +31,7 @@
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>  #include "qom/object.h"
> +#include "hw/acpi/ipmi.h"
>  
>  #define TYPE_ISA_IPMI_KCS "isa-ipmi-kcs"
>  OBJECT_DECLARE_SIMPLE_TYPE(ISAIPMIKCSDevice, ISA_IPMI_KCS)
> @@ -151,6 +152,7 @@ static void isa_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      IPMIInterfaceClass *iic = IPMI_INTERFACE_CLASS(oc);
> +    AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(oc);
>  
>      dc->realize = ipmi_isa_realize;
>      device_class_set_props(dc, ipmi_isa_properties);
> @@ -158,6 +160,7 @@ static void isa_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>      iic->get_backend_data = isa_ipmi_kcs_get_backend_data;
>      ipmi_kcs_class_init(iic);
>      iic->get_fwinfo = isa_ipmi_kcs_get_fwinfo;
> +    adevc->build_dev_aml = build_ipmi_dev_aml;
>  }
>  
>  static const TypeInfo isa_ipmi_kcs_info = {
> @@ -168,6 +171,7 @@ static const TypeInfo isa_ipmi_kcs_info = {
>      .class_init    = isa_ipmi_kcs_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_IPMI_INTERFACE },
> +        { TYPE_ACPI_DEV_AML_IF },
>          { }
>      }
>  };
> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> index 1fdf0a66b6..9ef9112dd5 100644
> --- a/hw/ipmi/smbus_ipmi.c
> +++ b/hw/ipmi/smbus_ipmi.c
> @@ -28,6 +28,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/ipmi/ipmi.h"
>  #include "qom/object.h"
> +#include "hw/acpi/ipmi.h"
>  
>  #define TYPE_SMBUS_IPMI "smbus-ipmi"
>  OBJECT_DECLARE_SIMPLE_TYPE(SMBusIPMIDevice, SMBUS_IPMI)
> @@ -353,6 +354,7 @@ static void smbus_ipmi_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      IPMIInterfaceClass *iic = IPMI_INTERFACE_CLASS(oc);
>      SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(oc);
> +    AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(oc);
>  
>      sc->receive_byte = ipmi_receive_byte;
>      sc->write_data = ipmi_write_data;
> @@ -363,6 +365,7 @@ static void smbus_ipmi_class_init(ObjectClass *oc, void *data)
>      iic->handle_if_event = smbus_ipmi_handle_event;
>      iic->set_irq_enable = smbus_ipmi_set_irq_enable;
>      iic->get_fwinfo = smbus_ipmi_get_fwinfo;
> +    adevc->build_dev_aml = build_ipmi_dev_aml;
>  }
>  
>  static const TypeInfo smbus_ipmi_info = {
> @@ -373,6 +376,7 @@ static const TypeInfo smbus_ipmi_info = {
>      .class_init    = smbus_ipmi_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_IPMI_INTERFACE },
> +        { TYPE_ACPI_DEV_AML_IF },
>          { }
>      }
>  };
> -- 
> 2.31.1