Re: [PATCH 16/35] acpi: ipmi: use AcpiDevAmlIf interface to build IPMI device descriptors
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
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, );
> +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()));
> +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, >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(, 0, sizeof(info));
> -iic->get_fwinfo(ii, );
> -aml_append(scope, aml_ipmi_device());
> -}
> +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 =
[PATCH 16/35] acpi: ipmi: use AcpiDevAmlIf interface to build IPMI device descriptors
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
---
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, );
+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()));
+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, >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(, 0, sizeof(info));
-iic->get_fwinfo(ii, );
-aml_append(scope, aml_ipmi_device());
-}
+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
