[Qemu-devel] [PATCH v1 0/3] Update AppleSMC for OS X Sierra 10.12.4 guests

[Qemu-devel] [PATCH v1 0/3] Update AppleSMC for OS X Sierra 10.12.4 guests

[Gabriel L. Somlo]

As of 10.12.4 (currently the latest Sierra update), OS X refuses to boot
unless the AppleSMC supports a third I/O port, which provides the current
error status when read.

This series consists of three patches:

	- 1/3: indentation/whitespace cleanup for applesmc.c to the point
		where it now passes scripts/checkpatc.pl, and allows
		subsequent changes to look nice in diff-patch format :)

	- 2/3: consolidate Port I/O into a single region, and invoke
		appropriate read/write methods based on the offset being
		accessed

	- 3/3: implement read-only error/status port, and update
		data and command read/write methods to correctly
		maintain the state machine for keeping the status_1e
		value up to date.

Thanks,
  --Gabriel

Gabriel L. Somlo (3):
  applesmc: cosmetic whitespace and indentation cleanup
  applesmc: consolidate port i/o into single contiguous region
  applesmc: implement error status port

 hw/misc/applesmc.c | 254 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 167 insertions(+), 87 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v1 1/3] applesmc: cosmetic whitespace and indentation cleanup

[Gabriel L. Somlo]

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 hw/misc/applesmc.c | 100 +++++++++++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 49 deletions(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 77fab5b..986f2ac 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -39,24 +39,27 @@
 /* #define DEBUG_SMC */
 
 #define APPLESMC_DEFAULT_IOBASE        0x300
-/* data port used by Apple SMC */
-#define APPLESMC_DATA_PORT             0x0
-/* command/status port used by Apple SMC */
-#define APPLESMC_CMD_PORT              0x4
-#define APPLESMC_NR_PORTS              32
 
-#define APPLESMC_READ_CMD              0x10
-#define APPLESMC_WRITE_CMD             0x11
-#define APPLESMC_GET_KEY_BY_INDEX_CMD  0x12
-#define APPLESMC_GET_KEY_TYPE_CMD      0x13
+enum {
+    APPLESMC_DATA_PORT               = 0x00,
+    APPLESMC_CMD_PORT                = 0x04,
+    APPLESMC_NUM_PORTS               = 0x20,
+};
+
+enum {
+    APPLESMC_READ_CMD                = 0x10,
+    APPLESMC_WRITE_CMD               = 0x11,
+    APPLESMC_GET_KEY_BY_INDEX_CMD    = 0x12,
+    APPLESMC_GET_KEY_TYPE_CMD        = 0x13,
+};
 
 #ifdef DEBUG_SMC
 #define smc_debug(...) fprintf(stderr, "AppleSMC: " __VA_ARGS__)
 #else
-#define smc_debug(...) do { } while(0)
+#define smc_debug(...) do { } while (0)
 #endif
 
-static char default_osk[64] = "This is a dummy key. Enter the real key "
+static char default_osk[65] = "This is a dummy key. Enter the real key "
                               "using the -osk parameter";
 
 struct AppleSMCData {
@@ -77,12 +80,11 @@ struct AppleSMCState {
     uint32_t iobase;
     uint8_t cmd;
     uint8_t status;
-    uint8_t key[4];
+    char key[4];
     uint8_t read_pos;
     uint8_t data_len;
     uint8_t data_pos;
     uint8_t data[255];
-    uint8_t charactic[4];
     char *osk;
     QLIST_HEAD(, AppleSMCData) data_def;
 };
@@ -93,10 +95,10 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
     AppleSMCState *s = opaque;
 
     smc_debug("CMD Write B: %#x = %#x\n", addr, val);
-    switch(val) {
-        case APPLESMC_READ_CMD:
-            s->status = 0x0c;
-            break;
+    switch (val) {
+    case APPLESMC_READ_CMD:
+        s->status = 0x0c;
+        break;
     }
     s->cmd = val;
     s->read_pos = 0;
@@ -123,54 +125,54 @@ static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
     AppleSMCState *s = opaque;
 
     smc_debug("DATA Write B: %#x = %#x\n", addr, val);
-    switch(s->cmd) {
-        case APPLESMC_READ_CMD:
-            if(s->read_pos < 4) {
-                s->key[s->read_pos] = val;
-                s->status = 0x04;
-            } else if(s->read_pos == 4) {
-                s->data_len = val;
-                s->status = 0x05;
-                s->data_pos = 0;
-                smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
-                          s->key[1], s->key[2], s->key[3], val);
-                applesmc_fill_data(s);
-            }
-            s->read_pos++;
-            break;
+    switch (s->cmd) {
+    case APPLESMC_READ_CMD:
+        if (s->read_pos < 4) {
+            s->key[s->read_pos] = val;
+            s->status = 0x04;
+        } else if (s->read_pos == 4) {
+            s->data_len = val;
+            s->status = 0x05;
+            s->data_pos = 0;
+            smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
+                      s->key[1], s->key[2], s->key[3], val);
+            applesmc_fill_data(s);
+        }
+        s->read_pos++;
+        break;
     }
 }
 
-static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr1,
-                                      unsigned size)
+static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr, unsigned size)
 {
     AppleSMCState *s = opaque;
     uint8_t retval = 0;
 
-    switch(s->cmd) {
-        case APPLESMC_READ_CMD:
-            if(s->data_pos < s->data_len) {
-                retval = s->data[s->data_pos];
-                smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
-                          retval);
-                s->data_pos++;
-                if(s->data_pos == s->data_len) {
-                    s->status = 0x00;
-                    smc_debug("EOF\n");
-                } else
-                    s->status = 0x05;
+    switch (s->cmd) {
+    case APPLESMC_READ_CMD:
+        if (s->data_pos < s->data_len) {
+            retval = s->data[s->data_pos];
+            smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
+                      retval);
+            s->data_pos++;
+            if (s->data_pos == s->data_len) {
+                s->status = 0x00;
+                smc_debug("EOF\n");
+            } else {
+                s->status = 0x05;
             }
+        }
     }
-    smc_debug("DATA Read b: %#x = %#x\n", addr1, retval);
+    smc_debug("DATA Read b: %#x = %#x\n", addr, retval);
 
     return retval;
 }
 
-static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr1, unsigned size)
+static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
 {
     AppleSMCState *s = opaque;
 
-    smc_debug("CMD Read B: %#x\n", addr1);
+    smc_debug("CMD Read B: %#x\n", addr);
     return s->status;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Gabriel L. Somlo]

Newer AppleSMC revisions support an error status (read) port
in addition to the data and command ports currently supported.

Register the full 32-bit region at once, and handle individual
ports at various offsets within the region from the top-level
applesmc_io_[write|read]() methods (ctual support for reading
the error status port to be added by a subsequent patch).

