[Qemu-devel] [PATCH 0/7] Fix PM SMBus and add IPMI over SMBus

[Qemu-devel] [PATCH 0/7] Fix PM SMBus and add IPMI over SMBus

[minyard]

This series of patches fixes a bunch of issues in the PM SMBus driver
for x86 chips so that IPMI over SMBus can actually work (and other
things can work, too, I have a number of devices that I've added
later, and there's the eeprom, of course).

It then adds an IPMI device that sits on I2C and fixes up the ACPI
handling so it can be added to the ACPI tables.

This is mostly for comment for now, though I'd be happy for it to
be merged if it's good.  Yeah, not likely at this point.  I'd like
to add tests, but that would be a separate patch anyway.  Tests
will be fairly complicated as there doesn't appear to be any
tests for the PM SMBus.

IPMI over SMBus is not common now, but I've seen some new systems
coming out with it outside of the Intel world.  I've needed
a test bed to reproduce bugs and test out new capabilities, but
other systems may need it in the future.

Later in my queue is support for an SMBus alert device (allowing
a shared interrupt from SMBus devices) and support for a couple of
I2C bus multiplexers.  If you are interested, you can see my queue
at the ever changing https://github.com/cminyard/qemu master-ipmi-rebase
branch.

[Qemu-devel] [PATCH 1/7] i2c: Fix the PM SMBus driver so it actually works correctly

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The PM SMBus driver really just didn't work.  This patch fixes it
to be fairly hardware compliant with the actual hardware.  Plus
it adds interrupts and working block transfers.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/pm_smbus.c         | 198 ++++++++++++++++++++++++++++++++++++++++------
 hw/i2c/smbus.c            |  24 +++---
 hw/i2c/smbus_ich9.c       |  27 ++++++-
 include/hw/i2c/pm_smbus.h |  23 +++++-
 include/hw/i2c/smbus.h    |   5 +-
 5 files changed, 236 insertions(+), 41 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 6fc3923..d8d1032 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -23,8 +23,6 @@
 #include "hw/i2c/pm_smbus.h"
 #include "hw/i2c/smbus.h"
 
-/* no save/load? */
-
 #define SMBHSTSTS       0x00
 #define SMBHSTCNT       0x02
 #define SMBHSTCMD       0x03
@@ -32,8 +30,9 @@
 #define SMBHSTDAT0      0x05
 #define SMBHSTDAT1      0x06
 #define SMBBLKDAT       0x07
+#define SMBAUXCTL       0x0d
 
-#define STS_HOST_BUSY   (1)
+#define STS_HOST_BUSY   (1<<0)
 #define STS_INTR        (1<<1)
 #define STS_DEV_ERR     (1<<2)
 #define STS_BUS_ERR     (1<<3)
@@ -45,10 +44,28 @@
 *  ByteDoneStatus = 1 (STS_BYTE_DONE) and INTR = 1 (STS_INTR )
 */
 
-//#define DEBUG
+#define CTL_INTREN      (1<<0)
+#define CTL_KILL        (1<<1)
+#define CTL_LAST_BYTE   (1<<5)
+#define CTL_START       (1<<6)
+#define CTL_PEC_EN      (1<<7)
+
+#define PROT_QUICK          0
+#define PROT_BYTE           1
+#define PROT_BYTE_DATA      2
+#define PROT_WORD_DATA      3
+#define PROT_PROC_CALL      4
+#define PROT_BLOCK_DATA     5
+#define PROT_I2C_BLOCK_DATA 6
+
+#define AUX_PEC       (1<<0)
+#define AUX_BLK       (1<<1)
+#define AUX_MASK      0x3
+
+/*#define DEBUG*/
 
 #ifdef DEBUG
-# define SMBUS_DPRINTF(format, ...)     printf(format, ## __VA_ARGS__)
+# define SMBUS_DPRINTF(format, ...)     fprintf(stderr, format, ## __VA_ARGS__)
 #else
 # define SMBUS_DPRINTF(format, ...)     do { } while (0)
 #endif
@@ -61,6 +78,7 @@ static void smb_transaction(PMSMBus *s)
     uint8_t cmd = s->smb_cmd;
     uint8_t addr = s->smb_addr >> 1;
     I2CBus *bus = s->smbus;
+    bool i2c_enable = s->i2c_enable;
     int ret;
 
     SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
@@ -68,11 +86,12 @@ static void smb_transaction(PMSMBus *s)
     if ((s->smb_stat & STS_DEV_ERR) != 0)  {
         goto error;
     }
+
     switch(prot) {
-    case 0x0:
+    case PROT_QUICK:
         ret = smbus_quick_command(bus, addr, read);
         goto done;
-    case 0x1:
+    case PROT_BYTE:
         if (read) {
             ret = smbus_receive_byte(bus, addr);
             goto data8;
@@ -80,7 +99,7 @@ static void smb_transaction(PMSMBus *s)
             ret = smbus_send_byte(bus, addr, cmd);
             goto done;
         }
-    case 0x2:
+    case PROT_BYTE_DATA:
         if (read) {
             ret = smbus_read_byte(bus, addr, cmd);
             goto data8;
@@ -89,22 +108,58 @@ static void smb_transaction(PMSMBus *s)
             goto done;
         }
         break;
-    case 0x3:
+    case PROT_WORD_DATA:
         if (read) {
             ret = smbus_read_word(bus, addr, cmd);
             goto data16;
         } else {
-            ret = smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | s->smb_data0);
+            ret = smbus_write_word(bus, addr, cmd,
+                                   (s->smb_data1 << 8) | s->smb_data0);
             goto done;
         }
         break;
-    case 0x5:
+    case PROT_I2C_BLOCK_DATA:
+        cmd = s->smb_data1;
+        if (s->smb_ctl & CTL_LAST_BYTE) {
+            s->smb_data0 = 1;
+        } else {
+            s->smb_data0 = PM_SMBUS_MAX_MSG_SIZE;
+        }
+        read = true;
+        i2c_enable = true;
+        /* Fallthrough */
+    case PROT_BLOCK_DATA:
         if (read) {
-            ret = smbus_read_block(bus, addr, cmd, s->smb_data);
-            goto data8;
+            ret = smbus_read_block(bus, addr, cmd, s->smb_data,
+                                   sizeof(s->smb_data), !i2c_enable);
+            s->smb_index = 0;
+            s->op_done = false;
+            if (s->smb_auxctl & AUX_BLK) {
+                s->smb_stat |= STS_INTR;
+            } else {
+                s->smb_stat |= STS_HOST_BUSY;
+            }
+            goto datablk;
         } else {
-            ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
-            goto done;
+            if (s->smb_auxctl & AUX_BLK || s->smb_index == s->smb_data0) {
+                if (s->smb_index != s->smb_data0) {
+                    s->smb_index = 0;
+                    goto error;
+                }
+                /* Data is already all written to the queue, just do
+                   the operation. */
+                ret = smbus_write_block(bus, addr, cmd, s->smb_data,
+                                        s->smb_data0, !i2c_enable);
+                s->op_done = true;
+                s->smb_index = 0;
+                s->smb_stat |= STS_INTR;
+                s->smb_stat &= ~STS_HOST_BUSY;
+            } else {
+                s->op_done = false;
+                s->smb_stat |= STS_HOST_BUSY;
+                ret = 0;
+            }
+            goto doneblk;
         }
         break;
     default:
@@ -128,11 +183,27 @@ done:
     }
     s->smb_stat |= STS_BYTE_DONE | STS_INTR;
     return;
