[Qemu-devel] [PATCH v5 0/6] Add IPMI to firmware tables

[Qemu-devel] [PATCH v5 0/6] Add IPMI to firmware tables

[minyard]

This update just fixes some style issues found by checkpatch.pl,
which I should have checked to see if it existed, but Markus
introduced me to.

-corey

[Qemu-devel] [PATCH v5 1/6] ipmi: rework the fwinfo to be fetched from the interface

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Instead of scanning IPMI devices from a fwinfo list, allow
the fwinfo to be fetched from the IPMI interface class.
Then the code looking for IPMI fwinfo can scan devices on a
bus and look for ones that implement the IPMI class.

This will let the ACPI scope be defined by the calling
code so the IPMI code doesn't have to know the scope.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi.c         | 34 +++++------------------
 hw/ipmi/isa_ipmi_bt.c  | 57 +++++++++++++++++++++-----------------
 hw/ipmi/isa_ipmi_kcs.c | 56 +++++++++++++++++++++-----------------
 include/hw/ipmi/ipmi.h | 74 ++++++++++++++++++++++++++------------------------
 4 files changed, 109 insertions(+), 112 deletions(-)

diff --git a/hw/ipmi/ipmi.c b/hw/ipmi/ipmi.c
index 6adec1e..f09f217 100644
--- a/hw/ipmi/ipmi.c
+++ b/hw/ipmi/ipmi.c
@@ -30,6 +30,13 @@
 #include "qom/object_interfaces.h"
 #include "qapi/visitor.h"
 
+static uint32_t ipmi_current_uuid = 1;
+
+uint32_t ipmi_next_uuid(void)
+{
+    return ipmi_current_uuid++;
+}
+
 static int ipmi_do_hw_op(IPMIInterface *s, enum ipmi_op op, int checkonly)
 {
     switch (op) {
@@ -122,30 +129,3 @@ static void ipmi_register_types(void)
 }
 
 type_init(ipmi_register_types)
-
-static IPMIFwInfo *ipmi_fw_info;
-static unsigned int ipmi_fw_info_len;
-
-static uint32_t current_uuid = 1;
-
-void ipmi_add_fwinfo(IPMIFwInfo *info, Error **errp)
-{
-    info->uuid = current_uuid++;
-    ipmi_fw_info = g_realloc(ipmi_fw_info,
-                             sizeof(*ipmi_fw_info) * (ipmi_fw_info_len + 1));
-    ipmi_fw_info[ipmi_fw_info_len] = *info;
-}
-
-IPMIFwInfo *ipmi_first_fwinfo(void)
-{
-    return ipmi_fw_info;
-}
-
-IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current)
-{
-    current++;
-    if (current >= &ipmi_fw_info[ipmi_fw_info_len]) {
-        return NULL;
-    }
-    return current;
-}
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index aaea12e..f036617 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -390,16 +390,6 @@ static void ipmi_bt_init(IPMIInterface *ii, Error **errp)
     memory_region_init_io(&ib->io, NULL, &ipmi_bt_io_ops, ii, "ipmi-bt", 3);
 }
 
-static void ipmi_bt_class_init(IPMIInterfaceClass *iic)
-{
-    iic->init = ipmi_bt_init;
-    iic->set_atn = ipmi_bt_set_atn;
-    iic->handle_rsp = ipmi_bt_handle_rsp;
-    iic->handle_if_event = ipmi_bt_handle_event;
-    iic->set_irq_enable = ipmi_bt_set_irq_enable;
-    iic->reset = ipmi_bt_handle_reset;
-}
-
 
 #define TYPE_ISA_IPMI_BT "isa-ipmi-bt"
 #define ISA_IPMI_BT(obj) OBJECT_CHECK(ISAIPMIBTDevice, (obj), \
@@ -409,9 +399,38 @@ typedef struct ISAIPMIBTDevice {
     ISADevice dev;
     int32_t isairq;
     IPMIBT bt;
-    IPMIFwInfo fwinfo;
+    uint32_t uuid;
 } ISAIPMIBTDevice;
 
+static void ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
+{
+    ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
+
+    info->interface_name = "bt";
+    info->interface_type = IPMI_SMBIOS_BT;
+    info->ipmi_spec_major_revision = 2;
+    info->ipmi_spec_minor_revision = 0;
+    info->base_address = iib->bt.io_base;
+    info->register_length = iib->bt.io_length;
+    info->register_spacing = 1;
+    info->memspace = IPMI_MEMSPACE_IO;
+    info->irq_type = IPMI_LEVEL_IRQ;
+    info->interrupt_number = iib->isairq;
+    info->i2c_slave_address = iib->bt.bmc->slave_addr;
+    info->uuid = iib->uuid;
+}
+
+static void ipmi_bt_class_init(IPMIInterfaceClass *iic)
+{
+    iic->init = ipmi_bt_init;
+    iic->set_atn = ipmi_bt_set_atn;
+    iic->handle_rsp = ipmi_bt_handle_rsp;
+    iic->handle_if_event = ipmi_bt_handle_event;
+    iic->set_irq_enable = ipmi_bt_set_irq_enable;
+    iic->reset = ipmi_bt_handle_reset;
+    iic->get_fwinfo = ipmi_bt_get_fwinfo;
+}
+
 static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE(dev);
@@ -424,6 +443,8 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    iib->uuid = ipmi_next_uuid();
+
     iib->bt.bmc->intf = ii;
 
     iic->init(ii, errp);
@@ -438,20 +459,6 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
     qdev_set_legacy_instance_id(dev, iib->bt.io_base, iib->bt.io_length);
 
     isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
-
-    iib->fwinfo.interface_name = "bt";
-    iib->fwinfo.interface_type = IPMI_SMBIOS_BT;
-    iib->fwinfo.ipmi_spec_major_revision = 2;
-    iib->fwinfo.ipmi_spec_minor_revision = 0;
-    iib->fwinfo.base_address = iib->bt.io_base;
-    iib->fwinfo.register_length = iib->bt.io_length;
-    iib->fwinfo.register_spacing = 1;
-    iib->fwinfo.memspace = IPMI_MEMSPACE_IO;
-    iib->fwinfo.irq_type = IPMI_LEVEL_IRQ;
-    iib->fwinfo.interrupt_number = iib->isairq;
-    iib->fwinfo.acpi_parent = "\\_SB.PCI0.ISA";
-    iib->fwinfo.i2c_slave_address = iib->bt.bmc->slave_addr;
-    ipmi_add_fwinfo(&iib->fwinfo, errp);
 }
 
 static const VMStateDescription vmstate_ISAIPMIBTDevice = {
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 2742ce0..9a38f8a 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -354,16 +354,6 @@ static void ipmi_kcs_init(IPMIInterface *ii, Error **errp)
     memory_region_init_io(&ik->io, NULL, &ipmi_kcs_io_ops, ii, "ipmi-kcs", 2);
 }
 
-static void ipmi_kcs_class_init(IPMIInterfaceClass *iic)
-{
-    iic->init = ipmi_kcs_init;
-    iic->set_atn = ipmi_kcs_set_atn;
-    iic->handle_rsp = ipmi_kcs_handle_rsp;
-    iic->handle_if_event = ipmi_kcs_handle_event;
-    iic->set_irq_enable = ipmi_kcs_set_irq_enable;
-}
-
-
 #define TYPE_ISA_IPMI_KCS "isa-ipmi-kcs"
 #define ISA_IPMI_KCS(obj) OBJECT_CHECK(ISAIPMIKCSDevice, (obj), \
                                        TYPE_ISA_IPMI_KCS)
@@ -372,9 +362,37 @@ typedef struct ISAIPMIKCSDevice {
     ISADevice dev;
     int32_t isairq;
     IPMIKCS kcs;
-    IPMIFwInfo fwinfo;
+    uint32_t uuid;
 } ISAIPMIKCSDevice;
 
+static void ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
+{
+    ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
+
+    info->interface_name = "kcs";
+    info->interface_type = IPMI_SMBIOS_KCS;
+    info->ipmi_spec_major_revision = 2;
+    info->ipmi_spec_minor_revision = 0;
+    info->base_address = iik->kcs.io_base;
+    info->i2c_slave_address = iik->kcs.bmc->slave_addr;
+    info->register_length = iik->kcs.io_length;
+    info->register_spacing = 1;
+    info->memspace = IPMI_MEMSPACE_IO;
+    info->irq_type = IPMI_LEVEL_IRQ;
+    info->interrupt_number = iik->isairq;
+    info->uuid = iik->uuid;
+}
+
+static void ipmi_kcs_class_init(IPMIInterfaceClass *iic)
+{
+    iic->init = ipmi_kcs_init;
+    iic->set_atn = ipmi_kcs_set_atn;
+    iic->handle_rsp = ipmi_kcs_handle_rsp;
+    iic->handle_if_event = ipmi_kcs_handle_event;
+    iic->set_irq_enable = ipmi_kcs_set_irq_enable;
+    iic->get_fwinfo = ipmi_kcs_get_fwinfo;
+}
+
 static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE(dev);