Originally proposed by Eric Shelton <eshelton@pobox.com>

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 hw/misc/applesmc.c | 56 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 986f2ac..e581e02 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -75,8 +75,7 @@ typedef struct AppleSMCState AppleSMCState;
 struct AppleSMCState {
     ISADevice parent_obj;
 
-    MemoryRegion io_data;
-    MemoryRegion io_cmd;
+    MemoryRegion io_reg;
     uint32_t iobase;
     uint8_t cmd;
     uint8_t status;
@@ -207,19 +206,36 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)
     applesmc_add_key(s, "MSSD", 1, "\0x3");
 }
 
-static const MemoryRegionOps applesmc_data_io_ops = {
-    .write = applesmc_io_data_write,
-    .read = applesmc_io_data_read,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
+static void applesmc_io_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    switch (addr) {
+    case APPLESMC_DATA_PORT:
+        applesmc_io_data_write(opaque, addr, val, size);
+        break;
+    case APPLESMC_CMD_PORT:
+        applesmc_io_cmd_write(opaque, addr, val, size);
+        break;
+    default:
+        break;
+    }
+}
 
-static const MemoryRegionOps applesmc_cmd_io_ops = {
-    .write = applesmc_io_cmd_write,
-    .read = applesmc_io_cmd_read,
+static uint64_t applesmc_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    switch (addr) {
+    case APPLESMC_DATA_PORT:
+        return applesmc_io_data_read(opaque, addr, size);
+    case APPLESMC_CMD_PORT:
+        return applesmc_io_cmd_read(opaque, addr, size);
+    default:
+        return 0xff;
+    }
+}
+
+static const MemoryRegionOps applesmc_io_ops = {
+    .write = applesmc_io_write,
+    .read = applesmc_io_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -231,15 +247,9 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
 {
     AppleSMCState *s = APPLE_SMC(dev);
 
-    memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
-                          "applesmc-data", 4);
-    isa_register_ioport(&s->parent_obj, &s->io_data,
-                        s->iobase + APPLESMC_DATA_PORT);
-
-    memory_region_init_io(&s->io_cmd, OBJECT(s), &applesmc_cmd_io_ops, s,
-                          "applesmc-cmd", 4);
-    isa_register_ioport(&s->parent_obj, &s->io_cmd,
-                        s->iobase + APPLESMC_CMD_PORT);
+    memory_region_init_io(&s->io_reg, OBJECT(s), &applesmc_io_ops, s,
+                          "applesmc", APPLESMC_NUM_PORTS);
+    isa_register_ioport(&s->parent_obj, &s->io_reg, s->iobase);
 
     if (!s->osk || (strlen(s->osk) != 64)) {
         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");
-- 
2.7.4

[Qemu-devel] [PATCH v1 3/3] applesmc: implement error status port

[Gabriel L. Somlo]

As of release 10.12.4, OS X (Sierra) refuses to boot unless the
AppleSMC supports an additional I/O port, expected to provide an
error status code.

Update the [cmd|data]_write() and data_read() methods to implement
the required state machine, and add an err_read() method to provide
the error status code to the guest.

Originally proposed by Eric Shelton <eshelton@pobox.com>

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 hw/misc/applesmc.c | 120 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 26 deletions(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index e581e02..eac0659 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -43,6 +43,7 @@
 enum {
     APPLESMC_DATA_PORT               = 0x00,
     APPLESMC_CMD_PORT                = 0x04,
+    APPLESMC_ERR_PORT                = 0x1e,
     APPLESMC_NUM_PORTS               = 0x20,
 };
 
@@ -53,6 +54,24 @@ enum {
     APPLESMC_GET_KEY_TYPE_CMD        = 0x13,
 };
 
+enum {
+    APPLESMC_ST_CMD_DONE             = 0x00,
+    APPLESMC_ST_DATA_READY           = 0x01,
+    APPLESMC_ST_BUSY                 = 0x02,
+    APPLESMC_ST_ACK                  = 0x04,
+    APPLESMC_ST_NEW_CMD              = 0x08,
+};
+
+enum {
+    APPLESMC_ST_1E_CMD_INTRUPTED     = 0x80,
+    APPLESMC_ST_1E_STILL_BAD_CMD     = 0x81,
+    APPLESMC_ST_1E_BAD_CMD           = 0x82,
+    APPLESMC_ST_1E_NOEXIST           = 0x84,
+    APPLESMC_ST_1E_WRITEONLY         = 0x85,
+    APPLESMC_ST_1E_READONLY          = 0x86,
+    APPLESMC_ST_1E_BAD_INDEX         = 0xb8,
+};
+
 #ifdef DEBUG_SMC
 #define smc_debug(...) fprintf(stderr, "AppleSMC: " __VA_ARGS__)
 #else
@@ -79,6 +98,8 @@ struct AppleSMCState {
     uint32_t iobase;
     uint8_t cmd;
     uint8_t status;
+    uint8_t status_1e;
+    uint8_t last_ret;
     char key[4];
     uint8_t read_pos;
     uint8_t data_len;
@@ -92,79 +113,112 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
                                   unsigned size)
 {
     AppleSMCState *s = opaque;
+    uint8_t status = s->status & 0x0f;
 
-    smc_debug("CMD Write B: %#x = %#x\n", addr, val);
+    smc_debug("CMD received: 0x%02x\n", (uint8_t)val);
     switch (val) {
     case APPLESMC_READ_CMD:
-        s->status = 0x0c;
+        /* did last command run through OK? */
+        if (status == APPLESMC_ST_CMD_DONE || status == APPLESMC_ST_NEW_CMD) {
+            s->cmd = val;
+            s->status = APPLESMC_ST_NEW_CMD | APPLESMC_ST_ACK;
+        } else {
+            smc_debug("ERROR: previous command interrupted!\n");
+            s->status = APPLESMC_ST_NEW_CMD;
+            s->status_1e = APPLESMC_ST_1E_CMD_INTRUPTED;
+        }
         break;
+    default:
+        smc_debug("UNEXPECTED CMD 0x%02x\n", (uint8_t)val);
+        s->status = APPLESMC_ST_NEW_CMD;
+        s->status_1e = APPLESMC_ST_1E_BAD_CMD;
     }
-    s->cmd = val;
     s->read_pos = 0;
     s->data_pos = 0;
 }
 
-static void applesmc_fill_data(AppleSMCState *s)
+static struct AppleSMCData *applesmc_find_key(AppleSMCState *s, const char *key)
 {
     struct AppleSMCData *d;
 
     QLIST_FOREACH(d, &s->data_def, node) {
-        if (!memcmp(d->key, s->key, 4)) {
-            smc_debug("Key matched (%s Len=%d Data=%s)\n", d->key,
-                      d->len, d->data);
-            memcpy(s->data, d->data, d->len);
-            return;
+        if (!memcmp(d->key, key, 4)) {
+            return d;
         }
     }
+    return NULL;
 }
 
 static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
                                    unsigned size)
 {
     AppleSMCState *s = opaque;
+    struct AppleSMCData *d;
 
-    smc_debug("DATA Write B: %#x = %#x\n", addr, val);
+    smc_debug("DATA received: 0x%02x\n", (uint8_t)val);
     switch (s->cmd) {
     case APPLESMC_READ_CMD:
+        if ((s->status & 0x0f) == APPLESMC_ST_CMD_DONE) {
+            break;
+        }
         if (s->read_pos < 4) {
             s->key[s->read_pos] = val;
-            s->status = 0x04;
+            s->status = APPLESMC_ST_ACK;
         } else if (s->read_pos == 4) {
-            s->data_len = val;
-            s->status = 0x05;
-            s->data_pos = 0;
-            smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
-                      s->key[1], s->key[2], s->key[3], val);
-            applesmc_fill_data(s);
+            d = applesmc_find_key(s, s->key);
+            if (d != NULL) {
+                memcpy(s->data, d->data, d->len);
+                s->data_len = d->len;
+                s->data_pos = 0;
+                s->status = APPLESMC_ST_ACK | APPLESMC_ST_DATA_READY;
+                s->status_1e = APPLESMC_ST_CMD_DONE;  /* clear on valid key */
+            } else {
+                smc_debug("READ_CMD: key '%c%c%c%c' not found!\n",
+                          s->key[0], s->key[1], s->key[2], s->key[3]);
+                s->status = APPLESMC_ST_CMD_DONE;
+                s->status_1e = APPLESMC_ST_1E_NOEXIST;
+            }
         }
         s->read_pos++;
         break;
+    default:
+        s->status = APPLESMC_ST_CMD_DONE;
+        s->status_1e = APPLESMC_ST_1E_STILL_BAD_CMD;
     }
 }
 
 static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr, unsigned size)
 {
     AppleSMCState *s = opaque;
-    uint8_t retval = 0;
 
     switch (s->cmd) {
     case APPLESMC_READ_CMD:
+        if (!(s->status & APPLESMC_ST_DATA_READY)) {
+            break;
+        }
         if (s->data_pos < s->data_len) {
-            retval = s->data[s->data_pos];
-            smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
-                      retval);
+            s->last_ret = s->data[s->data_pos];
+            smc_debug("READ '%c%c%c%c'[%d] = %0x%02x\n",
+                      s->key[0], s->key[1], s->key[2], s->key[3],
+                      s->data_pos, s->last_ret);
             s->data_pos++;
             if (s->data_pos == s->data_len) {
-                s->status = 0x00;
-                smc_debug("EOF\n");
+                s->status = APPLESMC_ST_CMD_DONE;
+                smc_debug("READ '%c%c%c%c' Len=%d complete!\n",
+                          s->key[0], s->key[1], s->key[2], s->key[3],
+                          s->data_len);
             } else {
-                s->status = 0x05;
+                s->status = APPLESMC_ST_ACK | APPLESMC_ST_DATA_READY;
             }
         }
+        break;
+    default:
+        s->status = APPLESMC_ST_CMD_DONE;
+        s->status_1e = APPLESMC_ST_1E_STILL_BAD_CMD;
     }
-    smc_debug("DATA Read b: %#x = %#x\n", addr, retval);
+    smc_debug("DATA sent: 0x%02x\n", s->last_ret);
 
-    return retval;
+    return s->last_ret;
 }
 
 static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