+datablk:
+    if (ret < 0) {
+        goto error;
+    }
+    s->smb_data0 = ret;
+doneblk:
+    if (ret < 0) {
+        goto error;
+    }
+    s->smb_stat |= STS_BYTE_DONE;
+    return;
 
 error:
     s->smb_stat |= STS_DEV_ERR;
     return;
+}
 
+static bool
+smb_irq_value(PMSMBus *s)
+{
+    return ((s->smb_stat & ~STS_HOST_BUSY) != 0) && (s->smb_ctl & CTL_INTREN);
 }
 
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
@@ -140,17 +211,30 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
 {
     PMSMBus *s = opaque;
 
-    SMBUS_DPRINTF("SMB writeb port=0x%04" HWADDR_PRIx
-                  " val=0x%02" PRIx64 "\n", addr, val);
+    SMBUS_DPRINTF("SMB writeb port=0x%04" HWADDR_PRIx " val=0x%02" PRIx64 "\n",
+                  addr, val);
     switch(addr) {
     case SMBHSTSTS:
-        s->smb_stat = (~(val & 0xff)) & s->smb_stat;
-        s->smb_index = 0;
+        s->smb_stat &= ~(val & ~STS_HOST_BUSY);
+        if (!s->op_done && !(s->smb_auxctl & AUX_BLK)) {
+            s->smb_stat |= STS_BYTE_DONE;
+        }
         break;
     case SMBHSTCNT:
         s->smb_ctl = val;
-        if (val & 0x40)
+        if (s->smb_ctl & CTL_START) {
+            if (!s->op_done) {
+                s->smb_index = 0;
+                s->op_done = true;
+            }
             smb_transaction(s);
+        }
+        if (s->smb_ctl & CTL_KILL) {
+            s->op_done = true;
+            s->smb_index = 0;
+            s->smb_stat |= STS_FAILED;
+            s->smb_stat &= ~STS_HOST_BUSY;
+        }
         break;
     case SMBHSTCMD:
         s->smb_cmd = val;
@@ -165,13 +249,28 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
         s->smb_data1 = val;
         break;
     case SMBBLKDAT:
-        s->smb_data[s->smb_index++] = val;
-        if (s->smb_index > 31)
+        if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
             s->smb_index = 0;
+        }
+        s->smb_data[s->smb_index++] = val;
+        if (!(s->smb_auxctl & AUX_BLK) && s->smb_ctl & CTL_START &&
+            !s->op_done && s->smb_index == s->smb_data0) {
+            smb_transaction(s);
+            s->op_done = true;
+            s->smb_stat |= STS_INTR;
+            s->smb_stat &= ~STS_HOST_BUSY;
+        }
+        break;
+    case SMBAUXCTL:
+        s->smb_auxctl = val & AUX_MASK;
         break;
     default:
         break;
     }
+
+    if (s->set_irq) {
+        s->set_irq(s, smb_irq_value(s));
+    }
 }
 
 static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
@@ -184,7 +283,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = s->smb_stat;
         break;
     case SMBHSTCNT:
-        s->smb_index = 0;
         val = s->smb_ctl & 0x1f;
         break;
     case SMBHSTCMD:
@@ -200,18 +298,47 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = s->smb_data1;
         break;
     case SMBBLKDAT:
+        if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
+            s->smb_index = 0;
+        }
         val = s->smb_data[s->smb_index++];
-        if (s->smb_index > 31)
+        if (s->smb_ctl & CTL_START && !s->op_done &&
+            s->smb_index == s->smb_data0) {
+            s->op_done = true;
+            s->smb_index = 0;
+            s->smb_stat &= ~STS_HOST_BUSY;
+        }
+        if (s->smb_ctl & CTL_LAST_BYTE) {
+            s->op_done = true;
             s->smb_index = 0;
+            s->smb_stat |= STS_INTR;
+            s->smb_stat &= ~STS_HOST_BUSY;
+        }
+        break;
+    case SMBAUXCTL:
+        val = s->smb_auxctl;
         break;
     default:
         val = 0;
         break;
     }
-    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, val);
+    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
+                  addr, val);
+
+    if (s->set_irq) {
+        s->set_irq(s, smb_irq_value(s));
+    }
+
     return val;
 }
 
+static void pm_smbus_reset(PMSMBus *s)
+{
+    s->op_done = true;
+    s->smb_index = 0;
+    s->smb_stat = 0;
+}
+
 static const MemoryRegionOps pm_smbus_ops = {
     .read = smb_ioport_readb,
     .write = smb_ioport_writeb,
@@ -220,8 +347,29 @@ static const MemoryRegionOps pm_smbus_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+const VMStateDescription pmsmb_vmstate = {
+    .name = "pmsmb",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(smb_stat, PMSMBus),
+        VMSTATE_UINT8(smb_ctl, PMSMBus),
+        VMSTATE_UINT8(smb_cmd, PMSMBus),
+        VMSTATE_UINT8(smb_addr, PMSMBus),
+        VMSTATE_UINT8(smb_data0, PMSMBus),
+        VMSTATE_UINT8(smb_data1, PMSMBus),
+        VMSTATE_VBUFFER_UINT32(smb_data, PMSMBus, 1, NULL, 0, smb_index),
+        VMSTATE_UINT8(smb_auxctl, PMSMBus),
+        VMSTATE_BOOL(i2c_enable, PMSMBus),
+        VMSTATE_BOOL(op_done, PMSMBus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
 {
+    smb->op_done = true;
+    smb->reset = pm_smbus_reset;
     smb->smbus = i2c_init_bus(parent, "i2c");
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 3979b3d..7016676 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -293,9 +293,10 @@ int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
     return 0;
 }
 
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data)
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len)
 {
-    int len;
+    int rlen;
     int i;
 
     if (i2c_start_transfer(bus, addr, 0)) {
@@ -303,20 +304,24 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data)
     }
     i2c_send(bus, command);
     i2c_start_transfer(bus, addr, 1);
-    len = i2c_recv(bus);
-    if (len > 32) {
-        len = 0;
+    if (recv_len) {
+        rlen = i2c_recv(bus);
+    } else {
+        rlen = len;
     }
-    for (i = 0; i < len; i++) {
+    if (rlen > len) {
+        rlen = 0;
+    }
+    for (i = 0; i < rlen; i++) {
         data[i] = i2c_recv(bus);
     }
     i2c_nack(bus);
     i2c_end_transfer(bus);
-    return len;
+    return rlen;
 }
 
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len)
+                      int len, bool send_len)
 {
     int i;
 
@@ -327,7 +332,8 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
         return -1;
     }
     i2c_send(bus, command);