@@ -387,6 +405,8 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    iik->uuid = ipmi_next_uuid();
+
     iik->kcs.bmc->intf = ii;
 
     iic->init(ii, errp);
@@ -401,20 +421,6 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
     qdev_set_legacy_instance_id(dev, iik->kcs.io_base, iik->kcs.io_length);
 
     isa_register_ioport(isadev, &iik->kcs.io, iik->kcs.io_base);
-
-    iik->fwinfo.interface_name = "kcs";
-    iik->fwinfo.interface_type = IPMI_SMBIOS_KCS;
-    iik->fwinfo.ipmi_spec_major_revision = 2;
-    iik->fwinfo.ipmi_spec_minor_revision = 0;
-    iik->fwinfo.base_address = iik->kcs.io_base;
-    iik->fwinfo.i2c_slave_address = iik->kcs.bmc->slave_addr;
-    iik->fwinfo.register_length = iik->kcs.io_length;
-    iik->fwinfo.register_spacing = 1;
-    iik->fwinfo.memspace = IPMI_MEMSPACE_IO;
-    iik->fwinfo.irq_type = IPMI_LEVEL_IRQ;
-    iik->fwinfo.interrupt_number = iik->isairq;
-    iik->fwinfo.acpi_parent = "\\_SB.PCI0.ISA";
-    ipmi_add_fwinfo(&iik->fwinfo, errp);
 }
 
 const VMStateDescription vmstate_ISAIPMIKCSDevice = {
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index 74a2b5a..91b83b5 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -65,6 +65,40 @@ enum ipmi_op {
 #define IPMI_SMBIOS_BT          0x03
 #define IPMI_SMBIOS_SSIF        0x04
 
+/*
+ * Used for transferring information to interfaces that add
+ * entries to firmware tables.
+ */
+typedef struct IPMIFwInfo {
+    const char *interface_name;
+    int interface_type;
+    uint8_t ipmi_spec_major_revision;
+    uint8_t ipmi_spec_minor_revision;
+    uint8_t i2c_slave_address;
+    uint32_t uuid;
+
+    uint64_t base_address;
+    uint64_t register_length;
+    uint8_t register_spacing;
+    enum {
+        IPMI_MEMSPACE_IO,
+        IPMI_MEMSPACE_MEM32,
+        IPMI_MEMSPACE_MEM64,
+        IPMI_MEMSPACE_SMBUS
+    } memspace;
+
+    int interrupt_number;
+    enum {
+        IPMI_LEVEL_IRQ,
+        IPMI_EDGE_IRQ
+    } irq_type;
+} IPMIFwInfo;
+
+/*
+ * Called by each instantiated IPMI interface device to get it's uuid.
+ */
+uint32_t ipmi_next_uuid(void);
+
 /* IPMI Interface types (KCS, SMIC, BT) are prefixed with this */
 #define TYPE_IPMI_INTERFACE_PREFIX "ipmi-interface-"
 
@@ -127,6 +161,11 @@ typedef struct IPMIInterfaceClass {
      * Set by the owner to hold the backend data for the interface.
      */
     void *(*get_backend_data)(struct IPMIInterface *s);
+
+    /*
+     * Return the firmware info for a device.
+     */
+    void (*get_fwinfo)(struct IPMIInterface *s, IPMIFwInfo *info);
 } IPMIInterfaceClass;
 
 /*
@@ -168,41 +207,6 @@ typedef struct IPMIBmcClass {
  */
 void ipmi_bmc_find_and_link(Object *obj, Object **bmc);
 
-/*
- * Used for transferring information to interfaces that add
- * entries to firmware tables.
- */
-typedef struct IPMIFwInfo {
-    const char *interface_name;
-    int interface_type;
-    uint8_t ipmi_spec_major_revision;
-    uint8_t ipmi_spec_minor_revision;
-    uint8_t i2c_slave_address;
-    uint32_t uuid;
-
-    uint64_t base_address;
-    uint64_t register_length;
-    uint8_t register_spacing;
-    enum {
-        IPMI_MEMSPACE_IO,
-        IPMI_MEMSPACE_MEM32,
-        IPMI_MEMSPACE_MEM64,
-        IPMI_MEMSPACE_SMBUS
-    } memspace;
-
-    int interrupt_number;
-    enum {
-        IPMI_LEVEL_IRQ,
-        IPMI_EDGE_IRQ
-    } irq_type;
-
-    const char *acpi_parent;
-} IPMIFwInfo;
-
-void ipmi_add_fwinfo(IPMIFwInfo *info, Error **errp);
-IPMIFwInfo *ipmi_first_fwinfo(void);
-IPMIFwInfo *ipmi_next_fwinfo(IPMIFwInfo *current);
-
 #ifdef IPMI_DEBUG
 #define ipmi_debug(fs, ...) \
     fprintf(stderr, "IPMI (%s): " fs, __func__, ##__VA_ARGS__)
-- 
2.7.4

[Qemu-devel] [PATCH v5 2/6] pc: Postpone SMBIOS table installation to post machine init

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is the same place that the ACPI SSDT table gets added, so that
devices can add themselves to the SMBIOS table.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i386/pc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 99437e0..5e78ef4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -764,8 +764,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
                      acpi_tables, acpi_tables_len);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
 
-    pc_build_smbios(fw_cfg);
-
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
                      &e820_reserve, sizeof(e820_reserve));
     fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
@@ -1161,6 +1159,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 {
     PCMachineState *pcms = container_of(notifier,
                                         PCMachineState, machine_done);
+    FWCfgState *fw_cfg = pcms->fw_cfg;
     PCIBus *bus = pcms->bus;
 
     if (bus) {
@@ -1172,15 +1171,17 @@ void pc_machine_done(Notifier *notifier, void *data)
                 extra_hosts++;
             }
         }
-        if (extra_hosts && pcms->fw_cfg) {
+        if (extra_hosts && fw_cfg) {
             uint64_t *val = g_malloc(sizeof(*val));
             *val = cpu_to_le64(extra_hosts);
-            fw_cfg_add_file(pcms->fw_cfg,
-                    "etc/extra-pci-roots", val, sizeof(*val));
+            fw_cfg_add_file(fw_cfg, "etc/extra-pci-roots", val, sizeof(*val));
         }
     }
 
     acpi_setup();
+    if (fw_cfg) {
+        pc_build_smbios(fw_cfg);
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.7.4

[Qemu-devel] [PATCH v5 3/6] smbios: Move table build tools into an include file.

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This will let things in other files (like IPMI) build SMBIOS tables.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/smbios/smbios.c       | 70 ++++---------------------------------------
 hw/smbios/smbios_build.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 64 deletions(-)
 create mode 100644 hw/smbios/smbios_build.h

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index cb8a111..5dc3e43 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -24,6 +24,7 @@
 #include "hw/smbios/smbios.h"
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
+#include "smbios_build.h"
 
 /* legacy structures and constants for <= 2.0 machines */
 struct smbios_header {
@@ -53,10 +54,10 @@ static bool smbios_uuid_encoded = true;
 /* end: legacy structures & constants for <= 2.0 machines */
 
 
-static uint8_t *smbios_tables;
-static size_t smbios_tables_len;
-static unsigned smbios_table_max;
-static unsigned smbios_table_cnt;
+uint8_t *smbios_tables;
+size_t smbios_tables_len;
+unsigned smbios_table_max;
+unsigned smbios_table_cnt;
 static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
 
 static SmbiosEntryPoint ep;
@@ -429,7 +430,7 @@ uint8_t *smbios_get_table_legacy(size_t *length)
 /* end: legacy setup functions for <= 2.0 machines */
 
 
-static bool smbios_skip_table(uint8_t type, bool required_table)
+bool smbios_skip_table(uint8_t type, bool required_table)
 {
     if (test_bit(type, have_binfile_bitmap)) {
         return true; /* user provided their own binary blob(s) */
@@ -443,65 +444,6 @@ static bool smbios_skip_table(uint8_t type, bool required_table)
     return true;
 }
 
-#define SMBIOS_BUILD_TABLE_PRE(tbl_type, tbl_handle, tbl_required)        \
-    struct smbios_type_##tbl_type *t;                                     \
-    size_t t_off; /* table offset into smbios_tables */                   \
-    int str_index = 0;                                                    \
-    do {                                                                  \
-        /* should we skip building this table ? */                        \
-        if (smbios_skip_table(tbl_type, tbl_required)) {                  \
-            return;                                                       \
-        }                                                                 \
-                                                                          \
-        /* use offset of table t within smbios_tables */                  \
-        /* (pointer must be updated after each realloc) */                \
-        t_off = smbios_tables_len;                                        \
-        smbios_tables_len += sizeof(*t);                                  \
-        smbios_tables = g_realloc(smbios_tables, smbios_tables_len);      \
-        t = (struct smbios_type_##tbl_type *)(smbios_tables + t_off);     \
-                                                                          \
-        t->header.type = tbl_type;                                        \
-        t->header.length = sizeof(*t);                                    \
-        t->header.handle = cpu_to_le16(tbl_handle);                       \
-    } while (0)
-
-#define SMBIOS_TABLE_SET_STR(tbl_type, field, value)                      \
-    do {                                                                  \
-        int len = (value != NULL) ? strlen(value) + 1 : 0;                \
-        if (len > 1) {                                                    \
-            smbios_tables = g_realloc(smbios_tables,                      \
-                                      smbios_tables_len + len);           \
-            memcpy(smbios_tables + smbios_tables_len, value, len);        \
-            smbios_tables_len += len;                                     \
-            /* update pointer post-realloc */                             \
-            t = (struct smbios_type_##tbl_type *)(smbios_tables + t_off); \
-            t->field = ++str_index;                                       \
-        } else {                                                          \
-            t->field = 0;                                                 \
-        }                                                                 \
-    } while (0)
-
-#define SMBIOS_BUILD_TABLE_POST                                           \
-    do {                                                                  \
-        size_t term_cnt, t_size;                                          \
-                                                                          \
-        /* add '\0' terminator (add two if no strings defined) */         \
-        term_cnt = (str_index == 0) ? 2 : 1;                              \
-        smbios_tables = g_realloc(smbios_tables,                          \
-                                  smbios_tables_len + term_cnt);          \
-        memset(smbios_tables + smbios_tables_len, 0, term_cnt);           \
-        smbios_tables_len += term_cnt;                                    \
-                                                                          \
-        /* update smbios max. element size */                             \
-        t_size = smbios_tables_len - t_off;                               \
-        if (t_size > smbios_table_max) {                                  \
-            smbios_table_max = t_size;                                    \
-        }                                                                 \
-                                                                          \
-        /* update smbios element count */                                 \
-        smbios_table_cnt++;                                               \
-    } while (0)
-
 static void smbios_build_type_0_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(0, 0x000, false); /* optional, leave up to BIOS */
diff --git a/hw/smbios/smbios_build.h b/hw/smbios/smbios_build.h
new file mode 100644
index 0000000..2257721
--- /dev/null
+++ b/hw/smbios/smbios_build.h
@@ -0,0 +1,77 @@
+/*
+ * SMBIOS Support
+ *
+ * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
+ * Copyright (C) 2013 Red Hat, Inc.
+ */
+
+#ifndef QEMU_SMBIOS_BUILD_H
+#define QEMU_SMBIOS_BUILD_H
+
+bool smbios_skip_table(uint8_t type, bool required_table);
+
+extern uint8_t *smbios_tables;
+extern size_t smbios_tables_len;
+extern unsigned smbios_table_max;
+extern unsigned smbios_table_cnt;
+
+#define SMBIOS_BUILD_TABLE_PRE(tbl_type, tbl_handle, tbl_required)        \
+    struct smbios_type_##tbl_type *t;                                     \
+    size_t t_off; /* table offset into smbios_tables */                   \
+    int str_index = 0;                                                    \
+    do {                                                                  \
+        /* should we skip building this table ? */                        \
+        if (smbios_skip_table(tbl_type, tbl_required)) {                  \
+            return;                                                       \
+        }                                                                 \
+                                                                          \
+        /* use offset of table t within smbios_tables */                  \
+        /* (pointer must be updated after each realloc) */                \
+        t_off = smbios_tables_len;                                        \
+        smbios_tables_len += sizeof(*t);                                  \
+        smbios_tables = g_realloc(smbios_tables, smbios_tables_len);      \
+        t = (struct smbios_type_##tbl_type *)(smbios_tables + t_off);     \
+                                                                          \
+        t->header.type = tbl_type;                                        \
+        t->header.length = sizeof(*t);                                    \
+        t->header.handle = cpu_to_le16(tbl_handle);                       \
+    } while (0)
+
+#define SMBIOS_TABLE_SET_STR(tbl_type, field, value)                      \
+    do {                                                                  \
+        int len = (value != NULL) ? strlen(value) + 1 : 0;                \
+        if (len > 1) {                                                    \
+            smbios_tables = g_realloc(smbios_tables,                      \
+                                      smbios_tables_len + len);           \
+            memcpy(smbios_tables + smbios_tables_len, value, len);        \
+            smbios_tables_len += len;                                     \
+            /* update pointer post-realloc */                             \
+            t = (struct smbios_type_##tbl_type *)(smbios_tables + t_off); \
+            t->field = ++str_index;                                       \
+        } else {                                                          \
+            t->field = 0;                                                 \
+        }                                                                 \
+    } while (0)
+
+#define SMBIOS_BUILD_TABLE_POST                                           \
+    do {                                                                  \
+        size_t term_cnt, t_size;                                          \
+                                                                          \
+        /* add '\0' terminator (add two if no strings defined) */         \
+        term_cnt = (str_index == 0) ? 2 : 1;                              \
+        smbios_tables = g_realloc(smbios_tables,                          \
+                                  smbios_tables_len + term_cnt);          \
+        memset(smbios_tables + smbios_tables_len, 0, term_cnt);           \
+        smbios_tables_len += term_cnt;                                    \
+                                                                          \
+        /* update smbios max. element size */                             \
+        t_size = smbios_tables_len - t_off;                               \
+        if (t_size > smbios_table_max) {                                  \
+            smbios_table_max = t_size;                                    \
+        }                                                                 \
+                                                                          \
+        /* update smbios element count */                                 \
+        smbios_table_cnt++;                                               \
+    } while (0)
+
+#endif /* QEMU_SMBIOS_BUILD_H */
-- 
2.7.4

[Qemu-devel] [PATCH v5 4/6] ipmi: Add SMBIOS table entry

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add an IPMI table entry to the SMBIOS.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/smbios/Makefile.objs  |   2 +
 hw/smbios/ipmi.c         | 114 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/smbios/noipmi.c       |  14 ++++++
 hw/smbios/smbios.c       |   2 +
 include/hw/smbios/ipmi.h |  15 +++++++
 5 files changed, 147 insertions(+)
 create mode 100644 hw/smbios/ipmi.c
 create mode 100644 hw/smbios/noipmi.c
 create mode 100644 include/hw/smbios/ipmi.h

diff --git a/hw/smbios/Makefile.objs b/hw/smbios/Makefile.objs
index f69a92f..5578f51 100644
--- a/hw/smbios/Makefile.objs
+++ b/hw/smbios/Makefile.objs
@@ -1 +1,3 @@
 common-obj-$(CONFIG_SMBIOS) += smbios.o
+common-obj-$(call land,$(CONFIG_SMBIOS),$(CONFIG_IPMI)) += ipmi.o
+common-obj-$(call land,$(CONFIG_SMBIOS),$(call lnot,$(CONFIG_IPMI))) += noipmi.o
diff --git a/hw/smbios/ipmi.c b/hw/smbios/ipmi.c
new file mode 100644
index 0000000..fd524b8
--- /dev/null
+++ b/hw/smbios/ipmi.c
@@ -0,0 +1,114 @@
+/*
+ * IPMI SMBIOS firmware handling
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ipmi/ipmi.h"
+#include "hw/smbios/ipmi.h"
+#include "hw/smbios/smbios.h"
+#include "qemu/error-report.h"
+#include "smbios_build.h"
+
+/* SMBIOS type 38 - IPMI */
+struct smbios_type_38 {
+    struct smbios_structure_header header;
+    uint8_t interface_type;
+    uint8_t ipmi_spec_revision;
+    uint8_t i2c_slave_address;
+    uint8_t nv_storage_device_address;
+    uint64_t base_address;
+    uint8_t base_address_modifier;
+    uint8_t interrupt_number;
+} QEMU_PACKED;
+
+static void ipmi_encode_one_smbios(IPMIFwInfo *info)
+{
+    uint64_t baseaddr = info->base_address;
+    SMBIOS_BUILD_TABLE_PRE(38, 0x3000, true);
+
+    t->interface_type = info->interface_type;
+    t->ipmi_spec_revision = ((info->ipmi_spec_major_revision << 4)
+                             | info->ipmi_spec_minor_revision);
+    t->i2c_slave_address = info->i2c_slave_address;
+    t->nv_storage_device_address = 0;
+
+    /* or 1 to set it to I/O space */
+    switch (info->memspace) {
+    case IPMI_MEMSPACE_IO:
+        baseaddr |= 1;
+        break;
+    case IPMI_MEMSPACE_MEM32:
+    case IPMI_MEMSPACE_MEM64:
+        break;
+    case IPMI_MEMSPACE_SMBUS:
+        baseaddr <<= 1;
+        break;
+    }
+
+    t->base_address = cpu_to_le64(baseaddr);
+
+    t->base_address_modifier = 0;
+    if (info->irq_type == IPMI_LEVEL_IRQ) {
+        t->base_address_modifier |= 1;
+    }
+    switch (info->register_spacing) {
+    case 1:
+        break;
+    case 4:
+        t->base_address_modifier |= 1 << 6;
+        break;
+    case 16:
+        t->base_address_modifier |= 2 << 6;
+        break;
+    default:
+        error_report("IPMI register spacing %d is not compatible with"
+                     " SMBIOS, ignoring this entry.", info->register_spacing);
+        return;
+    }
+    t->interrupt_number = info->interrupt_number;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void search_for_ipmi_dev(BusState *bus)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children,  sibling) {
+        DeviceState *dev = kid->child;
+        Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE);
+        BusState *childbus;
+
+        if (obj) {
+            IPMIInterface *ii;
+            IPMIInterfaceClass *iic;
+            IPMIFwInfo info;
+
+            ii = IPMI_INTERFACE(obj);
+            iic = IPMI_INTERFACE_GET_CLASS(obj);
+            memset(&info, 0, sizeof(info));
+            iic->get_fwinfo(ii, &info);
+            ipmi_encode_one_smbios(&info);
+            continue;
+        }
+
+        QLIST_FOREACH(childbus, &dev->child_bus, sibling) {
+            search_for_ipmi_dev(childbus);
+        }
+    }
+}
+
+void smbios_build_type_38_table(void)
+{
+    BusState *bus;
+
+    bus = sysbus_get_default();
+    if (bus) {
+        search_for_ipmi_dev(bus);
+    }
+}
diff --git a/hw/smbios/noipmi.c b/hw/smbios/noipmi.c
new file mode 100644
index 0000000..ad669a4
--- /dev/null
+++ b/hw/smbios/noipmi.c
@@ -0,0 +1,14 @@
+/*
+ * IPMI SMBIOS firmware handling
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/smbios/ipmi.h"
+
+void smbios_build_type_38_table(void)
+{
+}
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 5dc3e43..74c7102 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -25,6 +25,7 @@
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
 #include "smbios_build.h"
+#include "hw/smbios/ipmi.h"
 
 /* legacy structures and constants for <= 2.0 machines */
 struct smbios_header {
@@ -848,6 +849,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
         }
 
         smbios_build_type_32_table();
+        smbios_build_type_38_table();
         smbios_build_type_127_table();
 
         smbios_validate_table();
diff --git a/include/hw/smbios/ipmi.h b/include/hw/smbios/ipmi.h
new file mode 100644
index 0000000..fd53c96
--- /dev/null
+++ b/include/hw/smbios/ipmi.h
@@ -0,0 +1,15 @@
+/*
+ * IPMI SMBIOS firmware handling
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_SMBIOS_IPMI_H
+#define QEMU_SMBIOS_IPMI_H
+
+void smbios_build_type_38_table(void);
+
+#endif /* QEMU_SMBIOS_IPMI_H */
-- 
2.7.4

[Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Use the new ACPI table construction tools to create an ACPI
entry for IPMI.  This adds a function called from build_dsdt
to add an DSDT entry for IPMI if IPMI is compiled in and has
registered firmware.  It also adds a dummy function if IPMI
is not compiled in.

This conforms to section "C3-2 Locating IPMI System Interfaces in
ACPI Name Space" in the IPMI 2.0 specification.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/Makefile.objs  |   2 +
 hw/acpi/ipmi.c         | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/noipmi.c       |  14 ++++++
 hw/i386/acpi-build.c   |  10 +++--
 hw/i386/pc_piix.c      |   1 +
 hw/i386/pc_q35.c       |   1 +
 include/hw/acpi/ipmi.h |  22 ++++++++++
 include/hw/i386/pc.h   |   1 +
 8 files changed, 162 insertions(+), 3 deletions(-)
 create mode 100644 hw/acpi/ipmi.c
 create mode 100644 hw/acpi/noipmi.c
 create mode 100644 include/hw/acpi/ipmi.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index faee86c..b8d5c84 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -6,3 +6,5 @@ obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
 common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
 common-obj-$(CONFIG_ACPI) += aml-build.o
+common-obj-$(call land,$(CONFIG_ACPI),$(CONFIG_IPMI)) += ipmi.o
+common-obj-$(call land,$(CONFIG_ACPI),$(call lnot,$(CONFIG_IPMI))) += noipmi.o
diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
new file mode 100644
index 0000000..3d61550
--- /dev/null
+++ b/hw/acpi/ipmi.c
@@ -0,0 +1,114 @@
+/*
+ * IPMI ACPI firmware handling
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ipmi/ipmi.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/ipmi.h"
+
+static Aml *aml_ipmi_crs(IPMIFwInfo *info, const char *scope)
+{
+    Aml *crs = aml_resource_template();
+    uint8_t regspacing = info->register_spacing;
+
+    /*
+     * The base address is fixed and cannot change.  That may be different
+     * if someone does PCI, but we aren't there yet.
+     */
+    switch (info->memspace) {
+    case IPMI_MEMSPACE_IO:
+        aml_append(crs, aml_io(AML_DECODE16, info->base_address,
+                               info->base_address + info->register_length - 1,
+                               regspacing, info->register_length));
+        break;
+    case IPMI_MEMSPACE_MEM32:
+        aml_append(crs,
+                   aml_dword_memory(AML_POS_DECODE,
+                            AML_MIN_FIXED, AML_MAX_FIXED,
+                            AML_NON_CACHEABLE, AML_READ_WRITE,
+                            0xffffffff,
+                            info->base_address,
+                            info->base_address + info->register_length - 1,
+                            regspacing, info->register_length));
+        break;
+    case IPMI_MEMSPACE_MEM64:
+        aml_append(crs,
+                   aml_qword_memory(AML_POS_DECODE,
+                            AML_MIN_FIXED, AML_MAX_FIXED,
+                            AML_NON_CACHEABLE, AML_READ_WRITE,
+                            0xffffffffffffffffULL,
+                            info->base_address,
+                            info->base_address + info->register_length - 1,
+                            regspacing, info->register_length));
+        break;
+    case IPMI_MEMSPACE_SMBUS:
+        aml_append(crs, aml_return(aml_int(info->base_address)));
+        break;
+    default:
+        abort();
+    }
+
+    if (info->interrupt_number) {
+        aml_append(crs, aml_irq_no_flags(info->interrupt_number));
+    }
+
+    return crs;
+}
+
+static void ipmi_encode_one_acpi(Aml *table, IPMIFwInfo *info,
+                                 const char *resource)
+{
+    Aml *dev, *method;
+    uint16_t version = ((info->ipmi_spec_major_revision << 8)
+                        | (info->ipmi_spec_minor_revision << 4));
+
+    dev = aml_device("MI0");
+    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, resource)));
+
+    /*
+     * The spec seems to require these to be methods.  All the examples
+     * show them this way and it doesn't seem to work if they are not.
+     */
+    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
+    aml_append(method, aml_return(aml_int(info->interface_type)));
+    aml_append(dev, method);
+    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
+    aml_append(method, aml_return(aml_int(version)));
+    aml_append(dev, method);
+
+    aml_append(table, dev);
+}
+
+void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children,  sibling) {
+        DeviceState *dev = kid->child;
+        Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE);
+        IPMIInterface *ii;
+        IPMIInterfaceClass *iic;
+        IPMIFwInfo info;
+
+        if (!obj) {
+            continue;
+        }
+
+        ii = IPMI_INTERFACE(obj);
+        iic = IPMI_INTERFACE_GET_CLASS(obj);
+        memset(&info, 0, sizeof(info));
+        iic->get_fwinfo(ii, &info);
+        ipmi_encode_one_acpi(table, &info, resource);
+    }
+}
diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c
new file mode 100644
index 0000000..dd7590d
--- /dev/null
+++ b/hw/acpi/noipmi.c
@@ -0,0 +1,14 @@
+/*
+ * IPMI ACPI firmware handling
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/acpi/ipmi.h"
+
+void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource)
+{
+}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 279f0d7..101082d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -59,6 +59,8 @@
 #include "qapi/qmp/qint.h"
 #include "qom/qom-qobject.h"
 
+#include "hw/acpi/ipmi.h"
+
 /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
  * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
  * a little bit, there should be plenty of free space since the DSDT
@@ -1445,7 +1447,7 @@ static Aml *build_com_device_aml(uint8_t uid)
     return dev;
 }
 
-static void build_isa_devices_aml(Aml *table)
+static void build_isa_devices_aml(Aml *table, ISABus *isa_bus)
 {
     ISADevice *fdc = pc_find_fdc0();
 
@@ -1461,6 +1463,8 @@ static void build_isa_devices_aml(Aml *table)
     aml_append(scope, build_com_device_aml(1));
     aml_append(scope, build_com_device_aml(2));
 
+    acpi_add_ipmi(scope, BUS(isa_bus), "\\_SB.PCI0.ISA");
+
     aml_append(table, scope);
 }
 
@@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
         build_hpet_aml(dsdt);
         build_piix4_pm(dsdt);
         build_piix4_isa_bridge(dsdt);
-        build_isa_devices_aml(dsdt);
+        build_isa_devices_aml(dsdt, pcms->isa_bus);
         build_piix4_pci_hotplug(dsdt);
         build_piix4_pci0_int(dsdt);
     } else {
@@ -2039,7 +2043,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 
         build_hpet_aml(dsdt);
         build_q35_isa_bridge(dsdt);
-        build_isa_devices_aml(dsdt);
+        build_isa_devices_aml(dsdt, pcms->isa_bus);
         build_q35_pci0_int(dsdt);
     }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7f50116..2e5ff45 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -185,6 +185,7 @@ static void pc_init1(MachineState *machine,
                               &error_abort);
         no_hpet = 1;
     }
+    pcms->isa_bus = isa_bus;
     isa_bus_irqs(isa_bus, gsi);
 
     if (kvm_pic_in_kernel()) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 04aae89..027655b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -190,6 +190,7 @@ static void pc_q35_init(MachineState *machine)
                  ICH9_LPC_NB_PIRQS);
     pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
     isa_bus = ich9_lpc->isa_bus;
+    pcms->isa_bus = isa_bus;
 
     /*end early*/
     isa_bus_irqs(isa_bus, gsi);
diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h
new file mode 100644
index 0000000..67d6e2f
--- /dev/null
+++ b/include/hw/acpi/ipmi.h
@@ -0,0 +1,22 @@
+/*
+ * QEMU IPMI ACPI handling
+ *
+ * Copyright (c) 2015 Corey Minyard <cminyard@mvista.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HW_ACPI_IPMI_H
+#define HW_ACPI_IPMI_H
+
+#include "qemu/osdep.h"
+#include "hw/acpi/aml-build.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 acpi_add_ipmi(Aml *table, BusState *bus, const char *resource);
+
+#endif /* HW_ACPI_IPMI_H */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 96f0b66..aa8bc97 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -51,6 +51,7 @@ struct PCMachineState {
     HotplugHandler *acpi_dev;
     ISADevice *rtc;
     PCIBus *bus;
+    ISABus *isa_bus;
     FWCfgState *fw_cfg;
 
     /* Configuration options: */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 2/6] pc: Postpone SMBIOS table installation to post machine init

[Igor Mammedov]

On Thu, 19 May 2016 10:23:58 -0500
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> This is the same place that the ACPI SSDT table gets added, so that
> devices can add themselves to the SMBIOS table.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/i386/pc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 99437e0..5e78ef4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -764,8 +764,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
>                       acpi_tables, acpi_tables_len);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
>  
> -    pc_build_smbios(fw_cfg);
> -
>      fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
>                       &e820_reserve, sizeof(e820_reserve));
>      fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
> @@ -1161,6 +1159,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>  {
>      PCMachineState *pcms = container_of(notifier,
>                                          PCMachineState, machine_done);
> +    FWCfgState *fw_cfg = pcms->fw_cfg;
Patch looks ok.
except of adding extra local var, I'd just use pcms->fw_cfg

>      PCIBus *bus = pcms->bus;
>  
>      if (bus) {
> @@ -1172,15 +1171,17 @@ void pc_machine_done(Notifier *notifier, void *data)
>                  extra_hosts++;
>              }
>          }
> -        if (extra_hosts && pcms->fw_cfg) {
> +        if (extra_hosts && fw_cfg) {
>              uint64_t *val = g_malloc(sizeof(*val));
>              *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(pcms->fw_cfg,
> -                    "etc/extra-pci-roots", val, sizeof(*val));
> +            fw_cfg_add_file(fw_cfg, "etc/extra-pci-roots", val, sizeof(*val));
>          }
>      }
>  
>      acpi_setup();
> +    if (fw_cfg) {
> +        pc_build_smbios(fw_cfg);
> +    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)