@@ -175,6 +229,15 @@ static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
     return s->status;
 }
 
+static uint64_t applesmc_io_err_read(void *opaque, hwaddr addr, unsigned size)
+{
+    AppleSMCState *s = opaque;
+
+    /* NOTE: read does not clear the 1e status */
+    smc_debug("ERR_CODE sent: 0x%02x\n", s->status_1e);
+    return s->status_1e;
+}
+
 static void applesmc_add_key(AppleSMCState *s, const char *key,
                              int len, const char *data)
 {
@@ -197,6 +260,9 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)
     QLIST_FOREACH_SAFE(d, &s->data_def, node, next) {
         QLIST_REMOVE(d, node);
     }
+    s->status = 0x00;
+    s->status_1e = 0x00;
+    s->last_ret = 0x00;
 
     applesmc_add_key(s, "REV ", 6, "\x01\x13\x0f\x00\x00\x03");
     applesmc_add_key(s, "OSK0", 32, s->osk);
@@ -228,6 +294,8 @@ static uint64_t applesmc_io_read(void *opaque, hwaddr addr, unsigned size)
         return applesmc_io_data_read(opaque, addr, size);
     case APPLESMC_CMD_PORT:
         return applesmc_io_cmd_read(opaque, addr, size);
+    case APPLESMC_ERR_PORT:
+        return applesmc_io_err_read(opaque, addr, size);
     default:
         return 0xff;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 1/3] applesmc: cosmetic whitespace and indentation cleanup

[Alexander Graf]

On 03/31/2017 06:48 PM, Gabriel L. Somlo wrote:
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Alexander Graf]

On 03/31/2017 06:48 PM, Gabriel L. Somlo wrote:
> Newer AppleSMC revisions support an error status (read) port
> in addition to the data and command ports currently supported.
>
> Register the full 32-bit region at once, and handle individual
> ports at various offsets within the region from the top-level
> applesmc_io_[write|read]() methods (ctual support for reading
> the error status port to be added by a subsequent patch).
>
> Originally proposed by Eric Shelton <eshelton@pobox.com>
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

I would prefer if we could leave the multiplexing to the layer that 
knows how to do that the best: Memory Regions.

Why don't you just define a big region that ecompasses all of the IO 
space (with fallback I/O handlers for warnings) and then just sprinkle 
the ones we handle on top with higher prio?


Alex

Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Paolo Bonzini]

On 03/04/2017 11:32, Alexander Graf wrote:
> 
>> Newer AppleSMC revisions support an error status (read) port
>> in addition to the data and command ports currently supported.
>>
>> Register the full 32-bit region at once, and handle individual
>> ports at various offsets within the region from the top-level
>> applesmc_io_[write|read]() methods (ctual support for reading
>> the error status port to be added by a subsequent patch).
>>
>> Originally proposed by Eric Shelton <eshelton@pobox.com>
>>
>> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> 
> I would prefer if we could leave the multiplexing to the layer that
> knows how to do that the best: Memory Regions.
> 
> Why don't you just define a big region that ecompasses all of the IO
> space (with fallback I/O handlers for warnings) and then just sprinkle
> the ones we handle on top with higher prio?

You don't need priority at all, "contained" regions always win over the
container (docs/memory.txt just before "Region names").

Both what you propose and what Gabriel did makes sense.  In general QEMU
does things more like in this patch, but there are exceptions (e.g. ACPI).