-    i2c_send(bus, len);
+    if (send_len)
+        i2c_send(bus, len);
     for (i = 0; i < len; i++) {
         i2c_send(bus, data[i]);
     }
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 498f03e..8289cd1 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -42,6 +42,8 @@
 typedef struct ICH9SMBState {
     PCIDevice dev;
 
+    bool irq_enabled;
+
     PMSMBus smb;
 } ICH9SMBState;
 
@@ -50,7 +52,9 @@ static const VMStateDescription vmstate_ich9_smbus = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+        VMSTATE_BOOL(irq_enabled, ICH9SMBState),
+        VMSTATE_STRUCT(smb, ICH9SMBState, 1, pmsmb_vmstate, struct PMSMBus),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -63,12 +67,16 @@ static void ich9_smbus_write_config(PCIDevice *d, uint32_t address,
     pci_default_write_config(d, address, val, len);
     if (range_covers_byte(address, len, ICH9_SMB_HOSTC)) {
         uint8_t hostc = s->dev.config[ICH9_SMB_HOSTC];
-        if ((hostc & ICH9_SMB_HOSTC_HST_EN) &&
-            !(hostc & ICH9_SMB_HOSTC_I2C_EN)) {
+        if (hostc & ICH9_SMB_HOSTC_HST_EN) {
             memory_region_set_enabled(&s->smb.io, true);
         } else {
             memory_region_set_enabled(&s->smb.io, false);
         }
+        s->smb.i2c_enable = (hostc & ICH9_SMB_HOSTC_I2C_EN) != 0;
+        if (hostc & ICH9_SMB_HOSTC_SSRESET) {
+            s->smb.reset(&s->smb);
+            s->dev.config[ICH9_SMB_HOSTC] &= ~ICH9_SMB_HOSTC_SSRESET;
+        }
     }
 }
 
@@ -107,11 +115,24 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
     dc->cannot_instantiate_with_device_add_yet = true;
 }
 
+static void ich9_smb_set_irq(PMSMBus *pmsmb, bool enabled)
+{
+    ICH9SMBState *s = pmsmb->opaque;
+
+    if (enabled == s->irq_enabled)
+        return;
+
+    s->irq_enabled = enabled;
+    pci_set_irq(&s->dev, enabled);
+}
+
 I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
 {
     PCIDevice *d =
         pci_create_simple_multifunction(bus, devfn, true, TYPE_ICH9_SMB_DEVICE);
     ICH9SMBState *s = ICH9_SMB_DEVICE(d);
+    s->smb.set_irq = ich9_smb_set_irq;
+    s->smb.opaque = s;
     return s->smb.smbus;
 }
 
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 926603f..bfe740a 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -1,6 +1,8 @@
 #ifndef PM_SMBUS_H
 #define PM_SMBUS_H
 
+#define PM_SMBUS_MAX_MSG_SIZE 32
+
 typedef struct PMSMBus {
     I2CBus *smbus;
     MemoryRegion io;
@@ -11,10 +13,27 @@ typedef struct PMSMBus {
     uint8_t smb_addr;
     uint8_t smb_data0;
     uint8_t smb_data1;
-    uint8_t smb_data[32];
-    uint8_t smb_index;
+    uint8_t smb_data[PM_SMBUS_MAX_MSG_SIZE];
+    uint8_t smb_auxctl;
+    uint32_t smb_index;
+
+    /* Set by pm_smbus.c */
+    void (*reset)(struct PMSMBus *s);
+
+    /* Set by the user. */
+    bool i2c_enable;
+    void (*set_irq)(struct PMSMBus *s, bool enabled);
+    void *opaque;
+
+    /* Internally used by pm_smbus. */
+
+    /* Set on block transfers after the last byte has been read, so the
+       INTR bit can be set at the right time. */
+    bool op_done;
 } PMSMBus;
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
 
+extern const VMStateDescription pmsmb_vmstate;
+
 #endif /* !PM_SMBUS_H */
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 544bbc1..1270691 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -73,9 +73,10 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
 int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
 int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
 int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data);
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len);
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len);
+                      int len, bool send_len);
 
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
-- 
2.7.4

[Qemu-devel] [PATCH 2/7] pm_smbus: Add the ability to force block transfer enable

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The PIIX4 hardware has block transfer buffer always enabled in
the hardware, but the i801 does not.  Add a parameter to pm_smbus_init
to force on the block transfer so the PIIX4 handler can enable this
by default, as it was disabled by default before.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/piix4.c           | 2 +-
 hw/i2c/pm_smbus.c         | 4 +++-
 hw/i2c/smbus_ich9.c       | 2 +-
 include/hw/i2c/pm_smbus.h | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 16abdf1..ab3af21 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -466,7 +466,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x09;
-    pm_smbus_init(DEVICE(dev), &s->smb);
+    pm_smbus_init(DEVICE(dev), &s->smb, true);
     memory_region_set_enabled(&s->smb.io, pci_conf[0xd2] & 1);
     memory_region_add_subregion(pci_address_space_io(dev),
                                 s->smb_io_base, &s->smb.io);
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index d8d1032..4b4c165 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -366,11 +366,13 @@ const VMStateDescription pmsmb_vmstate = {
     }
 };
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
 {
     smb->op_done = true;
     smb->reset = pm_smbus_reset;
     smb->smbus = i2c_init_bus(parent, "i2c");
+    if (force_aux_blk)
+        smb->smb_auxctl |= AUX_BLK;
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
 }
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 8289cd1..4581cf6 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -90,7 +90,7 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
     pci_set_byte(d->config + ICH9_SMB_HOSTC, 0);
     /* TODO bar0, bar1: 64bit BAR support*/
 
-    pm_smbus_init(&d->qdev, &s->smb);
+    pm_smbus_init(&d->qdev, &s->smb, false);
     pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
                      &s->smb.io);
 }
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index bfe740a..e64a206 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -32,7 +32,7 @@ typedef struct PMSMBus {
     bool op_done;
 } PMSMBus;
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
 
 extern const VMStateDescription pmsmb_vmstate;
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/7] pc: Add the SMBus device to the ACPI tables

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i386/acpi-build.c                 |  15 +++++++++++++++
 hw/i386/pc.c                         |   2 ++
 hw/i386/pc_piix.c                    |   2 ++
 hw/i386/pc_q35.c                     |   2 ++
 include/hw/i386/pc.h                 |   4 ++++
 tests/acpi-test-data/pc/DSDT         | Bin 5587 -> 5626 bytes
 tests/acpi-test-data/pc/DSDT.bridge  | Bin 7446 -> 7485 bytes
 tests/acpi-test-data/pc/DSDT.ipmikcs | Bin 5683 -> 5722 bytes
 tests/acpi-test-data/q35/DSDT        | Bin 8357 -> 8396 bytes
 tests/acpi-test-data/q35/DSDT.bridge | Bin 8374 -> 8413 bytes
 tests/acpi-test-data/q35/DSDT.ipmibt | Bin 8456 -> 8495 bytes
 11 files changed, 25 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2294e7b..321b26f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1980,6 +1980,19 @@ static Aml *build_q35_osc_method(void)
     return method;
 }
 
