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 <gso...@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;
> > }
> >
> >
