[PATCH v3] isa-applesmc: provide OSK forwarding on Apple hosts

Vladislav Yaroshchuk posted 1 patch 2 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211022111007.43478-1-yaroshchuk2000@gmail.com
There is a newer version of this series
hw/misc/applesmc.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 146 insertions(+), 1 deletion(-)
[PATCH v3] isa-applesmc: provide OSK forwarding on Apple hosts
Posted by Vladislav Yaroshchuk 2 years, 5 months ago
On Apple hosts we can read AppleSMC OSK key directly from host's
SMC and forward this value to QEMU Guest.

Usage:
`-device isa-applesmc,hostosk=on`

Apple licence allows use and run up to two additional copies
or instances of macOS operating within virtual operating system
environments on each Apple-branded computer that is already running
the Apple Software, for purposes of:
- software development
- testing during software development
- using macOS Server
- personal, non-commercial use

Guest macOS requires AppleSMC with correct OSK. The most legal
way to pass it to the Guest is to forward the key from host SMC
without any value exposion.

Based on https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/

Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
---
 hw/misc/applesmc.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 1b9acaf1d3..bcc021f7b7 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -38,6 +38,10 @@
 #include "qemu/timer.h"
 #include "qom/object.h"
 
+#if defined(__APPLE__)
+#include <IOKit/IOKitLib.h>
+#endif
+
 /* #define DEBUG_SMC */
 
 #define APPLESMC_DEFAULT_IOBASE        0x300
@@ -108,6 +112,7 @@ struct AppleSMCState {
     uint8_t data_len;
     uint8_t data_pos;
     uint8_t data[255];
+    char *hostosk_flag;
     char *osk;
     QLIST_HEAD(, AppleSMCData) data_def;
 };
@@ -312,9 +317,136 @@ static const MemoryRegionOps applesmc_err_io_ops = {
     },
 };
 