+static void build_smb0(Aml *table, int devnr, int func)
+{
+    Aml *sb_scope = aml_scope("_SB");
+    Aml *pci0_scope = aml_scope("PCI0");
+    Aml *dev = aml_device("SMB0");
+
+    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
+    aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
+    aml_append(pci0_scope, dev);
+    aml_append(sb_scope, pci0_scope);
+    aml_append(table, sb_scope);
+}
+
 static void
 build_dsdt(GArray *table_data, GArray *linker,
            AcpiPmInfo *pm, AcpiMiscInfo *misc,
@@ -2044,6 +2057,8 @@ build_dsdt(GArray *table_data, GArray *linker,
         build_isa_devices_aml(dsdt);
         build_q35_pci0_int(dsdt);
     }
+    if (pcms->pci_smbus_devnr != -1)
+        build_smb0(dsdt, pcms->pci_smbus_devnr, pcms->pci_smbus_func);
 
     build_cpu_hotplug_aml(dsdt);
     build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5e78ef4..deb25db 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1889,6 +1889,8 @@ static void pc_machine_initfn(Object *obj)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
 
+    pcms->pci_smbus_devnr = -1;
+
     object_property_add(obj, PC_MACHINE_MEMHP_REGION_SIZE, "int",
                         pc_machine_get_hotplug_memory_region_size,
                         NULL, NULL, NULL, &error_abort);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7f50116..cf4fe53 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -261,6 +261,8 @@ static void pc_init1(MachineState *machine,
                               pc_machine_is_smm_enabled(pcms),
                               &piix4_pm);
         smbus_eeprom_init(smbus, 8, NULL, 0);
+        pcms->pci_smbus_devnr = piix3_devfn >> 3;
+        pcms->pci_smbus_func = 3;
 
         object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                                  TYPE_HOTPLUG_HANDLER,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 04aae89..5bc77d7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -244,6 +244,8 @@ static void pc_q35_init(MachineState *machine)
                                     PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
                                     0xb100),
                       8, NULL, 0);
+    pcms->pci_smbus_devnr = ICH9_SMB_DEV;
+    pcms->pci_smbus_func = ICH9_SMB_FUNC;
 
     pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 96f0b66..e0d2779 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -72,6 +72,10 @@ struct PCMachineState {
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
+
+    /* SMBus information: */
+    int pci_smbus_devnr;
+    int pci_smbus_func;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
diff --git a/tests/acpi-test-data/pc/DSDT b/tests/acpi-test-data/pc/DSDT
index 9d1274d3c2e2b7a316d5133d013b0550024ee413..4cd234c972dea489c453e67f7b320d09438657b5 100644
GIT binary patch
delta 63
zcmcbt{Y#t6CD<k8mnZ`R<D`vTmF#M2@xe~<0tx}no(9oPlEJ=C1|0Doo-RCW0t~Di
S@s2J*Jj@J?44a$S1Ni_qst;)Z

delta 24
fcmeyReOa5!CD<k8vM2)sqxnXzO7_jG*aP?gXB-E5