Re: [Qemu-devel] [PATCH v5 4/6] ipmi: Add SMBIOS table entry

[Igor Mammedov]

On Thu, 19 May 2016 10:24:00 -0500
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Add an IPMI table entry to the SMBIOS.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/smbios/Makefile.objs  |   2 +
>  hw/smbios/ipmi.c         | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>  hw/smbios/noipmi.c       |  14 ++++++
>  hw/smbios/smbios.c       |   2 +
>  include/hw/smbios/ipmi.h |  15 +++++++
>  5 files changed, 147 insertions(+)
>  create mode 100644 hw/smbios/ipmi.c
>  create mode 100644 hw/smbios/noipmi.c
>  create mode 100644 include/hw/smbios/ipmi.h
> 
> diff --git a/hw/smbios/Makefile.objs b/hw/smbios/Makefile.objs
> index f69a92f..5578f51 100644
> --- a/hw/smbios/Makefile.objs
> +++ b/hw/smbios/Makefile.objs
> @@ -1 +1,3 @@
>  common-obj-$(CONFIG_SMBIOS) += smbios.o
> +common-obj-$(call land,$(CONFIG_SMBIOS),$(CONFIG_IPMI)) += ipmi.o
> +common-obj-$(call land,$(CONFIG_SMBIOS),$(call lnot,$(CONFIG_IPMI))) += noipmi.o
why not use existing stub approach instead of adding conditional for noipmi.o?