+#if defined(__APPLE__)
+/*
+ * Based on
+ * https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
+ */
+enum {
+    SMC_CLIENT_OPEN      = 0,
+    SMC_CLIENT_CLOSE     = 1,
+    SMC_HANDLE_EVENT     = 2,
+    SMC_READ_KEY         = 5
+};
+
+struct AppleSMCParam {
+    uint32_t    key;
+    uint8_t     pad0[22];
+    IOByteCount data_size;
+    uint8_t     pad1[10];
+    uint8_t     command;
+    uint32_t    pad2;
+    uint8_t     bytes[32];
+};
+
+static int applesmc_read_host_osk(char **host_osk)
+{
+    assert(host_osk != NULL);
+
+    io_service_t            hostsmc_service = IO_OBJECT_NULL;
+    io_connect_t            hostsmc_connect = IO_OBJECT_NULL;
+    size_t                  out_size = sizeof(struct AppleSMCParam);
+    IOReturn                status = kIOReturnError;
+    struct AppleSMCParam    in = {0};
+    struct AppleSMCParam    out = {0};
+
+    /* OSK key size + '\0' */
+    *host_osk = g_malloc0(65);
+    (*host_osk)[64] = '\0';
+
+    hostsmc_service = IOServiceGetMatchingService(kIOMasterPortDefault,
+                                          IOServiceMatching("AppleSMC"));
+    if (hostsmc_service == IO_OBJECT_NULL) {
+        warn_report("host AppleSMC device is unreachable");
+        goto error_osk_buffer_free;
+    }
+
+    status = IOServiceOpen(hostsmc_service,
+                           mach_task_self(),
+                           1,
+                           &hostsmc_connect);
+    if (status != kIOReturnSuccess || hostsmc_connect == IO_OBJECT_NULL) {
+        warn_report("host AppleSMC device is unreachable");
+        goto error_osk_buffer_free;
+    }
+
+    status = IOConnectCallMethod(
+        hostsmc_connect,
+        SMC_CLIENT_OPEN,
+        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL
+    );
+    if (status != kIOReturnSuccess) {
+        warn_report("host AppleSMC device is unreachable");
+        goto error_ioservice_close;
+    }
+
+    in.key = ('OSK0');
+    in.data_size = sizeof(out.bytes);
+    in.command = SMC_READ_KEY;
+    status = IOConnectCallStructMethod(
+        hostsmc_connect,
+        SMC_HANDLE_EVENT,
+        &in,
+        sizeof(struct AppleSMCParam),
+        &out,
+        &out_size
+    );
+
+    if (status != kIOReturnSuccess) {
+        warn_report("unable to read OSK0 from host AppleSMC device");
+        goto error_ioconnect_close;
+    }
+    strncpy(*host_osk, (const char *) out.bytes, 32);
+
+    in.key = ('OSK1');
+    in.data_size = sizeof(out.bytes);
+    in.command = SMC_READ_KEY;
+    status = IOConnectCallStructMethod(
+        hostsmc_connect,
+        SMC_HANDLE_EVENT,
+        &in,
+        sizeof(struct AppleSMCParam),
+        &out,
+        &out_size
+    );
+    if (status != kIOReturnSuccess) {
+        warn_report("unable to read OSK1 from host AppleSMC device");
+        goto error_ioconnect_close;
+    }
+    strncpy((*host_osk) + 32, (const char *) out.bytes, 32);
+
+    IOConnectCallMethod(
+        hostsmc_connect,
+        SMC_CLIENT_CLOSE,
+        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
+    IOServiceClose(hostsmc_connect);
+    return 0;
+
+error_ioconnect_close:
+    IOConnectCallMethod(
+        hostsmc_connect,
+        SMC_CLIENT_CLOSE,
+        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
+error_ioservice_close:
+    IOServiceClose(hostsmc_connect);
+
+error_osk_buffer_free:
+    g_free(*host_osk);
+    return -1;
+}
+#else
+static int applesmc_read_host_osk(char **output_key)
+{
+    warn_report("isa-applesmc.hostosk ignored: "
+                "unsupported on non-Apple hosts");
+    return -1;
+}
+#endif
+
 static void applesmc_isa_realize(DeviceState *dev, Error **errp)
 {
-    AppleSMCState *s = APPLE_SMC(dev);
+    AppleSMCState   *s = APPLE_SMC(dev);
+    char            *host_osk;
 
     memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
                           "applesmc-data", 1);
@@ -331,6 +463,18 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
     isa_register_ioport(&s->parent_obj, &s->io_err,
                         s->iobase + APPLESMC_ERR_PORT);
 
+    /* Key retrieved from host SMC overwrites provided OSK string */
+    if (s->hostosk_flag
+        && !strcmp("on", s->hostosk_flag)
+        && !applesmc_read_host_osk(&host_osk)) {
+        if (s->osk) {
+            warn_report("provided isa-applesmc.osk "
+                        "is overwritten with host OSK");
+            g_free(s->osk);
+        }
+        s->osk = host_osk;
+    }
+
     if (!s->osk || (strlen(s->osk) != 64)) {
         warn_report("Using AppleSMC with invalid key");
         s->osk = default_osk;
@@ -344,6 +488,7 @@ static Property applesmc_isa_properties[] = {
     DEFINE_PROP_UINT32(APPLESMC_PROP_IO_BASE, AppleSMCState, iobase,
                        APPLESMC_DEFAULT_IOBASE),
     DEFINE_PROP_STRING("osk", AppleSMCState, osk),
+    DEFINE_PROP_STRING("hostosk", AppleSMCState, hostosk_flag),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.23.0


Re: [PATCH v3] isa-applesmc: provide OSK forwarding on Apple hosts
Posted by Laurent Vivier 2 years, 5 months ago
Le 22/10/2021 à 13:10, Vladislav Yaroshchuk a écrit :
> On Apple hosts we can read AppleSMC OSK key directly from host's
> SMC and forward this value to QEMU Guest.
> 
> Usage:
> `-device isa-applesmc,hostosk=on`
> 
> Apple licence allows use and run up to two additional copies
> or instances of macOS operating within virtual operating system
> environments on each Apple-branded computer that is already running
> the Apple Software, for purposes of:
> - software development
> - testing during software development
> - using macOS Server
> - personal, non-commercial use
> 
> Guest macOS requires AppleSMC with correct OSK. The most legal
> way to pass it to the Guest is to forward the key from host SMC
> without any value exposion.
> 
> Based on https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> 
> Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
> ---
>   hw/misc/applesmc.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 146 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 1b9acaf1d3..bcc021f7b7 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -38,6 +38,10 @@
>   #include "qemu/timer.h"
>   #include "qom/object.h"
>   
> +#if defined(__APPLE__)
> +#include <IOKit/IOKitLib.h>
> +#endif
> +
>   /* #define DEBUG_SMC */
>   
>   #define APPLESMC_DEFAULT_IOBASE        0x300
> @@ -108,6 +112,7 @@ struct AppleSMCState {
>       uint8_t data_len;
>       uint8_t data_pos;
>       uint8_t data[255];
> +    char *hostosk_flag;

Use a boolean (see below)

>       char *osk;
>       QLIST_HEAD(, AppleSMCData) data_def;
>   };
> @@ -312,9 +317,136 @@ static const MemoryRegionOps applesmc_err_io_ops = {
>       },
>   };
>   
> +#if defined(__APPLE__)
> +/*
> + * Based on
> + * https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> + */
> +enum {
> +    SMC_CLIENT_OPEN      = 0,
> +    SMC_CLIENT_CLOSE     = 1,
> +    SMC_HANDLE_EVENT     = 2,
> +    SMC_READ_KEY         = 5
> +};
> +
> +struct AppleSMCParam {
> +    uint32_t    key;
> +    uint8_t     pad0[22];
> +    IOByteCount data_size;
> +    uint8_t     pad1[10];
> +    uint8_t     command;
> +    uint32_t    pad2;
> +    uint8_t     bytes[32];
> +};
> +
> +static int applesmc_read_host_osk(char **host_osk)

I think you should return the error message using an "Error **errp".

> +{
> +    assert(host_osk != NULL);
> +
> +    io_service_t            hostsmc_service = IO_OBJECT_NULL;
> +    io_connect_t            hostsmc_connect = IO_OBJECT_NULL;
> +    size_t                  out_size = sizeof(struct AppleSMCParam);
> +    IOReturn                status = kIOReturnError;
> +    struct AppleSMCParam    in = {0};
> +    struct AppleSMCParam    out = {0};
> +
> +    /* OSK key size + '\0' */
> +    *host_osk = g_malloc0(65);
> +    (*host_osk)[64] = '\0';
> +
> +    hostsmc_service = IOServiceGetMatchingService(kIOMasterPortDefault,
> +                                          IOServiceMatching("AppleSMC"));
> +    if (hostsmc_service == IO_OBJECT_NULL) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_osk_buffer_free;
> +    }
> +
> +    status = IOServiceOpen(hostsmc_service,
> +                           mach_task_self(),
> +                           1,
> +                           &hostsmc_connect);
> +    if (status != kIOReturnSuccess || hostsmc_connect == IO_OBJECT_NULL) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_osk_buffer_free;
> +    }
> +
> +    status = IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_OPEN,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL
> +    );
> +    if (status != kIOReturnSuccess) {
> +        warn_report("host AppleSMC device is unreachable");
> +        goto error_ioservice_close;
> +    }
> +
> +    in.key = ('OSK0');
> +    in.data_size = sizeof(out.bytes);
> +    in.command = SMC_READ_KEY;
> +    status = IOConnectCallStructMethod(
> +        hostsmc_connect,
> +        SMC_HANDLE_EVENT,
> +        &in,
> +        sizeof(struct AppleSMCParam),
> +        &out,
> +        &out_size
> +    );
> +
> +    if (status != kIOReturnSuccess) {
> +        warn_report("unable to read OSK0 from host AppleSMC device");
> +        goto error_ioconnect_close;
> +    }
> +    strncpy(*host_osk, (const char *) out.bytes, 32);
> +
> +    in.key = ('OSK1');
> +    in.data_size = sizeof(out.bytes);
> +    in.command = SMC_READ_KEY;
> +    status = IOConnectCallStructMethod(
> +        hostsmc_connect,
> +        SMC_HANDLE_EVENT,
> +        &in,
> +        sizeof(struct AppleSMCParam),
> +        &out,
> +        &out_size
> +    );
> +    if (status != kIOReturnSuccess) {
> +        warn_report("unable to read OSK1 from host AppleSMC device");
> +        goto error_ioconnect_close;
> +    }
> +    strncpy((*host_osk) + 32, (const char *) out.bytes, 32);
> +
> +    IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_CLOSE,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> +    IOServiceClose(hostsmc_connect);
> +    return 0;
> +
> +error_ioconnect_close:
> +    IOConnectCallMethod(
> +        hostsmc_connect,
> +        SMC_CLIENT_CLOSE,
> +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> +error_ioservice_close:
> +    IOServiceClose(hostsmc_connect);
> +
> +error_osk_buffer_free:
> +    g_free(*host_osk);
> +    return -1;
> +}
> +#else
> +static int applesmc_read_host_osk(char **output_key)
> +{
> +    warn_report("isa-applesmc.hostosk ignored: "
> +                "unsupported on non-Apple hosts");

I think a failure would be better than a warning.
Moreover will it work if the user doenst provide the OSK?

> +    return -1;
> +}
> +#endif
> +
>   static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>   {
> -    AppleSMCState *s = APPLE_SMC(dev);
> +    AppleSMCState   *s = APPLE_SMC(dev);
> +    char            *host_osk;
>   
>       memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s,
>                             "applesmc-data", 1);
> @@ -331,6 +463,18 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>       isa_register_ioport(&s->parent_obj, &s->io_err,
>                           s->iobase + APPLESMC_ERR_PORT);
>   
> +    /* Key retrieved from host SMC overwrites provided OSK string */
> +    if (s->hostosk_flag
> +        && !strcmp("on", s->hostosk_flag)

Use a bool property for hostosk (see below), that will allow "on", "yes", "true", ... and here 
you'll only have to test for the boolean value.

> +        && !applesmc_read_host_osk(&host_osk)) {
> +        if (s->osk) {
> +            warn_report("provided isa-applesmc.osk "
> +                        "is overwritten with host OSK");
> +            g_free(s->osk);
> +        }
> +        s->osk = host_osk;
> +    }
> +
>       if (!s->osk || (strlen(s->osk) != 64)) {
>           warn_report("Using AppleSMC with invalid key");
>           s->osk = default_osk;
> @@ -344,6 +488,7 @@ static Property applesmc_isa_properties[] = {
>       DEFINE_PROP_UINT32(APPLESMC_PROP_IO_BASE, AppleSMCState, iobase,
>                          APPLESMC_DEFAULT_IOBASE),
>       DEFINE_PROP_STRING("osk", AppleSMCState, osk),
> +    DEFINE_PROP_STRING("hostosk", AppleSMCState, hostosk_flag),

An DEFINE_PROP_BOOL() would be more appropriate for this.

>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Thanks,
Laurent


Re: [PATCH v3] isa-applesmc: provide OSK forwarding on Apple hosts
Posted by Vladislav Yaroshchuk 2 years, 5 months ago
Hi, Laurent!
Thank you for your review!

пт, 22 окт. 2021 г. в 14:36, Laurent Vivier <laurent@vivier.eu>:

> Le 22/10/2021 à 13:10, Vladislav Yaroshchuk a écrit :
> > On Apple hosts we can read AppleSMC OSK key directly from host's
> > SMC and forward this value to QEMU Guest.
> >
> > Usage:
> > `-device isa-applesmc,hostosk=on`
> >
> > Apple licence allows use and run up to two additional copies
> > or instances of macOS operating within virtual operating system
> > environments on each Apple-branded computer that is already running
> > the Apple Software, for purposes of:
> > - software development
> > - testing during software development
> > - using macOS Server
> > - personal, non-commercial use
> >
> > Guest macOS requires AppleSMC with correct OSK. The most legal
> > way to pass it to the Guest is to forward the key from host SMC
> > without any value exposion.
> >
> > Based on
> https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> >
> > Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
> > ---
> >   hw/misc/applesmc.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 146 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> > index 1b9acaf1d3..bcc021f7b7 100644
> > --- a/hw/misc/applesmc.c
> > +++ b/hw/misc/applesmc.c
> > @@ -38,6 +38,10 @@
> >   #include "qemu/timer.h"
> >   #include "qom/object.h"
> >
> > +#if defined(__APPLE__)
> > +#include <IOKit/IOKitLib.h>
> > +#endif
> > +
> >   /* #define DEBUG_SMC */
> >
> >   #define APPLESMC_DEFAULT_IOBASE        0x300
> > @@ -108,6 +112,7 @@ struct AppleSMCState {
> >       uint8_t data_len;
> >       uint8_t data_pos;
> >       uint8_t data[255];
> > +    char *hostosk_flag;
>
> Use a boolean (see below)
>
> >       char *osk;
> >       QLIST_HEAD(, AppleSMCData) data_def;
> >   };
> > @@ -312,9 +317,136 @@ static const MemoryRegionOps applesmc_err_io_ops =
> {
> >       },
> >   };
> >
> > +#if defined(__APPLE__)
> > +/*
> > + * Based on
> > + *
> https://web.archive.org/web/20200103161737/osxbook.com/book/bonus/chapter7/tpmdrmmyth/
> > + */
> > +enum {
> > +    SMC_CLIENT_OPEN      = 0,
> > +    SMC_CLIENT_CLOSE     = 1,
> > +    SMC_HANDLE_EVENT     = 2,
> > +    SMC_READ_KEY         = 5
> > +};
> > +
> > +struct AppleSMCParam {
> > +    uint32_t    key;
> > +    uint8_t     pad0[22];
> > +    IOByteCount data_size;
> > +    uint8_t     pad1[10];
> > +    uint8_t     command;
> > +    uint32_t    pad2;
> > +    uint8_t     bytes[32];
> > +};
> > +
> > +static int applesmc_read_host_osk(char **host_osk)
>
> I think you should return the error message using an "Error **errp".


Yep, sounds much better, will fix this in the next patch version.

> +{
> > +    assert(host_osk != NULL);
> > +
> > +    io_service_t            hostsmc_service = IO_OBJECT_NULL;
> > +    io_connect_t            hostsmc_connect = IO_OBJECT_NULL;
> > +    size_t                  out_size = sizeof(struct AppleSMCParam);
> > +    IOReturn                status = kIOReturnError;
> > +    struct AppleSMCParam    in = {0};
> > +    struct AppleSMCParam    out = {0};
> > +
> > +    /* OSK key size + '\0' */
> > +    *host_osk = g_malloc0(65);
> > +    (*host_osk)[64] = '\0';
> > +
> > +    hostsmc_service = IOServiceGetMatchingService(kIOMasterPortDefault,
> > +
> IOServiceMatching("AppleSMC"));
> > +    if (hostsmc_service == IO_OBJECT_NULL) {
> > +        warn_report("host AppleSMC device is unreachable");
> > +        goto error_osk_buffer_free;
> > +    }
> > +
> > +    status = IOServiceOpen(hostsmc_service,
> > +                           mach_task_self(),
> > +                           1,
> > +                           &hostsmc_connect);
> > +    if (status != kIOReturnSuccess || hostsmc_connect ==
> IO_OBJECT_NULL) {
> > +        warn_report("host AppleSMC device is unreachable");
> > +        goto error_osk_buffer_free;
> > +    }
> > +
> > +    status = IOConnectCallMethod(
> > +        hostsmc_connect,
> > +        SMC_CLIENT_OPEN,
> > +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL
> > +    );
> > +    if (status != kIOReturnSuccess) {
> > +        warn_report("host AppleSMC device is unreachable");
> > +        goto error_ioservice_close;
> > +    }
> > +
> > +    in.key = ('OSK0');
> > +    in.data_size = sizeof(out.bytes);
> > +    in.command = SMC_READ_KEY;
> > +    status = IOConnectCallStructMethod(
> > +        hostsmc_connect,
> > +        SMC_HANDLE_EVENT,
> > +        &in,
> > +        sizeof(struct AppleSMCParam),
> > +        &out,
> > +        &out_size
> > +    );
> > +
> > +    if (status != kIOReturnSuccess) {
> > +        warn_report("unable to read OSK0 from host AppleSMC device");
> > +        goto error_ioconnect_close;
> > +    }
> > +    strncpy(*host_osk, (const char *) out.bytes, 32);
> > +
> > +    in.key = ('OSK1');
> > +    in.data_size = sizeof(out.bytes);
> > +    in.command = SMC_READ_KEY;
> > +    status = IOConnectCallStructMethod(
> > +        hostsmc_connect,
> > +        SMC_HANDLE_EVENT,
> > +        &in,
> > +        sizeof(struct AppleSMCParam),
> > +        &out,
> > +        &out_size
> > +    );
> > +    if (status != kIOReturnSuccess) {
> > +        warn_report("unable to read OSK1 from host AppleSMC device");
> > +        goto error_ioconnect_close;
> > +    }
> > +    strncpy((*host_osk) + 32, (const char *) out.bytes, 32);
> > +
> > +    IOConnectCallMethod(
> > +        hostsmc_connect,
> > +        SMC_CLIENT_CLOSE,
> > +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> > +    IOServiceClose(hostsmc_connect);
> > +    return 0;
> > +
> > +error_ioconnect_close:
> > +    IOConnectCallMethod(
> > +        hostsmc_connect,
> > +        SMC_CLIENT_CLOSE,
> > +        NULL, 0, NULL, 0, NULL, NULL, NULL, NULL);
> > +error_ioservice_close:
> > +    IOServiceClose(hostsmc_connect);
> > +
> > +error_osk_buffer_free:
> > +    g_free(*host_osk);
> > +    return -1;
> > +}
> > +#else
> > +static int applesmc_read_host_osk(char **output_key)
> > +{
> > +    warn_report("isa-applesmc.hostosk ignored: "
> > +                "unsupported on non-Apple hosts");
>
> I think a failure would be better than a warning.
> Moreover will it work if the user doenst provide the OSK?
>
>
Not sure that failure is more suitable than warning. See below [0].

> +    return -1;
> > +}
> > +#endif
> > +
> >   static void applesmc_isa_realize(DeviceState *dev, Error **errp)
> >   {
> > -    AppleSMCState *s = APPLE_SMC(dev);
> > +    AppleSMCState   *s = APPLE_SMC(dev);
> > +    char            *host_osk;
> >
> >       memory_region_init_io(&s->io_data, OBJECT(s),
> &applesmc_data_io_ops, s,
> >                             "applesmc-data", 1);
> > @@ -331,6 +463,18 @@ static void applesmc_isa_realize(DeviceState *dev,
> Error **errp)
> >       isa_register_ioport(&s->parent_obj, &s->io_err,
> >                           s->iobase + APPLESMC_ERR_PORT);
> >
> > +    /* Key retrieved from host SMC overwrites provided OSK string */
> > +    if (s->hostosk_flag
> > +        && !strcmp("on", s->hostosk_flag)
>
> Use a bool property for hostosk (see below), that will allow "on", "yes",
> "true", ... and here
> you'll only have to test for the boolean value.
>
> +        && !applesmc_read_host_osk(&host_osk)) {
> > +        if (s->osk) {
> > +            warn_report("provided isa-applesmc.osk "
> > +                        "is overwritten with host OSK");
> > +            g_free(s->osk);
> > +        }
> > +        s->osk = host_osk;
> > +    }
> > +
> >       if (!s->osk || (strlen(s->osk) != 64)) {
> >           warn_report("Using AppleSMC with invalid key");

>           s->osk = default_osk;
>

[0] The behavior of `osk` property handle: when the wrong OSK is provided
(or not provided at all) isa-applesmc uses `default_osk` and continues to
work fine.
Only a warning is printed.
Seems it's better to meet this "rule": when we can't read OSK from host-SMC
just
warn the user and continue with `default_osk`.



> > @@ -344,6 +488,7 @@ static Property applesmc_isa_properties[] = {
> >       DEFINE_PROP_UINT32(APPLESMC_PROP_IO_BASE, AppleSMCState, iobase,
> >                          APPLESMC_DEFAULT_IOBASE),
> >       DEFINE_PROP_STRING("osk", AppleSMCState, osk),
> > +    DEFINE_PROP_STRING("hostosk", AppleSMCState, hostosk_flag),
>
> An DEFINE_PROP_BOOL() would be more appropriate for this.
>
>
Will fix the property in the next patch version, thank you!

>       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> >
>
> Thanks,
> Laurent
>
>

-- 
Best Regards,

Vladislav Yaroshchuk
Re: [PATCH v3] isa-applesmc: provide OSK forwarding on Apple hosts
Posted by Laurent Vivier 2 years, 5 months ago
Le 22/10/2021 à 18:07, Vladislav Yaroshchuk a écrit :
> Hi, Laurent!
> Thank you for your review!
> 
> пт, 22 окт. 2021 г. в 14:36, Laurent Vivier <laurent@vivier.eu <mailto:laurent@vivier.eu>>:
> 
...
>      > +        && !applesmc_read_host_osk(&host_osk)) {
>      > +        if (s->osk) {
>      > +            warn_report("provided isa-applesmc.osk "
>      > +                        "is overwritten with host OSK");
>      > +            g_free(s->osk);
>      > +        }
>      > +        s->osk = host_osk;
>      > +    }
>      > +
>      >       if (!s->osk || (strlen(s->osk) != 64)) {
>      >           warn_report("Using AppleSMC with invalid key");
> 
>      >           s->osk = default_osk;
> 
> [0] The behavior of `osk` property handle: when the wrong OSK is provided
> (or not provided at all) isa-applesmc uses `default_osk` and continues to work fine.
> Only a warning is printed.
> Seems it's better to meet this "rule": when we can't read OSK from host-SMC just
> warn the user and continue with `default_osk`.

Ok, it sounds reasonable.

Thanks,
Laurent