diff --git a/tests/acpi-test-data/pc/DSDT.bridge b/tests/acpi-test-data/pc/DSDT.bridge
index cf48c62aa71a7dd7d816fd4ef8ad626e39d4965c..438cd31e3de1e981c2c734dfc72981dbeee7b157 100644
GIT binary patch
delta 63
zcmbPcwbzQvCD<jzR+fQ*@zzGJN_I81_+Y1a0fhi(PlM<t$zWe61CDqPPZu6G0R~o%
Sct@8Y9%cqchRsdvQ$zs?V-D&7

delta 24
fcmdmMHO-34CD<iIOqPLxv3?_0CHv-8?2|<SS9AvZ

diff --git a/tests/acpi-test-data/pc/DSDT.ipmikcs b/tests/acpi-test-data/pc/DSDT.ipmikcs
index 280aeb3059eb9220108b9a2b7f0ac07819023b06..abacdfa3bca8843eb4c2b26afa326ebae02206c8 100644
GIT binary patch
delta 63
zcmdn2b4!QICD<h-N{oSlk!>SaCA*qhe6UlzfI@(?r$Ka+WU#N30Y|)trwb3800S#W
SyrWAH4>JQJ!{#RTjeG#?{tjRO

delta 24
gcmcbmvss7BCD<jzSd4*zarH*7O7_jG*f;P209}3uaR2}S

diff --git a/tests/acpi-test-data/q35/DSDT b/tests/acpi-test-data/q35/DSDT
index 1c089c34b06c9f2ea9fe67abb45498021319303c..9c1964afe6180e3fce7c7f9f6e78d7565c54f60f 100644
GIT binary patch
delta 63
zcmZ4Lc*c>-CD<k8i~<7#W9~+-MoBfb_+Y1a0fhi(PlM<t$zWe61CDqPPZu6G0R~o%
Sct@8Y9%cr4hRt1)((C{ts}BYM

delta 24
fcmX@(xYUu$CD<iosR9E7qrpb5M#;^aB&FB^V95s;

diff --git a/tests/acpi-test-data/q35/DSDT.bridge b/tests/acpi-test-data/q35/DSDT.bridge
index b29fcda0bb1717ff708668c6e98f3ded3f34a96c..e294f27e73f99172ec9d5d236e6abbbf15898019 100644
GIT binary patch
delta 63
zcmdnyc-N83CD<k8t^xxCBkxA8MoBfb_+Y1a0fhi(PlM<t$zWe61CDqPPZu6G0R~o%
Sct@8Y9%cr4hRt1)I_v-(KMv{u

delta 24
gcmccXxXqEvCD<ion*sv^<Ase}jgp%;NounL0Bbl0`v3p{

diff --git a/tests/acpi-test-data/q35/DSDT.ipmibt b/tests/acpi-test-data/q35/DSDT.ipmibt
index 6704538dee7dcfade6a1dc74eda5b8b832d7c6a2..d81ed0c63a0d40d854e37bda68c2328e0e213ae3 100644
GIT binary patch
delta 63
zcmeBhTJOZ=66_M9ugJi_*uRmhQBqAUKG-Q<Kq0`{(;&J@GT7J2fFs_+(}jmkfPs}G
S-q9t9hnYd1VRM&c9XkN~X%4pl

delta 24
fcmZ4Q)ZxVC66_Mfp~%3%7_^bAQF8Mp$y#;*RI~=j

-- 
2.7.4

[Qemu-devel] [PATCH 4/7] ipmi: Add an SMBus IPMI interface

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 default-configs/i386-softmmu.mak   |   1 +
 default-configs/x86_64-softmmu.mak |   1 +
 hw/ipmi/Makefile.objs              |   1 +
 hw/ipmi/smbus_ipmi.c               | 216 +++++++++++++++++++++++++++++++++++++
 4 files changed, 219 insertions(+)
 create mode 100644 hw/ipmi/smbus_ipmi.c

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index c94431d..636bf1d 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -14,6 +14,7 @@ CONFIG_IPMI_LOCAL=y
 CONFIG_IPMI_EXTERN=y
 CONFIG_ISA_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
+CONFIG_IPMI_SSIF=y
 CONFIG_SERIAL=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 256294d..e268980 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -14,6 +14,7 @@ CONFIG_IPMI_LOCAL=y
 CONFIG_IPMI_EXTERN=y
 CONFIG_ISA_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
+CONFIG_IPMI_SSIF=y
 CONFIG_SERIAL=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
index a90318d..6a17912 100644
--- a/hw/ipmi/Makefile.objs
+++ b/hw/ipmi/Makefile.objs
@@ -3,3 +3,4 @@ common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_sim.o
 common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_extern.o
 common-obj-$(CONFIG_ISA_IPMI_KCS) += isa_ipmi_kcs.o
 common-obj-$(CONFIG_ISA_IPMI_BT) += isa_ipmi_bt.o
+common-obj-$(CONFIG_IPMI_SSIF) += smbus_ipmi.o
diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
new file mode 100644
index 0000000..4e7203b
--- /dev/null
+++ b/hw/ipmi/smbus_ipmi.c
@@ -0,0 +1,216 @@
+/*
+ * QEMU IPMI SMBus (SSIF) emulation
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/i2c/smbus.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/ipmi/ipmi.h"
+
+#define TYPE_SMBUS_IPMI "smbus-ipmi"
+#define SMBUS_IPMI(obj) OBJECT_CHECK(SMBusIPMIDevice, (obj), TYPE_SMBUS_IPMI)
+
+#define SSIF_IPMI_REQUEST                       2
+#define SSIF_IPMI_MULTI_PART_REQUEST_START      6
+#define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE     7
+#define SSIF_IPMI_RESPONSE                      3
+#define SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE    9
+
+typedef struct SMBusIPMIDevice {
+    SMBusDevice parent;
+
+    IPMIBmc *bmc;
+
+    uint8_t outmsg[MAX_IPMI_MSG_SIZE];
+    uint32_t outpos;
+    uint32_t outlen;
+
+    uint8_t inmsg[MAX_IPMI_MSG_SIZE];
+    uint32_t inlen;
+
+    /*
+     * This is a response number that we send with the command to make
+     * sure that the response matches the command.
+     */
+    uint8_t waiting_rsp;
+
+    IPMIFwInfo fwinfo;
+} SMBusIPMIDevice;
+
+static void smbus_ipmi_handle_event(IPMIInterface *ii)
+{
+    /* No interrupts, so nothing to do here. */
+}
+
+static void smbus_ipmi_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
+                                  unsigned char *rsp, unsigned int rsp_len)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(ii);
+
+    if (sid->waiting_rsp == msg_id) {
+        sid->waiting_rsp++;
+
+        memcpy(sid->outmsg, rsp, rsp_len);
+        sid->outlen = rsp_len;
+        sid->outpos = 0;
+    }
+}
+
+static void smbus_ipmi_set_atn(IPMIInterface *ii, int val, int irq)
+{
+    /* This is where PEC would go. */
+}
+
+static void smbus_ipmi_set_irq_enable(IPMIInterface *ii, int val)
+{
+}
+
+static void ipmi_quick_cmd(SMBusDevice *dev, uint8_t read)
+{
+}
+
+static void ipmi_send_byte(SMBusDevice *dev, uint8_t val)
+{
+}
+
+static uint8_t ipmi_receive_byte(SMBusDevice *dev)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
+
+    if (sid->outpos >= sid->outlen)
+        return 0;
+
+    return sid->outmsg[sid->outpos++];
+}
+
+static void ipmi_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int len)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
+    IPMIBmcClass *bk = IPMI_BMC_GET_CLASS(sid->bmc);
+
+    if (cmd != SSIF_IPMI_REQUEST)
+        return;
+
+    if (len < 3 || len > MAX_IPMI_MSG_SIZE || buf[0] != len - 1)
+        return;
+
+    memcpy(sid->inmsg, buf + 1, len - 1);
+    sid->inlen = len;
+
+    sid->outlen = 0;
+    sid->outpos = 0;
+    bk->handle_command(sid->bmc, sid->inmsg, sid->inlen, sizeof(sid->inmsg),
+                       sid->waiting_rsp);
+}
+
+static uint8_t ipmi_read_data(SMBusDevice *dev, uint8_t cmd, int n)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
+
+    if (cmd != SSIF_IPMI_RESPONSE)
+        return 0;
+
+    if (n == 0)
+        return sid->outlen;
+
+    return ipmi_receive_byte(dev);
+}
+
+static const VMStateDescription vmstate_smbus_ipmi = {
+    .name = TYPE_SMBUS_IPMI,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(waiting_rsp, SMBusIPMIDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
+    IPMIInterface *ii = IPMI_INTERFACE(dev);
+
+    if (!sid->bmc) {
+        error_setg(errp, "IPMI device requires a bmc attribute to be set");
+        return;
+    }
+
+    sid->bmc->intf = ii;
+
+    sid->fwinfo.interface_name = "smbus";
+    sid->fwinfo.interface_type = IPMI_SMBIOS_SSIF;
+    sid->fwinfo.ipmi_spec_major_revision = 2;
+    sid->fwinfo.ipmi_spec_minor_revision = 0;
+    sid->fwinfo.i2c_slave_address = sid->bmc->slave_addr;
+    sid->fwinfo.base_address = sid->parent.i2c.address;
+    sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
+    sid->fwinfo.register_spacing = 1;
+    ipmi_add_fwinfo(&sid->fwinfo, errp);
+}
+
+static void smbus_ipmi_init(Object *obj)
+{
+    SMBusIPMIDevice *sid = SMBUS_IPMI(obj);
+
+    ipmi_bmc_find_and_link(OBJECT(obj), (Object **) &sid->bmc);
+}
+
+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);
+
+    sc->quick_cmd = ipmi_quick_cmd;
+    sc->send_byte = ipmi_send_byte;
+    sc->receive_byte = ipmi_receive_byte;
+    sc->write_data = ipmi_write_data;
+    sc->read_data = ipmi_read_data;
+    dc->vmsd = &vmstate_smbus_ipmi;
+    dc->realize = smbus_ipmi_realize;
+    iic->set_atn = smbus_ipmi_set_atn;
+    iic->handle_rsp = smbus_ipmi_handle_rsp;
+    iic->handle_if_event = smbus_ipmi_handle_event;
+    iic->set_irq_enable = smbus_ipmi_set_irq_enable;
+}
+
+static const TypeInfo smbus_ipmi_info = {
+    .name          = TYPE_SMBUS_IPMI,
+    .parent        = TYPE_SMBUS_DEVICE,
+    .instance_size = sizeof(SMBusIPMIDevice),
+    .instance_init = smbus_ipmi_init,
+    .class_init    = smbus_ipmi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_IPMI_INTERFACE },
+        { }
+    }
+};
+
+static void smbus_ipmi_register_types(void)
+{
+    type_register_static(&smbus_ipmi_info);
+}
+
+type_init(smbus_ipmi_register_types)
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/aml-build.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h | 11 +++++++++++
 2 files changed, 53 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ab89ca6..7a3874b 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1563,3 +1563,45 @@ build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets,
     build_header(linker, table_data,
                  (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
 }
+
+static Aml *aml_serial_bus_device(AmlSerialBusType type, uint8_t flags,
+                                  uint16_t type_flags,
+                                  uint8_t revid, uint16_t data_length,
+                                  uint16_t resource_source_length)
+{
+    Aml *var = aml_alloc();
+    uint16_t length = data_length + resource_source_length + 9;
+
+    build_append_byte(var->buf, 0x8e); /* Serial Bus Connection Descriptor */
+    build_append_byte(var->buf, length & 0xff);
+    build_append_byte(var->buf, length >> 8);
+    build_append_byte(var->buf, 1);    /* Revision */
+    build_append_byte(var->buf, 0);    /* Resource source index */
+    build_append_byte(var->buf, type); /* Serial bus type */
+    build_append_byte(var->buf, flags); /* Serial bus type */
+    build_append_byte(var->buf, type_flags & 0xff);
+    build_append_byte(var->buf, type_flags >> 8);
+    build_append_byte(var->buf, revid);
+    build_append_byte(var->buf, data_length & 0xff);
+    build_append_byte(var->buf, data_length >> 8);
+
+    return var;
+}
+
+Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
+                               uint16_t address, const char *resource_source)
+{
+    unsigned int resource_source_len = strlen(resource_source) + 1;
+    Aml *var = aml_serial_bus_device(aml_serial_bus_type_i2c, flags, 0, 1,
+                                     6, resource_source_len);
+
+    build_append_byte(var->buf, con_speed & 0xff);
+    build_append_byte(var->buf, (con_speed >> 8) & 0xff);
+    build_append_byte(var->buf, (con_speed >> 16) & 0xff);
+    build_append_byte(var->buf, (con_speed >> 24) & 0xff);
+    build_append_byte(var->buf, address & 0xff);
+    build_append_byte(var->buf, address >> 8);
+    g_array_append_vals(var->buf, resource_source, resource_source_len);
+
+    return var;
+}
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 2c994b3..1eb3ebd 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -206,6 +206,15 @@ struct AcpiBuildTables {
     GArray *linker;
 } AcpiBuildTables;
 