Paolo

Re: [Qemu-devel] [PATCH v1 1/3] applesmc: cosmetic whitespace and indentation cleanup

[Philippe Mathieu-Daudé]

Hi Gabriel,

On 03/31/2017 01:48 PM, Gabriel L. Somlo wrote:
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  hw/misc/applesmc.c | 100 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 51 insertions(+), 49 deletions(-)
>
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 77fab5b..986f2ac 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -39,24 +39,27 @@
>  /* #define DEBUG_SMC */
>
>  #define APPLESMC_DEFAULT_IOBASE        0x300
> -/* data port used by Apple SMC */
> -#define APPLESMC_DATA_PORT             0x0
> -/* command/status port used by Apple SMC */
> -#define APPLESMC_CMD_PORT              0x4
> -#define APPLESMC_NR_PORTS              32
>
> -#define APPLESMC_READ_CMD              0x10
> -#define APPLESMC_WRITE_CMD             0x11
> -#define APPLESMC_GET_KEY_BY_INDEX_CMD  0x12
> -#define APPLESMC_GET_KEY_TYPE_CMD      0x13
> +enum {
> +    APPLESMC_DATA_PORT               = 0x00,
> +    APPLESMC_CMD_PORT                = 0x04,
> +    APPLESMC_NUM_PORTS               = 0x20,
> +};
> +
> +enum {
> +    APPLESMC_READ_CMD                = 0x10,
> +    APPLESMC_WRITE_CMD               = 0x11,
> +    APPLESMC_GET_KEY_BY_INDEX_CMD    = 0x12,
> +    APPLESMC_GET_KEY_TYPE_CMD        = 0x13,
> +};
>
>  #ifdef DEBUG_SMC
>  #define smc_debug(...) fprintf(stderr, "AppleSMC: " __VA_ARGS__)
>  #else
> -#define smc_debug(...) do { } while(0)
> +#define smc_debug(...) do { } while (0)
>  #endif
>
> -static char default_osk[64] = "This is a dummy key. Enter the real key "
> +static char default_osk[65] = "This is a dummy key. Enter the real key "

Here is more than a cosmetic change.

Since this array is initialized you can declare it without specify the size:
static char default_osk[] = ...

OSK0 + OSK1 is 64, why need an extra byte?

Can yoy add some self-explanatory #define and use them later in this file:

     applesmc_add_key(s, "OSK0", 32, s->osk);
     applesmc_add_key(s, "OSK1", 32, s->osk + 32);

and

     if (!s->osk || (strlen(s->osk) != 64)) {
         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");


>                                "using the -osk parameter";
>
>  struct AppleSMCData {
> @@ -77,12 +80,11 @@ struct AppleSMCState {
>      uint32_t iobase;
>      uint8_t cmd;
>      uint8_t status;
> -    uint8_t key[4];
> +    char key[4];
>      uint8_t read_pos;
>      uint8_t data_len;
>      uint8_t data_pos;
>      uint8_t data[255];
> -    uint8_t charactic[4];
>      char *osk;
>      QLIST_HEAD(, AppleSMCData) data_def;
>  };
> @@ -93,10 +95,10 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
>      AppleSMCState *s = opaque;
>
>      smc_debug("CMD Write B: %#x = %#x\n", addr, val);
> -    switch(val) {
> -        case APPLESMC_READ_CMD:
> -            s->status = 0x0c;
> -            break;
> +    switch (val) {
> +    case APPLESMC_READ_CMD:
> +        s->status = 0x0c;
> +        break;
>      }
>      s->cmd = val;
>      s->read_pos = 0;
> @@ -123,54 +125,54 @@ static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
>      AppleSMCState *s = opaque;
>
>      smc_debug("DATA Write B: %#x = %#x\n", addr, val);
> -    switch(s->cmd) {
> -        case APPLESMC_READ_CMD:
> -            if(s->read_pos < 4) {
> -                s->key[s->read_pos] = val;
> -                s->status = 0x04;
> -            } else if(s->read_pos == 4) {
> -                s->data_len = val;
> -                s->status = 0x05;
> -                s->data_pos = 0;
> -                smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
> -                          s->key[1], s->key[2], s->key[3], val);
> -                applesmc_fill_data(s);
> -            }
> -            s->read_pos++;
> -            break;
> +    switch (s->cmd) {
> +    case APPLESMC_READ_CMD:
> +        if (s->read_pos < 4) {
> +            s->key[s->read_pos] = val;
> +            s->status = 0x04;
> +        } else if (s->read_pos == 4) {
> +            s->data_len = val;
> +            s->status = 0x05;
> +            s->data_pos = 0;
> +            smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
> +                      s->key[1], s->key[2], s->key[3], val);
> +            applesmc_fill_data(s);
> +        }
> +        s->read_pos++;
> +        break;
>      }
>  }
>
> -static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr1,
> -                                      unsigned size)
> +static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      AppleSMCState *s = opaque;
>      uint8_t retval = 0;
>
> -    switch(s->cmd) {
> -        case APPLESMC_READ_CMD:
> -            if(s->data_pos < s->data_len) {
> -                retval = s->data[s->data_pos];
> -                smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
> -                          retval);
> -                s->data_pos++;
> -                if(s->data_pos == s->data_len) {
> -                    s->status = 0x00;
> -                    smc_debug("EOF\n");
> -                } else
> -                    s->status = 0x05;
> +    switch (s->cmd) {
> +    case APPLESMC_READ_CMD:
> +        if (s->data_pos < s->data_len) {
> +            retval = s->data[s->data_pos];
> +            smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
> +                      retval);
> +            s->data_pos++;
> +            if (s->data_pos == s->data_len) {
> +                s->status = 0x00;
> +                smc_debug("EOF\n");
> +            } else {
> +                s->status = 0x05;
>              }
> +        }
>      }
> -    smc_debug("DATA Read b: %#x = %#x\n", addr1, retval);
> +    smc_debug("DATA Read b: %#x = %#x\n", addr, retval);
>
>      return retval;
>  }
>
> -static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr1, unsigned size)
> +static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      AppleSMCState *s = opaque;
>
> -    smc_debug("CMD Read B: %#x\n", addr1);
> +    smc_debug("CMD Read B: %#x\n", addr);
>      return s->status;
>  }
>
>

Re: [Qemu-devel] [PATCH v1 1/3] applesmc: cosmetic whitespace and indentation cleanup

[Gabriel L. Somlo]