[...]
> diff --git a/hw/smbios/noipmi.c b/hw/smbios/noipmi.c
this file should be in a/hw/stubs/
and maybe s/noipmi.c/smbios_type_38.c/

> new file mode 100644
> index 0000000..ad669a4
> --- /dev/null
> +++ b/hw/smbios/noipmi.c
> @@ -0,0 +1,14 @@
> +/*
> + * IPMI SMBIOS firmware handling
> + *
> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/smbios/ipmi.h"
> +
> +void smbios_build_type_38_table(void)
> +{
> +}
[...]

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Igor Mammedov]

On Thu, 19 May 2016 10:24:01 -0500
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Use the new ACPI table construction tools to create an ACPI
> entry for IPMI.  This adds a function called from build_dsdt
> to add an DSDT entry for IPMI if IPMI is compiled in and has
> registered firmware.  It also adds a dummy function if IPMI
> is not compiled in.
> 
> This conforms to section "C3-2 Locating IPMI System Interfaces in
> ACPI Name Space" in the IPMI 2.0 specification.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/acpi/Makefile.objs  |   2 +
>  hw/acpi/ipmi.c         | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/noipmi.c       |  14 ++++++
>  hw/i386/acpi-build.c   |  10 +++--
>  hw/i386/pc_piix.c      |   1 +
>  hw/i386/pc_q35.c       |   1 +
>  include/hw/acpi/ipmi.h |  22 ++++++++++
>  include/hw/i386/pc.h   |   1 +
>  8 files changed, 162 insertions(+), 3 deletions(-)
>  create mode 100644 hw/acpi/ipmi.c
>  create mode 100644 hw/acpi/noipmi.c
>  create mode 100644 include/hw/acpi/ipmi.h
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index faee86c..b8d5c84 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -6,3 +6,5 @@ obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
>  common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
>  common-obj-$(CONFIG_ACPI) += aml-build.o
> +common-obj-$(call land,$(CONFIG_ACPI),$(CONFIG_IPMI)) += ipmi.o
> +common-obj-$(call land,$(CONFIG_ACPI),$(call lnot,$(CONFIG_IPMI))) += noipmi.o
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> new file mode 100644
> index 0000000..3d61550
> --- /dev/null
> +++ b/hw/acpi/ipmi.c
> @@ -0,0 +1,114 @@
> +/*
> + * IPMI ACPI firmware handling
> + *
> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ipmi/ipmi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/ipmi.h"
> +
> +static Aml *aml_ipmi_crs(IPMIFwInfo *info, const char *scope)
scope is unused inside this function, drop it?

> +{
> +    Aml *crs = aml_resource_template();
> +    uint8_t regspacing = info->register_spacing;
> +
> +    /*
> +     * The base address is fixed and cannot change.  That may be different
> +     * if someone does PCI, but we aren't there yet.
> +     */
> +    switch (info->memspace) {
> +    case IPMI_MEMSPACE_IO:
> +        aml_append(crs, aml_io(AML_DECODE16, info->base_address,
> +                               info->base_address + info->register_length - 1,
> +                               regspacing, info->register_length));
> +        break;
> +    case IPMI_MEMSPACE_MEM32:
> +        aml_append(crs,
> +                   aml_dword_memory(AML_POS_DECODE,
> +                            AML_MIN_FIXED, AML_MAX_FIXED,
> +                            AML_NON_CACHEABLE, AML_READ_WRITE,
> +                            0xffffffff,
> +                            info->base_address,
> +                            info->base_address + info->register_length - 1,
> +                            regspacing, info->register_length));
> +        break;
> +    case IPMI_MEMSPACE_MEM64:
> +        aml_append(crs,
> +                   aml_qword_memory(AML_POS_DECODE,
> +                            AML_MIN_FIXED, AML_MAX_FIXED,
> +                            AML_NON_CACHEABLE, AML_READ_WRITE,
> +                            0xffffffffffffffffULL,
> +                            info->base_address,
> +                            info->base_address + info->register_length - 1,
> +                            regspacing, info->register_length));
> +        break;
> +    case IPMI_MEMSPACE_SMBUS:
> +        aml_append(crs, aml_return(aml_int(info->base_address)));
> +        break;
> +    default:
> +        abort();
> +    }
> +
> +    if (info->interrupt_number) {
> +        aml_append(crs, aml_irq_no_flags(info->interrupt_number));
> +    }
> +
> +    return crs;
> +}
> +
> +static void ipmi_encode_one_acpi(Aml *table, IPMIFwInfo *info,
> +                                 const char *resource)
it seems that 'resource' is used incorrectly in this patch,
or more exactly it's not needed.