+typedef enum {
+    aml_serial_bus_type_i2c = 1,
+    aml_serial_bus_type_spi = 2,
+    aml_serial_bus_type_uart = 3
+} AmlSerialBusType;
+
+#define AML_SERIAL_BUS_FLAG_MASTER_DEVICE       (1 << 0)
+#define AML_SERIAL_BUS_FLAG_CONSUME_ONLY        (1 << 1)
+
 /**
  * init_aml_allocator:
  *
@@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
 Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
              uint8_t channel);
 Aml *aml_sleep(uint64_t msec);
+Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
+                               uint16_t address, const char *resource_source);
 
 /* Block AML object primitives */
 Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
-- 
2.7.4

[Qemu-devel] [PATCH 6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/ipmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
index 731f4ad..c187fdd 100644
--- a/hw/acpi/ipmi.c
+++ b/hw/acpi/ipmi.c
@@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
                             regspacing, info->register_length));
         break;
     case IPMI_MEMSPACE_SMBUS:
-        aml_append(crs, aml_return(aml_int(info->base_address)));
+        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
+                                                  info->base_address,
+                                                  info->acpi_parent));
         break;
     default:
         abort();
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/smbus_ipmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
index 4e7203b..3a34aaf 100644
--- a/hw/ipmi/smbus_ipmi.c
+++ b/hw/ipmi/smbus_ipmi.c
@@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
     sid->fwinfo.base_address = sid->parent.i2c.address;
     sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
     sid->fwinfo.register_spacing = 1;
+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
     ipmi_add_fwinfo(&sid->fwinfo, errp);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:46:04PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/acpi/aml-build.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h | 11 +++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ab89ca6..7a3874b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1563,3 +1563,45 @@ build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets,
>      build_header(linker, table_data,
>                   (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
>  }
> +

Pls add comment with earliest spec revision that includes this.

> +static Aml *aml_serial_bus_device(AmlSerialBusType type, uint8_t flags,
> +                                  uint16_t type_flags,
> +                                  uint8_t revid, uint16_t data_length,
> +                                  uint16_t resource_source_length)
> +{
> +    Aml *var = aml_alloc();
> +    uint16_t length = data_length + resource_source_length + 9;
> +
> +    build_append_byte(var->buf, 0x8e); /* Serial Bus Connection Descriptor */
> +    build_append_byte(var->buf, length & 0xff);
> +    build_append_byte(var->buf, length >> 8);
> +    build_append_byte(var->buf, 1);    /* Revision */
> +    build_append_byte(var->buf, 0);    /* Resource source index */
> +    build_append_byte(var->buf, type); /* Serial bus type */
> +    build_append_byte(var->buf, flags); /* Serial bus type */

Fix comments to match spec verbatim please.

> +    build_append_byte(var->buf, type_flags & 0xff);
> +    build_append_byte(var->buf, type_flags >> 8);
> +    build_append_byte(var->buf, revid);
> +    build_append_byte(var->buf, data_length & 0xff);
> +    build_append_byte(var->buf, data_length >> 8);
> +

Please use build_append_int_noprefix.

> +    return var;
> +}
> +
> +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
> +                               uint16_t address, const char *resource_source)
> +{
> +    unsigned int resource_source_len = strlen(resource_source) + 1;
> +    Aml *var = aml_serial_bus_device(aml_serial_bus_type_i2c, flags, 0, 1,
> +                                     6, resource_source_len);
> +
> +    build_append_byte(var->buf, con_speed & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 8) & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 16) & 0xff);
> +    build_append_byte(var->buf, (con_speed >> 24) & 0xff);

Please use build_append_int_noprefix.

> +    build_append_byte(var->buf, address & 0xff);
> +    build_append_byte(var->buf, address >> 8);

Same.

> +    g_array_append_vals(var->buf, resource_source, resource_source_len);