On Mon, Apr 03, 2017 at 10:34:09AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Gabriel,
> 
> On 03/31/2017 01:48 PM, Gabriel L. Somlo wrote:
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > ---
> >  hw/misc/applesmc.c | 100 +++++++++++++++++++++++++++--------------------------
> >  1 file changed, 51 insertions(+), 49 deletions(-)
> > 
> > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> > index 77fab5b..986f2ac 100644
> > --- a/hw/misc/applesmc.c
> > +++ b/hw/misc/applesmc.c
> > @@ -39,24 +39,27 @@
> >  /* #define DEBUG_SMC */
> > 
> >  #define APPLESMC_DEFAULT_IOBASE        0x300
> > -/* data port used by Apple SMC */
> > -#define APPLESMC_DATA_PORT             0x0
> > -/* command/status port used by Apple SMC */
> > -#define APPLESMC_CMD_PORT              0x4
> > -#define APPLESMC_NR_PORTS              32
> > 
> > -#define APPLESMC_READ_CMD              0x10
> > -#define APPLESMC_WRITE_CMD             0x11
> > -#define APPLESMC_GET_KEY_BY_INDEX_CMD  0x12
> > -#define APPLESMC_GET_KEY_TYPE_CMD      0x13
> > +enum {
> > +    APPLESMC_DATA_PORT               = 0x00,
> > +    APPLESMC_CMD_PORT                = 0x04,
> > +    APPLESMC_NUM_PORTS               = 0x20,
> > +};
> > +
> > +enum {
> > +    APPLESMC_READ_CMD                = 0x10,
> > +    APPLESMC_WRITE_CMD               = 0x11,
> > +    APPLESMC_GET_KEY_BY_INDEX_CMD    = 0x12,
> > +    APPLESMC_GET_KEY_TYPE_CMD        = 0x13,
> > +};
> > 
> >  #ifdef DEBUG_SMC
> >  #define smc_debug(...) fprintf(stderr, "AppleSMC: " __VA_ARGS__)
> >  #else
> > -#define smc_debug(...) do { } while(0)
> > +#define smc_debug(...) do { } while (0)
> >  #endif
> > 
> > -static char default_osk[64] = "This is a dummy key. Enter the real key "
> > +static char default_osk[65] = "This is a dummy key. Enter the real key "
> 
> Here is more than a cosmetic change.

You're right, that was my mistake (should have left it 64.
> 
> Since this array is initialized you can declare it without specify the size:
> static char default_osk[] = ...

It's initialized to something that's not necessarily 64 characters, so
(I assume) the size specification is there to reserve the appropriate
amount of space, which is precisely 64 bytes.

> OSK0 + OSK1 is 64, why need an extra byte?

Right, I undid that part of the patch, will resubmit everything minus
that hunk as part of v2.

> Can yoy add some self-explanatory #define and use them later in this file:
> 
>     applesmc_add_key(s, "OSK0", 32, s->osk);
>     applesmc_add_key(s, "OSK1", 32, s->osk + 32);
> 
> and
> 
>     if (!s->osk || (strlen(s->osk) != 64)) {
>         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");

Do I have to, now that I'm staying away from touching that bit in the
first place ? :)

The original patch Eric Shelton proposed (inspired by FakeSMC)
completely reworks how keys are initialized, adds write support (and
access permissions) for select keys, and generally implements a much
more complete emulation of the AppleSMC. Since this is just about
getting OS X to keep working, I figured I'd keep changes to a minimum.

Reworking key initialization, adding write support, etc. could
(should?) be part of a separate series, if we decide we want a more
realistically emulated AppleSMC. In that case, minimizing churn and
leaving things as-is in this particular case would be preferable.

Let me know what you think.

Thanks,
--Gabriel
 
> 
> >                                "using the -osk parameter";
> > 
> >  struct AppleSMCData {
> > @@ -77,12 +80,11 @@ struct AppleSMCState {
> >      uint32_t iobase;
> >      uint8_t cmd;
> >      uint8_t status;
> > -    uint8_t key[4];
> > +    char key[4];
> >      uint8_t read_pos;
> >      uint8_t data_len;
> >      uint8_t data_pos;
> >      uint8_t data[255];
> > -    uint8_t charactic[4];
> >      char *osk;
> >      QLIST_HEAD(, AppleSMCData) data_def;
> >  };
> > @@ -93,10 +95,10 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
> >      AppleSMCState *s = opaque;
> > 
> >      smc_debug("CMD Write B: %#x = %#x\n", addr, val);
> > -    switch(val) {
> > -        case APPLESMC_READ_CMD:
> > -            s->status = 0x0c;
> > -            break;
> > +    switch (val) {
> > +    case APPLESMC_READ_CMD:
> > +        s->status = 0x0c;
> > +        break;
> >      }
> >      s->cmd = val;
> >      s->read_pos = 0;
> > @@ -123,54 +125,54 @@ static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
> >      AppleSMCState *s = opaque;
> > 
> >      smc_debug("DATA Write B: %#x = %#x\n", addr, val);
> > -    switch(s->cmd) {
> > -        case APPLESMC_READ_CMD:
> > -            if(s->read_pos < 4) {
> > -                s->key[s->read_pos] = val;
> > -                s->status = 0x04;
> > -            } else if(s->read_pos == 4) {
> > -                s->data_len = val;
> > -                s->status = 0x05;
> > -                s->data_pos = 0;
> > -                smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
> > -                          s->key[1], s->key[2], s->key[3], val);
> > -                applesmc_fill_data(s);
> > -            }
> > -            s->read_pos++;
> > -            break;
> > +    switch (s->cmd) {
> > +    case APPLESMC_READ_CMD:
> > +        if (s->read_pos < 4) {
> > +            s->key[s->read_pos] = val;
> > +            s->status = 0x04;
> > +        } else if (s->read_pos == 4) {
> > +            s->data_len = val;
> > +            s->status = 0x05;
> > +            s->data_pos = 0;
> > +            smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
> > +                      s->key[1], s->key[2], s->key[3], val);
> > +            applesmc_fill_data(s);
> > +        }
> > +        s->read_pos++;
> > +        break;
> >      }
> >  }
> > 
> > -static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr1,
> > -                                      unsigned size)
> > +static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> >      AppleSMCState *s = opaque;
> >      uint8_t retval = 0;
> > 
> > -    switch(s->cmd) {
> > -        case APPLESMC_READ_CMD:
> > -            if(s->data_pos < s->data_len) {
> > -                retval = s->data[s->data_pos];
> > -                smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
> > -                          retval);
> > -                s->data_pos++;
> > -                if(s->data_pos == s->data_len) {
> > -                    s->status = 0x00;
> > -                    smc_debug("EOF\n");
> > -                } else
> > -                    s->status = 0x05;
> > +    switch (s->cmd) {
> > +    case APPLESMC_READ_CMD:
> > +        if (s->data_pos < s->data_len) {
> > +            retval = s->data[s->data_pos];
> > +            smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
> > +                      retval);
> > +            s->data_pos++;
> > +            if (s->data_pos == s->data_len) {
> > +                s->status = 0x00;
> > +                smc_debug("EOF\n");
> > +            } else {
> > +                s->status = 0x05;
> >              }
> > +        }
> >      }
> > -    smc_debug("DATA Read b: %#x = %#x\n", addr1, retval);
> > +    smc_debug("DATA Read b: %#x = %#x\n", addr, retval);
> > 
> >      return retval;
> >  }
> > 
> > -static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr1, unsigned size)
> > +static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> >      AppleSMCState *s = opaque;
> > 
> > -    smc_debug("CMD Read B: %#x\n", addr1);
> > +    smc_debug("CMD Read B: %#x\n", addr);
> >      return s->status;
> >  }
> > 
> > 