s/ipmi_encode_one_acpi/acpi_ipmi_device/
drop 'table' and 'resource' args and make it return
'dev' it composes.

> +{
> +    Aml *dev, *method;
> +    uint16_t version = ((info->ipmi_spec_major_revision << 8)
> +                        | (info->ipmi_spec_minor_revision << 4));
> +
> +    dev = aml_device("MI0");
> +    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, resource)));
> +
> +    /*
> +     * The spec seems to require these to be methods.  All the examples
> +     * show them this way and it doesn't seem to work if they are not.
> +     */
> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(info->interface_type)));
> +    aml_append(dev, method);
> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(version)));
> +    aml_append(dev, method);
replace these methods with aml_name_decl() as they do not contain any logic
except of returning static value.

> +
> +    aml_append(table, dev);
> +}
> +
> +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource)
s/acpi_add_ipmi/build_acpi_ipmi_devices/

drop 'resource'
s/table/scope/

> +{
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &bus->children,  sibling) {
> +        DeviceState *dev = kid->child;
> +        Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE);
> +        IPMIInterface *ii;
> +        IPMIInterfaceClass *iic;
> +        IPMIFwInfo info;
> +
> +        if (!obj) {
> +            continue;
> +        }
> +
> +        ii = IPMI_INTERFACE(obj);
> +        iic = IPMI_INTERFACE_GET_CLASS(obj);
> +        memset(&info, 0, sizeof(info));
> +        iic->get_fwinfo(ii, &info);
> +        ipmi_encode_one_acpi(table, &info, resource);
aml_append(scope, my_ipmi_device)

> +    }
> +}
> diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c
> new file mode 100644
> index 0000000..dd7590d
> --- /dev/null
> +++ b/hw/acpi/noipmi.c
move this to .a/stubs/

> @@ -0,0 +1,14 @@
> +/*
> + * IPMI ACPI firmware handling
> + *
> + * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/acpi/ipmi.h"
> +
> +void acpi_add_ipmi(Aml *table, BusState *bus, const char *resource)
> +{
> +}
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 279f0d7..101082d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -59,6 +59,8 @@
>  #include "qapi/qmp/qint.h"
>  #include "qom/qom-qobject.h"
>  
> +#include "hw/acpi/ipmi.h"
> +
>  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
>   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
>   * a little bit, there should be plenty of free space since the DSDT
> @@ -1445,7 +1447,7 @@ static Aml *build_com_device_aml(uint8_t uid)
>      return dev;
>  }
>  
> -static void build_isa_devices_aml(Aml *table)
> +static void build_isa_devices_aml(Aml *table, ISABus *isa_bus)
>  {
>      ISADevice *fdc = pc_find_fdc0();
>  
> @@ -1461,6 +1463,8 @@ static void build_isa_devices_aml(Aml *table)
>      aml_append(scope, build_com_device_aml(1));
>      aml_append(scope, build_com_device_aml(2));
>  
> +    acpi_add_ipmi(scope, BUS(isa_bus), "\\_SB.PCI0.ISA");
> +
>      aml_append(table, scope);
>  }
>  
> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>          build_hpet_aml(dsdt);
>          build_piix4_pm(dsdt);
>          build_piix4_isa_bridge(dsdt);
> -        build_isa_devices_aml(dsdt);
> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
I'm not sure about adding 'isa_bus' field to PCMachineState,
it might be better to find a ISA bus object internally in
build_isa_devices_aml() and assert if found more than one,
since code assumes that there is only one anyway.

>          build_piix4_pci_hotplug(dsdt);
>          build_piix4_pci0_int(dsdt);
>      } else {
> @@ -2039,7 +2043,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>  
>          build_hpet_aml(dsdt);
>          build_q35_isa_bridge(dsdt);
> -        build_isa_devices_aml(dsdt);
> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>          build_q35_pci0_int(dsdt);
>      }
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7f50116..2e5ff45 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -185,6 +185,7 @@ static void pc_init1(MachineState *machine,
>                                &error_abort);
>          no_hpet = 1;
>      }
> +    pcms->isa_bus = isa_bus;
>      isa_bus_irqs(isa_bus, gsi);
>  
>      if (kvm_pic_in_kernel()) {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 04aae89..027655b 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -190,6 +190,7 @@ static void pc_q35_init(MachineState *machine)
>                   ICH9_LPC_NB_PIRQS);
>      pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
>      isa_bus = ich9_lpc->isa_bus;
> +    pcms->isa_bus = isa_bus;
>  
>      /*end early*/
>      isa_bus_irqs(isa_bus, gsi);
> diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h
> new file mode 100644
> index 0000000..67d6e2f
> --- /dev/null
> +++ b/include/hw/acpi/ipmi.h
> @@ -0,0 +1,22 @@
> +/*
> + * QEMU IPMI ACPI handling
> + *
> + * Copyright (c) 2015 Corey Minyard <cminyard@mvista.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_ACPI_IPMI_H
> +#define HW_ACPI_IPMI_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/aml-build.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 acpi_add_ipmi(Aml *table, BusState *bus, const char *resource);
> +
> +#endif /* HW_ACPI_IPMI_H */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 96f0b66..aa8bc97 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -51,6 +51,7 @@ struct PCMachineState {
>      HotplugHandler *acpi_dev;
>      ISADevice *rtc;
>      PCIBus *bus;
> +    ISABus *isa_bus;
>      FWCfgState *fw_cfg;
>  
>      /* Configuration options: */

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard]

Thanks for all the comments.  I didn't know about stubs, as
there's nothing that currently uses it in hw directory, but
it's easy enough to add.  I did have two comment below:

On 05/20/2016 04:53 AM, Igor Mammedov wrote:
> On Thu, 19 May 2016 10:24:01 -0500
> minyard@acm.org wrote:
.
.
.
>
> +    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, resource)));
> +
> +    /*
> +     * The spec seems to require these to be methods.  All the examples
> +     * show them this way and it doesn't seem to work if they are not.
> +     */
> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(info->interface_type)));
> +    aml_append(dev, method);
> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(version)));
> +    aml_append(dev, method);
> replace these methods with aml_name_decl() as they do not contain any logic
> except of returning static value.

I'm not sure why, but what you ask doesn't work.  These have to be
methods, and that is show by the IPMI spec, as the comment above
these says.

.
.
.

>   
> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>           build_hpet_aml(dsdt);
>           build_piix4_pm(dsdt);one
>           build_piix4_isa_bridge(dsdt);
> -        build_isa_devices_aml(dsdt);
> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
> I'm not sure about adding 'isa_bus' field to PCMachineState,
> it might be better to find a ISA bus object internally in
> build_isa_devices_aml() and assert if found more than one,
> since code assumes that there is only one anyway.

I don't see a clean way to find the ISA bus object.  Well, I guess
you can scan the PCI bus for an ISA bus object, is that what you
mean?

-corey

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Marcel Apfelbaum]

On 05/22/2016 03:28 AM, Corey Minyard wrote:
> Thanks for all the comments.  I didn't know about stubs, as
> there's nothing that currently uses it in hw directory, but
> it's easy enough to add.  I did have two comment below:
>
> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>> On Thu, 19 May 2016 10:24:01 -0500
>> minyard@acm.org wrote:
> .
> .
> .
>>
>> +    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, resource)));
>> +
>> +    /*
>> +     * The spec seems to require these to be methods.  All the examples
>> +     * show them this way and it doesn't seem to work if they are not.
>> +     */
>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>> +    aml_append(dev, method);
>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_return(aml_int(version)));
>> +    aml_append(dev, method);
>> replace these methods with aml_name_decl() as they do not contain any logic
>> except of returning static value.
>
> I'm not sure why, but what you ask doesn't work.  These have to be
> methods, and that is show by the IPMI spec, as the comment above
> these says.
>
> .
> .
> .
>
>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>           build_hpet_aml(dsdt);
>>           build_piix4_pm(dsdt);one
>>           build_piix4_isa_bridge(dsdt);
>> -        build_isa_devices_aml(dsdt);
>> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>> I'm not sure about adding 'isa_bus' field to PCMachineState,
>> it might be better to find a ISA bus object internally in
>> build_isa_devices_aml() and assert if found more than one,
>> since code assumes that there is only one anyway.
>