Is resource_source a name then? Then you should do
    aml_append(var, aml_name("%s", alias_object));


> +
> +    return var;
> +}
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 2c994b3..1eb3ebd 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -206,6 +206,15 @@ struct AcpiBuildTables {
>      GArray *linker;
>  } AcpiBuildTables;
>  
> +typedef enum {
> +    aml_serial_bus_type_i2c = 1,
> +    aml_serial_bus_type_spi = 2,
> +    aml_serial_bus_type_uart = 3
> +} AmlSerialBusType;
> +
> +#define AML_SERIAL_BUS_FLAG_MASTER_DEVICE       (1 << 0)
> +#define AML_SERIAL_BUS_FLAG_CONSUME_ONLY        (1 << 1)
> +

Please drop enums and add numbers with comments matching
text in ACPI spec in aml-build.c

>  /**
>   * init_aml_allocator:
>   *
> @@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>  Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
>               uint8_t channel);
>  Aml *aml_sleep(uint64_t msec);
> +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,

Drop these two parameters until they are actually useful.

> +                               uint16_t address, const char *resource_source);
>  
>  /* Block AML object primitives */
>  Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/acpi/ipmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index 731f4ad..c187fdd 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>                              regspacing, info->register_length));
>          break;
>      case IPMI_MEMSPACE_SMBUS:
> -        aml_append(crs, aml_return(aml_int(info->base_address)));
> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
> +                                                  info->base_address,
> +                                                  info->acpi_parent));

Isn't this fairly new? If so using these opcodes
is likely to break some older guests. Maybe they already don't
work, but I'd like to see some explanation about that,
and what was tested.

>          break;
>      default:
>          abort();
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/ipmi/smbus_ipmi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> index 4e7203b..3a34aaf 100644
> --- a/hw/ipmi/smbus_ipmi.c
> +++ b/hw/ipmi/smbus_ipmi.c
> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>      sid->fwinfo.base_address = sid->parent.i2c.address;
>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>      sid->fwinfo.register_spacing = 1;
> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";

I don't think it's a good idea to spread things like PCI0
outside acpi-build.c. Why do you want to pass in the path
at all?

>      ipmi_add_fwinfo(&sid->fwinfo, errp);
>  }
>  
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling

[Corey Minyard]

On 05/12/2016 02:30 AM, Michael S. Tsirkin wrote:

Thanks for the comments, all fixed.

>>   /**
>>    * init_aml_allocator:
>>    *
>> @@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>>   Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
>>                uint8_t channel);
>>   Aml *aml_sleep(uint64_t msec);
>> +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
> Drop these two parameters until they are actually useful.

I'm not sure I follow here.  The address parameter is important, it's 
for the I2C address.  The top 8 bytes are reserved 0, but not the 
bottom.  The resource source parameter tells which I2C bus it's hooked to.

>
>> +                               uint16_t address, const char *resource_source);
>>   
>>   /* Block AML object primitives */
>>   Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
>> -- 
>> 2.7.4

Re: [Qemu-devel] [PATCH 6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

[Corey Minyard]

On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/acpi/ipmi.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>> index 731f4ad..c187fdd 100644
>> --- a/hw/acpi/ipmi.c
>> +++ b/hw/acpi/ipmi.c
>> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>                               regspacing, info->register_length));
>>           break;
>>       case IPMI_MEMSPACE_SMBUS:
>> -        aml_append(crs, aml_return(aml_int(info->base_address)));
>> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
>> +                                                  info->base_address,
>> +                                                  info->acpi_parent));
> Isn't this fairly new? If so using these opcodes
> is likely to break some older guests. Maybe they already don't
> work, but I'd like to see some explanation about that,
> and what was tested.

This is new with the 5.0 specification.

I haven't done extensive testing on anything but Linux 3.10 and later.  
Well, I might have run 2.6.32, but I can't remember.  I don't have the 
ability to test Windows.

But isn't the idea of these definitions that they are ignored if the OS 
doesn't understand them?  Otherwise you could never add anything.

-corey

>>           break;
>>       default:
>>           abort();
>> -- 
>> 2.7.4

Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Corey Minyard]

On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/ipmi/smbus_ipmi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
>> index 4e7203b..3a34aaf 100644
>> --- a/hw/ipmi/smbus_ipmi.c
>> +++ b/hw/ipmi/smbus_ipmi.c
>> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>>       sid->fwinfo.base_address = sid->parent.i2c.address;
>>       sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>>       sid->fwinfo.register_spacing = 1;
>> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> I don't think it's a good idea to spread things like PCI0
> outside acpi-build.c. Why do you want to pass in the path
> at all?

You have to define the namespace for the ASL definition,
and you have to give the name in the serial bus definition.
However, thinking it through some more, this name needs
to come from the I2C device, not just hard-coded here.

-corey

>>       ipmi_add_fwinfo(&sid->fwinfo, errp);
>>   }
>>   
>> -- 
>> 2.7.4

Re: [Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 08:26:57AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:30 AM, Michael S. Tsirkin wrote:
> 
> Thanks for the comments, all fixed.
> 
> >>  /**
> >>   * init_aml_allocator:
> >>   *
> >>@@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
> >>  Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
> >>               uint8_t channel);
> >>  Aml *aml_sleep(uint64_t msec);
> >>+Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed,
> >Drop these two parameters until they are actually useful.
> 
> I'm not sure I follow here.  The address parameter is important, it's for
> the I2C address.  The top 8 bytes are reserved 0, but not the bottom.  The
> resource source parameter tells which I2C bus it's hooked to.

I mean flags and con_speed.

> >
> >>+                               uint16_t address, const char *resource_source);
> >>  /* Block AML object primitives */
> >>  Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
> >>-- 
> >>2.7.4

Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> >On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> >>From: Corey Minyard <cminyard@mvista.com>
> >>
> >>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>---
> >>  hw/ipmi/smbus_ipmi.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >>diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> >>index 4e7203b..3a34aaf 100644
> >>--- a/hw/ipmi/smbus_ipmi.c
> >>+++ b/hw/ipmi/smbus_ipmi.c
> >>@@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
> >>      sid->fwinfo.base_address = sid->parent.i2c.address;
> >>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
> >>      sid->fwinfo.register_spacing = 1;
> >>+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> >I don't think it's a good idea to spread things like PCI0
> >outside acpi-build.c. Why do you want to pass in the path
> >at all?
> 
> You have to define the namespace for the ASL definition,
> and you have to give the name in the serial bus definition.
> However, thinking it through some more, this name needs
> to come from the I2C device, not just hard-coded here.
> 
> -corey

For now you can put it as PCI0 within aml-build.c.

Also do we need \\_SB.PCI0? Why not just create the
device within PCI0 scope?

> >>      ipmi_add_fwinfo(&sid->fwinfo, errp);
> >>  }
> >>-- 
> >>2.7.4