Re: [Qemu-devel] [PATCH v1 1/3] applesmc: cosmetic whitespace and indentation cleanup

[Philippe Mathieu-Daudé]

On 04/03/2017 06:12 PM, Gabriel L. Somlo wrote:
> On Mon, Apr 03, 2017 at 10:34:09AM -0300, Philippe Mathieu-Daudé wrote:
>> Hi Gabriel,
>>
>> On 03/31/2017 01:48 PM, Gabriel L. Somlo wrote:
>>> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
>>> ---
>>>  hw/misc/applesmc.c | 100 +++++++++++++++++++++++++++--------------------------
>>>  1 file changed, 51 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
>>> index 77fab5b..986f2ac 100644
>>> --- a/hw/misc/applesmc.c
>>> +++ b/hw/misc/applesmc.c
>>> @@ -39,24 +39,27 @@
>>>  /* #define DEBUG_SMC */
>>>
>>>  #define APPLESMC_DEFAULT_IOBASE        0x300
>>> -/* data port used by Apple SMC */
>>> -#define APPLESMC_DATA_PORT             0x0
>>> -/* command/status port used by Apple SMC */
>>> -#define APPLESMC_CMD_PORT              0x4
>>> -#define APPLESMC_NR_PORTS              32
>>>
>>> -#define APPLESMC_READ_CMD              0x10
>>> -#define APPLESMC_WRITE_CMD             0x11
>>> -#define APPLESMC_GET_KEY_BY_INDEX_CMD  0x12
>>> -#define APPLESMC_GET_KEY_TYPE_CMD      0x13
>>> +enum {
>>> +    APPLESMC_DATA_PORT               = 0x00,
>>> +    APPLESMC_CMD_PORT                = 0x04,
>>> +    APPLESMC_NUM_PORTS               = 0x20,
>>> +};
>>> +
>>> +enum {
>>> +    APPLESMC_READ_CMD                = 0x10,
>>> +    APPLESMC_WRITE_CMD               = 0x11,
>>> +    APPLESMC_GET_KEY_BY_INDEX_CMD    = 0x12,
>>> +    APPLESMC_GET_KEY_TYPE_CMD        = 0x13,
>>> +};
>>>
>>>  #ifdef DEBUG_SMC
>>>  #define smc_debug(...) fprintf(stderr, "AppleSMC: " __VA_ARGS__)
>>>  #else
>>> -#define smc_debug(...) do { } while(0)
>>> +#define smc_debug(...) do { } while (0)
>>>  #endif
>>>
>>> -static char default_osk[64] = "This is a dummy key. Enter the real key "
>>> +static char default_osk[65] = "This is a dummy key. Enter the real key "
>>
>> Here is more than a cosmetic change.
>
> You're right, that was my mistake (should have left it 64.
>>
>> Since this array is initialized you can declare it without specify the size:
>> static char default_osk[] = ...
>
> It's initialized to something that's not necessarily 64 characters, so
> (I assume) the size specification is there to reserve the appropriate
> amount of space, which is precisely 64 bytes.

Since 'default_osk' is used read-only, it could be 'const' but since it 
is assigned as `s->osk = default_osk` which is a PROP_STRING it can not 
be const. For this read-only reason I think it is safer to let the cpp 
calculate the size to allocate to this buffer (without specifying 64 
which lead to confusion).

>
>> OSK0 + OSK1 is 64, why need an extra byte?
>
> Right, I undid that part of the patch, will resubmit everything minus
> that hunk as part of v2.
>
>> Can yoy add some self-explanatory #define and use them later in this file:
>>
>>     applesmc_add_key(s, "OSK0", 32, s->osk);
>>     applesmc_add_key(s, "OSK1", 32, s->osk + 32);
>>
>> and
>>
>>     if (!s->osk || (strlen(s->osk) != 64)) {
>>         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");
>
> Do I have to, now that I'm staying away from touching that bit in the
> first place ? :)
>
> The original patch Eric Shelton proposed (inspired by FakeSMC)
> completely reworks how keys are initialized, adds write support (and
> access permissions) for select keys, and generally implements a much
> more complete emulation of the AppleSMC. Since this is just about
> getting OS X to keep working, I figured I'd keep changes to a minimum.
>
> Reworking key initialization, adding write support, etc. could
> (should?) be part of a separate series, if we decide we want a more
> realistically emulated AppleSMC. In that case, minimizing churn and
> leaving things as-is in this particular case would be preferable.
>
> Let me know what you think.

Let keep it simple enough for this serie :)

