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

2017-04-03 Thread 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 
---
 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_IOBASE0x300
-/* 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é 



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;
-

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

2017-04-03 Thread 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 
> > ---
> >  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_IOBASE0x300
> > -/* 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",

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

2017-04-03 Thread Philippe Mathieu-Daudé 

Hi Gabriel,

On 03/31/2017 01:48 PM, Gabriel L. Somlo wrote:

Signed-off-by: Gabriel Somlo 
---
 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_IOBASE0x300
-/* 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];
+

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

2017-04-03 Thread Alexander Graf 

On 03/31/2017 06:48 PM, Gabriel L. Somlo wrote:

Signed-off-by: Gabriel Somlo 


Reviewed-by: Alexander Graf 


Alex

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

2017-03-31 Thread Gabriel L. Somlo 
Signed-off-by: Gabriel Somlo 
---
 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_IOBASE0x300
-/* 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,