Re: [Qemu-devel] [PATCH 6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 08:29:53AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
> >On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
> >>From: Corey Minyard <cminyard@mvista.com>
> >>
> >>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>---
> >>  hw/acpi/ipmi.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> >>index 731f4ad..c187fdd 100644
> >>--- a/hw/acpi/ipmi.c
> >>+++ b/hw/acpi/ipmi.c
> >>@@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
> >>                              regspacing, info->register_length));
> >>          break;
> >>      case IPMI_MEMSPACE_SMBUS:
> >>-        aml_append(crs, aml_return(aml_int(info->base_address)));
> >>+        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
> >>+                                                  info->base_address,
> >>+                                                  info->acpi_parent));
> >Isn't this fairly new? If so using these opcodes
> >is likely to break some older guests. Maybe they already don't
> >work, but I'd like to see some explanation about that,
> >and what was tested.
> 
> This is new with the 5.0 specification.
> 
> I haven't done extensive testing on anything but Linux 3.10 and later.
> Well, I might have run 2.6.32, but I can't remember.  I don't have the
> ability to test Windows.
> 
> But isn't the idea of these definitions that they are ignored if the OS
> doesn't understand them?

Not always. It depends, spec does not require it.

You can check which revision does OSPM support but
you have to decide what to do for an old revision then.

>  Otherwise you could never add anything.
> 
> -corey

Question is, what happened before this change?


> >>          break;
> >>      default:
> >>          abort();
> >>-- 
> >>2.7.4

Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Corey Minyard]

On 05/12/2016 08:35 AM, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
>> On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>   hw/ipmi/smbus_ipmi.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
>>>> index 4e7203b..3a34aaf 100644
>>>> --- a/hw/ipmi/smbus_ipmi.c
>>>> +++ b/hw/ipmi/smbus_ipmi.c
>>>> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>>>>       sid->fwinfo.base_address = sid->parent.i2c.address;
>>>>       sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>>>>       sid->fwinfo.register_spacing = 1;
>>>> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
>>> I don't think it's a good idea to spread things like PCI0
>>> outside acpi-build.c. Why do you want to pass in the path
>>> at all?
>> You have to define the namespace for the ASL definition,
>> and you have to give the name in the serial bus definition.
>> However, thinking it through some more, this name needs
>> to come from the I2C device, not just hard-coded here.
>>
>> -corey
> For now you can put it as PCI0 within aml-build.c.
>
> Also do we need \\_SB.PCI0? Why not just create the
> device within PCI0 scope?
I'm not sure I follow here. \SB.PCI0.SMB0 is the proper
scope to identify which SMBus the device is on.  This
is how the ISA devices are added, for instance.

What I've done now to fix this is added an ACPI namespace
to the I2C bus structure and stored the value for the I2C
bus there in the i386 code.

Maybe it should be in BusState?  That way the ISA IPMI code
could pull it from the bus, too.

Putting PCI0 into aml-build.c (or hw/acpi/ipmi.c, really) seems
like a violation of scope.

-corey

>>>>       ipmi_add_fwinfo(&sid->fwinfo, errp);
>>>>   }
>>>> -- 
>>>> 2.7.4

Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 02:20:25PM -0500, Corey Minyard wrote:
> On 05/12/2016 08:35 AM, Michael S. Tsirkin wrote:
> >On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
> >>On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> >>>On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> >>>>From: Corey Minyard <cminyard@mvista.com>
> >>>>
> >>>>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>>>---
> >>>>  hw/ipmi/smbus_ipmi.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>>diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> >>>>index 4e7203b..3a34aaf 100644
> >>>>--- a/hw/ipmi/smbus_ipmi.c
> >>>>+++ b/hw/ipmi/smbus_ipmi.c
> >>>>@@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
> >>>>      sid->fwinfo.base_address = sid->parent.i2c.address;
> >>>>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
> >>>>      sid->fwinfo.register_spacing = 1;
> >>>>+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> >>>I don't think it's a good idea to spread things like PCI0
> >>>outside acpi-build.c. Why do you want to pass in the path
> >>>at all?
> >>You have to define the namespace for the ASL definition,
> >>and you have to give the name in the serial bus definition.
> >>However, thinking it through some more, this name needs
> >>to come from the I2C device, not just hard-coded here.
> >>
> >>-corey
> >For now you can put it as PCI0 within aml-build.c.
> >
> >Also do we need \\_SB.PCI0? Why not just create the
> >device within PCI0 scope?
> I'm not sure I follow here. \SB.PCI0.SMB0 is the proper
> scope to identify which SMBus the device is on.  This
> is how the ISA devices are added, for instance.

We add most of them within PCI0 scope instead.
E.g.

Device (PCI0) {
	Device (SMB0) {
	}
}

this way device does not need to know where it is.


> What I've done now to fix this is added an ACPI namespace
> to the I2C bus structure and stored the value for the I2C
> bus there in the i386 code.
> 
> Maybe it should be in BusState?  That way the ISA IPMI code
> could pull it from the bus, too.
> 
> Putting PCI0 into aml-build.c (or hw/acpi/ipmi.c, really) seems
> like a violation of scope.
> 
> -corey

Generally we scan each bus and list devices found there.


> >>>>      ipmi_add_fwinfo(&sid->fwinfo, errp);
> >>>>  }
> >>>>-- 
> >>>>2.7.4

Re: [Qemu-devel] [PATCH 6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

[Corey Minyard]

On 05/12/2016 08:39 AM, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 08:29:53AM -0500, Corey Minyard wrote:
>> On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>   hw/acpi/ipmi.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>>>> index 731f4ad..c187fdd 100644
>>>> --- a/hw/acpi/ipmi.c
>>>> +++ b/hw/acpi/ipmi.c
>>>> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>>>                               regspacing, info->register_length));
>>>>           break;
>>>>       case IPMI_MEMSPACE_SMBUS:
>>>> -        aml_append(crs, aml_return(aml_int(info->base_address)));
>>>> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
>>>> +                                                  info->base_address,
>>>> +                                                  info->acpi_parent));
>>> Isn't this fairly new? If so using these opcodes
>>> is likely to break some older guests. Maybe they already don't
>>> work, but I'd like to see some explanation about that,
>>> and what was tested.
>> This is new with the 5.0 specification.
>>
>> I haven't done extensive testing on anything but Linux 3.10 and later.
>> Well, I might have run 2.6.32, but I can't remember.  I don't have the
>> ability to test Windows.
>>
>> But isn't the idea of these definitions that they are ignored if the OS
>> doesn't understand them?
> Not always. It depends, spec does not require it.
>
> You can check which revision does OSPM support but
> you have to decide what to do for an old revision then.
>
>>   Otherwise you could never add anything.
>>
>> -corey
> Question is, what happened before this change?

I could add a "noacpi" option for the device, that way the user could
accommodate an OS that couldn't handle the option.

I also realized that the addition of the SMBus ACPI device required
backwards compatibility so it wouldn't be there for previous machine
versions.  So you could specify a 2.6 machine and not get the ACPI
entry.

-corey