Either with the 64B array size back or without:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>
> Thanks,
> --Gabriel
>
>>
>>>                                "using the -osk parameter";
>>>
>>>  struct AppleSMCData {
>>> @@ -77,12 +80,11 @@ struct AppleSMCState {
>>>      uint32_t iobase;
>>>      uint8_t cmd;
>>>      uint8_t status;
>>> -    uint8_t key[4];
>>> +    char key[4];
>>>      uint8_t read_pos;
>>>      uint8_t data_len;
>>>      uint8_t data_pos;
>>>      uint8_t data[255];
>>> -    uint8_t charactic[4];
>>>      char *osk;
>>>      QLIST_HEAD(, AppleSMCData) data_def;
>>>  };
>>> @@ -93,10 +95,10 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
>>>      AppleSMCState *s = opaque;
>>>
>>>      smc_debug("CMD Write B: %#x = %#x\n", addr, val);
>>> -    switch(val) {
>>> -        case APPLESMC_READ_CMD:
>>> -            s->status = 0x0c;
>>> -            break;
>>> +    switch (val) {
>>> +    case APPLESMC_READ_CMD:
>>> +        s->status = 0x0c;
>>> +        break;
>>>      }
>>>      s->cmd = val;
>>>      s->read_pos = 0;
>>> @@ -123,54 +125,54 @@ static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
>>>      AppleSMCState *s = opaque;
>>>
>>>      smc_debug("DATA Write B: %#x = %#x\n", addr, val);
>>> -    switch(s->cmd) {
>>> -        case APPLESMC_READ_CMD:
>>> -            if(s->read_pos < 4) {
>>> -                s->key[s->read_pos] = val;
>>> -                s->status = 0x04;
>>> -            } else if(s->read_pos == 4) {
>>> -                s->data_len = val;
>>> -                s->status = 0x05;
>>> -                s->data_pos = 0;
>>> -                smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
>>> -                          s->key[1], s->key[2], s->key[3], val);
>>> -                applesmc_fill_data(s);
>>> -            }
>>> -            s->read_pos++;
>>> -            break;
>>> +    switch (s->cmd) {
>>> +    case APPLESMC_READ_CMD:
>>> +        if (s->read_pos < 4) {
>>> +            s->key[s->read_pos] = val;
>>> +            s->status = 0x04;
>>> +        } else if (s->read_pos == 4) {
>>> +            s->data_len = val;
>>> +            s->status = 0x05;
>>> +            s->data_pos = 0;
>>> +            smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
>>> +                      s->key[1], s->key[2], s->key[3], val);
>>> +            applesmc_fill_data(s);
>>> +        }
>>> +        s->read_pos++;
>>> +        break;
>>>      }
>>>  }
>>>
>>> -static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr1,
>>> -                                      unsigned size)
>>> +static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr, unsigned size)
>>>  {
>>>      AppleSMCState *s = opaque;
>>>      uint8_t retval = 0;
>>>
>>> -    switch(s->cmd) {
>>> -        case APPLESMC_READ_CMD:
>>> -            if(s->data_pos < s->data_len) {
>>> -                retval = s->data[s->data_pos];
>>> -                smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
>>> -                          retval);
>>> -                s->data_pos++;
>>> -                if(s->data_pos == s->data_len) {
>>> -                    s->status = 0x00;
>>> -                    smc_debug("EOF\n");
>>> -                } else
>>> -                    s->status = 0x05;
>>> +    switch (s->cmd) {
>>> +    case APPLESMC_READ_CMD:
>>> +        if (s->data_pos < s->data_len) {
>>> +            retval = s->data[s->data_pos];
>>> +            smc_debug("READ_DATA[%d] = %#hhx\n", s->data_pos,
>>> +                      retval);
>>> +            s->data_pos++;
>>> +            if (s->data_pos == s->data_len) {
>>> +                s->status = 0x00;
>>> +                smc_debug("EOF\n");
>>> +            } else {
>>> +                s->status = 0x05;
>>>              }
>>> +        }
>>>      }
>>> -    smc_debug("DATA Read b: %#x = %#x\n", addr1, retval);
>>> +    smc_debug("DATA Read b: %#x = %#x\n", addr, retval);
>>>
>>>      return retval;
>>>  }
>>>
>>> -static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr1, unsigned size)
>>> +static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr, unsigned size)
>>>  {
>>>      AppleSMCState *s = opaque;
>>>
>>> -    smc_debug("CMD Read B: %#x\n", addr1);
>>> +    smc_debug("CMD Read B: %#x\n", addr);
>>>      return s->status;
>>>  }
>>>
>>>

Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Gabriel L. Somlo]

On Mon, Apr 03, 2017 at 12:27:15PM +0200, Paolo Bonzini wrote:
> 
> 
> On 03/04/2017 11:32, Alexander Graf wrote:
> > 
> >> Newer AppleSMC revisions support an error status (read) port
> >> in addition to the data and command ports currently supported.
> >>
> >> Register the full 32-bit region at once, and handle individual
> >> ports at various offsets within the region from the top-level
> >> applesmc_io_[write|read]() methods (ctual support for reading
> >> the error status port to be added by a subsequent patch).
> >>
> >> Originally proposed by Eric Shelton <eshelton@pobox.com>
> >>
> >> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > 
> > I would prefer if we could leave the multiplexing to the layer that
> > knows how to do that the best: Memory Regions.
> > 
> > Why don't you just define a big region that ecompasses all of the IO
> > space (with fallback I/O handlers for warnings) and then just sprinkle
> > the ones we handle on top with higher prio?
> 
> You don't need priority at all, "contained" regions always win over the
> container (docs/memory.txt just before "Region names").
> 
> Both what you propose and what Gabriel did makes sense.  In general QEMU
> does things more like in this patch, but there are exceptions (e.g. ACPI).

So, option 1 would be:

Leave the region covering the entire AppleSMC port range (APPLESMC_NUM_PORTS)
as-is:

static const MemoryRegionOps applesmc_io_ops = {
    .write = applesmc_io_write,
    .read = applesmc_io_read,
    .endianness = DEVICE_NATIVE_ENDIAN,
    .impl = {
        .min_access_size = 1,
        .max_access_size = 1,
    },
};

but modify applesmc_io_write() and applesmc_io_read() to do nothing or
return 0xff, respectively. Then, add three separate regions covering 1
byte, for each of the three ports, with their own methods pointing at
the existing port-specific read/write methods.


Option 2 would be to leave everything as-is, per Paolo's suggestion
that it's the more common way of doing things. I personally find this
one easier to follow, but really don't mind doing either.


If I end up going with #1, I'd probably be bringing back
applesmc_data_io_ops and applesmc_cmd_io_ops, in which case I'd like
to know why the access size was set to 4 bytes to begin with:

-    memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
-                          "applesmc-data", 4);
-    isa_register_ioport(&s->parent_obj, &s->io_data,
-                        s->iobase + APPLESMC_DATA_PORT);
-
-    memory_region_init_io(&s->io_cmd, OBJECT(s), &applesmc_cmd_io_ops, s,
-                          "applesmc-cmd", 4);
-    isa_register_ioport(&s->parent_obj, &s->io_cmd,
-                        s->iobase + APPLESMC_CMD_PORT);

Each port is 8-bits wide only, so why 4 and not 1, above ?

I guess that doesn't matter if we stick with option #2... :)

Any further advice on which way to go much appreciated!

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Alexander Graf]

On 04/04/2017 02:04 AM, Gabriel L. Somlo wrote:
> On Mon, Apr 03, 2017 at 12:27:15PM +0200, Paolo Bonzini wrote:
>>
>> On 03/04/2017 11:32, Alexander Graf wrote:
>>>> Newer AppleSMC revisions support an error status (read) port
>>>> in addition to the data and command ports currently supported.
>>>>
>>>> Register the full 32-bit region at once, and handle individual
>>>> ports at various offsets within the region from the top-level
>>>> applesmc_io_[write|read]() methods (ctual support for reading
>>>> the error status port to be added by a subsequent patch).
>>>>
>>>> Originally proposed by Eric Shelton <eshelton@pobox.com>
>>>>
>>>> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
>>> I would prefer if we could leave the multiplexing to the layer that
>>> knows how to do that the best: Memory Regions.
>>>
>>> Why don't you just define a big region that ecompasses all of the IO
>>> space (with fallback I/O handlers for warnings) and then just sprinkle
>>> the ones we handle on top with higher prio?
>> You don't need priority at all, "contained" regions always win over the
>> container (docs/memory.txt just before "Region names").
>>
>> Both what you propose and what Gabriel did makes sense.  In general QEMU
>> does things more like in this patch, but there are exceptions (e.g. ACPI).
> So, option 1 would be:
>
> Leave the region covering the entire AppleSMC port range (APPLESMC_NUM_PORTS)
> as-is:
>
> static const MemoryRegionOps applesmc_io_ops = {
>      .write = applesmc_io_write,
>      .read = applesmc_io_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .impl = {
>          .min_access_size = 1,
>          .max_access_size = 1,
>      },
> };
>
> but modify applesmc_io_write() and applesmc_io_read() to do nothing or
> return 0xff, respectively. Then, add three separate regions covering 1
> byte, for each of the three ports, with their own methods pointing at
> the existing port-specific read/write methods.