I agree.

> I don't see a clean way to find the ISA bus object.  Well, I guess
> you can scan the PCI bus for an ISA bus object, is that what you
> mean?
>

You can run a QOM query. Please look for object_resolve_path function.
Please let me know if you have difficulties with it.

Thanks,
Marcel



> -corey
>

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Igor Mammedov]

On Sat, 21 May 2016 19:28:59 -0500
Corey Minyard <minyard@acm.org> wrote:

> Thanks for all the comments.  I didn't know about stubs, as
> there's nothing that currently uses it in hw directory, but
> it's easy enough to add.  I did have two comment below:
> 
> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
> > On Thu, 19 May 2016 10:24:01 -0500
> > minyard@acm.org wrote:  
> .
> .
> .
> >
> > +    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, resource)));
> > +
> > +    /*
> > +     * The spec seems to require these to be methods.  All the examples
> > +     * show them this way and it doesn't seem to work if they are not.
> > +     */
> > +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
> > +    aml_append(method, aml_return(aml_int(info->interface_type)));
> > +    aml_append(dev, method);
> > +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
> > +    aml_append(method, aml_return(aml_int(version)));
> > +    aml_append(dev, method);
> > replace these methods with aml_name_decl() as they do not contain any logic
> > except of returning static value.  
> 
> I'm not sure why, but what you ask doesn't work.  These have to be
> methods, and that is show by the IPMI spec, as the comment above
> these says.
on linux these methods are evaluated by ACPICA core and named constant
is equivalent to a method without arguments that returns constant value.

It might be worth to investigate why it doesn't work.

> -corey

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard]

On 05/23/2016 05:01 AM, Igor Mammedov wrote:
> On Sat, 21 May 2016 19:28:59 -0500
> Corey Minyard <minyard@acm.org> wrote:
>
>> Thanks for all the comments.  I didn't know about stubs, as
>> there's nothing that currently uses it in hw directory, but
>> it's easy enough to add.  I did have two comment below:
>>
>> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>>> On Thu, 19 May 2016 10:24:01 -0500
>>> minyard@acm.org wrote:
>> .
>> .
>> .
>>> +    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, resource)));
>>> +
>>> +    /*
>>> +     * The spec seems to require these to be methods.  All the examples
>>> +     * show them this way and it doesn't seem to work if they are not.
>>> +     */
>>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>>> +    aml_append(dev, method);
>>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>>> +    aml_append(method, aml_return(aml_int(version)));
>>> +    aml_append(dev, method);
>>> replace these methods with aml_name_decl() as they do not contain any logic
>>> except of returning static value.
>> I'm not sure why, but what you ask doesn't work.  These have to be
>> methods, and that is show by the IPMI spec, as the comment above
>> these says.
> on linux these methods are evaluated by ACPICA core and named constant
> is equivalent to a method without arguments that returns constant value.
>
> It might be worth to investigate why it doesn't work.

I just tried this again and it did work.  I'm not sure why it didn't work
before, if it was a change in Linux or my error.

However, the latest IPMI spec has the following text:

Note: _IFT and _SRV, following, have been reserved in ACPI 3.0 as names for
control methods defined for SPMI

Just because it works in Linux doesn't mean it will work on other OSes.
Wouldn't it be safer to use a method here?

-corey

>> -corey

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Igor Mammedov]

On Mon, 23 May 2016 07:42:32 -0500
Corey Minyard <minyard@acm.org> wrote:

> On 05/23/2016 05:01 AM, Igor Mammedov wrote:
> > On Sat, 21 May 2016 19:28:59 -0500
> > Corey Minyard <minyard@acm.org> wrote:
> >  
> >> Thanks for all the comments.  I didn't know about stubs, as
> >> there's nothing that currently uses it in hw directory, but
> >> it's easy enough to add.  I did have two comment below:
> >>
> >> On 05/20/2016 04:53 AM, Igor Mammedov wrote:  
> >>> On Thu, 19 May 2016 10:24:01 -0500
> >>> minyard@acm.org wrote:  
> >> .
> >> .
> >> .  
> >>> +    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, resource)));
> >>> +
> >>> +    /*
> >>> +     * The spec seems to require these to be methods.  All the examples
> >>> +     * show them this way and it doesn't seem to work if they are not.
> >>> +     */
> >>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
> >>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
> >>> +    aml_append(dev, method);
> >>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
> >>> +    aml_append(method, aml_return(aml_int(version)));
> >>> +    aml_append(dev, method);
> >>> replace these methods with aml_name_decl() as they do not contain any logic
> >>> except of returning static value.  
> >> I'm not sure why, but what you ask doesn't work.  These have to be
> >> methods, and that is show by the IPMI spec, as the comment above
> >> these says.  
> > on linux these methods are evaluated by ACPICA core and named constant
> > is equivalent to a method without arguments that returns constant value.
> >
> > It might be worth to investigate why it doesn't work.  
> 
> I just tried this again and it did work.  I'm not sure why it didn't work
> before, if it was a change in Linux or my error.
> 
> However, the latest IPMI spec has the following text:
> 
> Note: _IFT and _SRV, following, have been reserved in ACPI 3.0 as names for
> control methods defined for SPMI
> 
> Just because it works in Linux doesn't mean it will work on other OSes.
> Wouldn't it be safer to use a method here?
I think it should be safe to use named constant instead of method.
Spec often uses terms method/object interchangeably for argument-less
methods and we use it to a make simpler/smaller AML bytecode.
So far there weren't any issues caused by it either on linux/windows guests.

PS:
ACPI6.0 spec uses named object instead of method for _IFT as an example.


> -corey
> 
> >> -corey  
> 

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard]

On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote:
> On 05/22/2016 03:28 AM, Corey Minyard wrote:
>> Thanks for all the comments.  I didn't know about stubs, as
>> there's nothing that currently uses it in hw directory, but
>> it's easy enough to add.  I did have two comment below:
>>
>> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>>> On Thu, 19 May 2016 10:24:01 -0500
>>> minyard@acm.org wrote:
>> .
>> .
>> .
>>>
>>> +    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, 
>>> resource)));
>>> +
>>> +    /*
>>> +     * The spec seems to require these to be methods.  All the 
>>> examples
>>> +     * show them this way and it doesn't seem to work if they are not.
>>> +     */
>>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>>> +    aml_append(dev, method);
>>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>>> +    aml_append(method, aml_return(aml_int(version)));
>>> +    aml_append(dev, method);
>>> replace these methods with aml_name_decl() as they do not contain 
>>> any logic
>>> except of returning static value.
>>
>> I'm not sure why, but what you ask doesn't work.  These have to be
>> methods, and that is show by the IPMI spec, as the comment above
>> these says.
>>
>> .
>> .
>> .
>>
>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>>           build_hpet_aml(dsdt);
>>>           build_piix4_pm(dsdt);one
>>>           build_piix4_isa_bridge(dsdt);
>>> -        build_isa_devices_aml(dsdt);
>>> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>>> I'm not sure about adding 'isa_bus' field to PCMachineState,
>>> it might be better to find a ISA bus object internally in
>>> build_isa_devices_aml() and assert if found more than one,
>>> since code assumes that there is only one anyway.
>>
>
> I agree.
>
>> I don't see a clean way to find the ISA bus object.  Well, I guess
>> you can scan the PCI bus for an ISA bus object, is that what you
>> mean?
>>
>
> You can run a QOM query. Please look for object_resolve_path function.
> Please let me know if you have difficulties with it.
>
> Thanks,
> Marcel
>

Thanks for that info.

I looked at that, and it works for a single object, but I don't see a way to
make this work if there is more than one IPMI device defined.

I changed to the following code, which seems to work ok.  You pass in the
ISA bus (or the SMBus in the I2C case).

void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
{
     BusChild *kid;

     QTAILQ_FOREACH(kid, &bus->children,  sibling) {
         DeviceState *dev = kid->child;
         Object *obj = object_dynamic_cast(OBJECT(dev), 
TYPE_IPMI_INTERFACE);
         IPMIInterface *ii;
         IPMIInterfaceClass *iic;
         IPMIFwInfo info;

         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, acpi_ipmi_device(&info));
     }
}

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Marcel Apfelbaum]