Basically, yes. I think it would reduce the code churn quite a bit, as 
99% of it stays the same. You only add the fall-through layer.

> Option 2 would be to leave everything as-is, per Paolo's suggestion
> that it's the more common way of doing things. I personally find this
> one easier to follow, but really don't mind doing either.
>
>
> If I end up going with #1, I'd probably be bringing back
> applesmc_data_io_ops and applesmc_cmd_io_ops, in which case I'd like
> to know why the access size was set to 4 bytes to begin with:
>
> -    memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
> -                          "applesmc-data", 4);
> -    isa_register_ioport(&s->parent_obj, &s->io_data,
> -                        s->iobase + APPLESMC_DATA_PORT);
> -
> -    memory_region_init_io(&s->io_cmd, OBJECT(s), &applesmc_cmd_io_ops, s,
> -                          "applesmc-cmd", 4);
> -    isa_register_ioport(&s->parent_obj, &s->io_cmd,
> -                        s->iobase + APPLESMC_CMD_PORT);
>
> Each port is 8-bits wide only, so why 4 and not 1, above ?

I don't fully remember, but I suppose Mac OS X or Linux were accessing 
it using 32bit read/write operations, so we had to encompass the full 
size. I don't think you need to do that if you have the fall-through.

> I guess that doesn't matter if we stick with option #2... :)
>
> Any further advice on which way to go much appreciated!

I was mostly curious on why you would change so much code without any 
obvious gain. I'm perfectly fine with moving to a switch based approach 
for the memory region, but I didn't really understand *why* it was done 
in the first place :). It just felt like a lot of patch for no reason.


Alex

Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region

[Gabriel L. Somlo]

On Tue, Apr 04, 2017 at 11:44:29AM +0200, Alexander Graf wrote:
> On 04/04/2017 02:04 AM, Gabriel L. Somlo wrote:
> > On Mon, Apr 03, 2017 at 12:27:15PM +0200, Paolo Bonzini wrote:
> > > 
> > > On 03/04/2017 11:32, Alexander Graf wrote:
> > > > > Newer AppleSMC revisions support an error status (read) port
> > > > > in addition to the data and command ports currently supported.
> > > > > 
> > > > > Register the full 32-bit region at once, and handle individual
> > > > > ports at various offsets within the region from the top-level
> > > > > applesmc_io_[write|read]() methods (ctual support for reading
> > > > > the error status port to be added by a subsequent patch).
> > > > > 
> > > > > Originally proposed by Eric Shelton <eshelton@pobox.com>
> > > > > 
> > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > > > I would prefer if we could leave the multiplexing to the layer that
> > > > knows how to do that the best: Memory Regions.
> > > > 
> > > > Why don't you just define a big region that ecompasses all of the IO
> > > > space (with fallback I/O handlers for warnings) and then just sprinkle
> > > > the ones we handle on top with higher prio?
> > > You don't need priority at all, "contained" regions always win over the
> > > container (docs/memory.txt just before "Region names").
> > > 
> > > Both what you propose and what Gabriel did makes sense.  In general QEMU
> > > does things more like in this patch, but there are exceptions (e.g. ACPI).
> > So, option 1 would be:
> > 
> > Leave the region covering the entire AppleSMC port range (APPLESMC_NUM_PORTS)
> > as-is:
> > 
> > static const MemoryRegionOps applesmc_io_ops = {
> >      .write = applesmc_io_write,
> >      .read = applesmc_io_read,
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >      .impl = {
> >          .min_access_size = 1,
> >          .max_access_size = 1,
> >      },
> > };
> > 
> > but modify applesmc_io_write() and applesmc_io_read() to do nothing or
> > return 0xff, respectively. Then, add three separate regions covering 1
> > byte, for each of the three ports, with their own methods pointing at
> > the existing port-specific read/write methods.
> 
> Basically, yes. I think it would reduce the code churn quite a bit, as 99%
> of it stays the same. You only add the fall-through layer.
> 
> > Option 2 would be to leave everything as-is, per Paolo's suggestion
> > that it's the more common way of doing things. I personally find this
> > one easier to follow, but really don't mind doing either.
> > 
> > 
> > If I end up going with #1, I'd probably be bringing back
> > applesmc_data_io_ops and applesmc_cmd_io_ops, in which case I'd like
> > to know why the access size was set to 4 bytes to begin with:
> > 
> > -    memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
> > -                          "applesmc-data", 4);
> > -    isa_register_ioport(&s->parent_obj, &s->io_data,
> > -                        s->iobase + APPLESMC_DATA_PORT);
> > -
> > -    memory_region_init_io(&s->io_cmd, OBJECT(s), &applesmc_cmd_io_ops, s,
> > -                          "applesmc-cmd", 4);
> > -    isa_register_ioport(&s->parent_obj, &s->io_cmd,
> > -                        s->iobase + APPLESMC_CMD_PORT);
> > 
> > Each port is 8-bits wide only, so why 4 and not 1, above ?
> 
> I don't fully remember, but I suppose Mac OS X or Linux were accessing it
> using 32bit read/write operations, so we had to encompass the full size. I
> don't think you need to do that if you have the fall-through.
> 
> > I guess that doesn't matter if we stick with option #2... :)
> > 
> > Any further advice on which way to go much appreciated!
> 
> I was mostly curious on why you would change so much code without any
> obvious gain. I'm perfectly fine with moving to a switch based approach for
> the memory region, but I didn't really understand *why* it was done in the
> first place :). It just felt like a lot of patch for no reason.

At first glance, I found the idea of having a single pair of
read/write access methods for the whole 32-byte region, with
supported/unsupported ports handled internally to be aesthetically
appealing. Now I'm not so sure anymore... :)

What about if I leave everything alone, and just add a third region to
represent the error port only, with no fallback?

Also, I'll probably be able to reverse engineer this by staring at the
code long enough, but if something doesn't support e.g. a write method,
is it better to not provide a .write method to the respective region at
all, or to provide one that does nothing and returns immediately ?
(same question for the fallback/container region, if it turns out to
be reauired: provide empty read/write methods, or don't set .read and
.write at all ?)

Thanks,
--Gabriel