On 05/23/2016 04:39 PM, Corey Minyard wrote:
> On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote:
>> On 05/22/2016 03:28 AM, Corey Minyard wrote:
>>> Thanks for all the comments.  I didn't know about stubs, as
>>> there's nothing that currently uses it in hw directory, but
>>> it's easy enough to add.  I did have two comment below:
>>>
>>> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>>>> On Thu, 19 May 2016 10:24:01 -0500
>>>> minyard@acm.org wrote:
>>> .
>>> .
>>> .
>>>>
>>>> +    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, resource)));
>>>> +
>>>> +    /*
>>>> +     * The spec seems to require these to be methods.  All the examples
>>>> +     * show them this way and it doesn't seem to work if they are not.
>>>> +     */
>>>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>>>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>>>> +    aml_append(dev, method);
>>>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>>>> +    aml_append(method, aml_return(aml_int(version)));
>>>> +    aml_append(dev, method);
>>>> replace these methods with aml_name_decl() as they do not contain any logic
>>>> except of returning static value.
>>>
>>> I'm not sure why, but what you ask doesn't work.  These have to be
>>> methods, and that is show by the IPMI spec, as the comment above
>>> these says.
>>>
>>> .
>>> .
>>> .
>>>
>>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>>>           build_hpet_aml(dsdt);
>>>>           build_piix4_pm(dsdt);one
>>>>           build_piix4_isa_bridge(dsdt);
>>>> -        build_isa_devices_aml(dsdt);
>>>> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>>>> I'm not sure about adding 'isa_bus' field to PCMachineState,
>>>> it might be better to find a ISA bus object internally in
>>>> build_isa_devices_aml() and assert if found more than one,
>>>> since code assumes that there is only one anyway.
>>>
>>
>> I agree.
>>
>>> I don't see a clean way to find the ISA bus object.  Well, I guess
>>> you can scan the PCI bus for an ISA bus object, is that what you
>>> mean?
>>>
>>
>> You can run a QOM query. Please look for object_resolve_path function.
>> Please let me know if you have difficulties with it.
>>
>> Thanks,
>> Marcel
>>
>
> Thanks for that info.
>
> I looked at that, and it works for a single object, but I don't see a way to
> make this work if there is more than one IPMI device defined.


I was under impression that you are looking for a way to find the isa_bus.
Since we have only a single isa_bus you can use object_resolve_path instead
of adding the isa_bus to the pc machine.
I didn't refer to the IPMI device, sorry if I was not clear enough.

Thanks,
Marcel

>
> I changed to the following code, which seems to work ok.  You pass in the
> ISA bus (or the SMBus in the I2C case).
>
> void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
> {
>      BusChild *kid;
>
>      QTAILQ_FOREACH(kid, &bus->children,  sibling) {
>          DeviceState *dev = kid->child;
>          Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_IPMI_INTERFACE);
>          IPMIInterface *ii;
>          IPMIInterfaceClass *iic;
>          IPMIFwInfo info;
>
>          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, acpi_ipmi_device(&info));
>      }
> }

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard]

On 05/23/2016 08:51 AM, Marcel Apfelbaum wrote:
> On 05/23/2016 04:39 PM, Corey Minyard wrote:
>> On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote:
>>> On 05/22/2016 03:28 AM, Corey Minyard wrote:
>>>> Thanks for all the comments.  I didn't know about stubs, as
>>>> there's nothing that currently uses it in hw directory, but
>>>> it's easy enough to add.  I did have two comment below:
>>>>
>>>> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>>>>> On Thu, 19 May 2016 10:24:01 -0500
>>>>> minyard@acm.org wrote:
>>>> .
>>>> .
>>>> .
>>>>>
>>>>> +    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, 
>>>>> resource)));
>>>>> +
>>>>> +    /*
>>>>> +     * The spec seems to require these to be methods. All the 
>>>>> examples
>>>>> +     * show them this way and it doesn't seem to work if they are 
>>>>> not.
>>>>> +     */
>>>>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>>>>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>>>>> +    aml_append(dev, method);
>>>>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>>>>> +    aml_append(method, aml_return(aml_int(version)));
>>>>> +    aml_append(dev, method);
>>>>> replace these methods with aml_name_decl() as they do not contain 
>>>>> any logic
>>>>> except of returning static value.
>>>>
>>>> I'm not sure why, but what you ask doesn't work.  These have to be
>>>> methods, and that is show by the IPMI spec, as the comment above
>>>> these says.
>>>>
>>>> .
>>>> .
>>>> .
>>>>
>>>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>>>>           build_hpet_aml(dsdt);
>>>>>           build_piix4_pm(dsdt);one
>>>>>           build_piix4_isa_bridge(dsdt);
>>>>> -        build_isa_devices_aml(dsdt);
>>>>> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>>>>> I'm not sure about adding 'isa_bus' field to PCMachineState,
>>>>> it might be better to find a ISA bus object internally in
>>>>> build_isa_devices_aml() and assert if found more than one,
>>>>> since code assumes that there is only one anyway.
>>>>
>>>
>>> I agree.
>>>
>>>> I don't see a clean way to find the ISA bus object.  Well, I guess
>>>> you can scan the PCI bus for an ISA bus object, is that what you
>>>> mean?
>>>>
>>>
>>> You can run a QOM query. Please look for object_resolve_path function.
>>> Please let me know if you have difficulties with it.
>>>
>>> Thanks,
>>> Marcel
>>>
>>
>> Thanks for that info.
>>
>> I looked at that, and it works for a single object, but I don't see a 
>> way to
>> make this work if there is more than one IPMI device defined.
>
>
> I was under impression that you are looking for a way to find the 
> isa_bus.
> Since we have only a single isa_bus you can use object_resolve_path 
> instead
> of adding the isa_bus to the pc machine.
> I didn't refer to the IPMI device, sorry if I was not clear enough.
>

Got it, thanks.  I was over thinking it.

-corey

> Thanks,
> Marcel
>
>>
>> I changed to the following code, which seems to work ok.  You pass in 
>> the
>> ISA bus (or the SMBus in the I2C case).
>>
>> void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
>> {
>>      BusChild *kid;
>>
>>      QTAILQ_FOREACH(kid, &bus->children,  sibling) {
>>          DeviceState *dev = kid->child;
>>          Object *obj = object_dynamic_cast(OBJECT(dev), 
>> TYPE_IPMI_INTERFACE);
>>          IPMIInterface *ii;
>>          IPMIInterfaceClass *iic;
>>          IPMIFwInfo info;
>>
>>          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, acpi_ipmi_device(&info));
>>      }
>> }
>

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Paolo Bonzini]

On 20/05/2016 11:53, Igor Mammedov wrote:
>> > diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c
>> > new file mode 100644
>> > index 0000000..dd7590d
>> > --- /dev/null
>> > +++ b/hw/acpi/noipmi.c
> move this to .a/stubs/
> 

Don't overuse stubs.  The stubs library is mostly to share code between
QEMU and the tools.  What Corey did is more similar to what hw/ already
does in other places.

Unfortunately, weak symbols are not portable. :(

Paolo

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard]

On 05/23/2016 01:06 PM, Paolo Bonzini wrote:
>
> On 20/05/2016 11:53, Igor Mammedov wrote:
>>>> diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c
>>>> new file mode 100644
>>>> index 0000000..dd7590d
>>>> --- /dev/null
>>>> +++ b/hw/acpi/noipmi.c
>> move this to .a/stubs/
>>
> Don't overuse stubs.  The stubs library is mostly to share code between
> QEMU and the tools.  What Corey did is more similar to what hw/ already
> does in other places.

Should I change it back?

> Unfortunately, weak symbols are not portable. :(

Yeah, that's really a pain.

-corey

> Paolo

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Igor Mammedov]

On Mon, 23 May 2016 20:06:57 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 20/05/2016 11:53, Igor Mammedov wrote:
> >> > diff --git a/hw/acpi/noipmi.c b/hw/acpi/noipmi.c
> >> > new file mode 100644
> >> > index 0000000..dd7590d
> >> > --- /dev/null
> >> > +++ b/hw/acpi/noipmi.c  
> > move this to .a/stubs/
> >   
> 
> Don't overuse stubs.  The stubs library is mostly to share code between
> QEMU and the tools.  What Corey did is more similar to what hw/ already
> does in other places.
So what's the rule when one should use stubs or not?

> 
> Unfortunately, weak symbols are not portable. :(
> 
> Paolo

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Paolo Bonzini]

On 23/05/2016 21:25, Corey Minyard wrote:
>> Don't overuse stubs.  The stubs library is mostly to share code between
>> QEMU and the tools.  What Corey did is more similar to what hw/ already
>> does in other places.
> 
> Should I change it back?

Yes, please.  Sorry.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Paolo Bonzini]

On 24/05/2016 09:17, Igor Mammedov wrote:
> > Don't overuse stubs.  The stubs library is mostly to share code between
> > QEMU and the tools.  What Corey did is more similar to what hw/ already
> > does in other places.
> 
> So what's the rule when one should use stubs or not?

If you have a config symbol to base your choice on, do not use stubs.

Paolo