Re: [PATCH v3] hw/misc: applesmc: use host osk as default on macs

[Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]

I've CCed all the people from previous threads.


> [...]
> +static bool applesmc_read_osk(uint8_t *osk)
> +{
> +#if defined(__APPLE__) && defined(__MACH__)
> +    struct AppleSMCParams {
> +        uint32_t key;
> +        uint8_t __pad0[16];
> +        uint8_t result;
> +        uint8_t __pad1[7];
> +        uint32_t size;
> +        uint8_t __pad2[10];
> +        uint8_t data8;
> +        uint8_t __pad3[5];
> +        uint8_t output[32];
> +    };
> +
> +    io_service_t svc;
> +    io_connect_t conn;
> +    kern_return_t ret;
> +    size_t size = sizeof(struct AppleSMCParams);
> +    struct AppleSMCParams params_in = { .size = 32, .data8 = 5 };

Maybe it's better to name this magic number '5'

> +    struct AppleSMCParams params_out = {};
> +

params_in and params_out can be the same variable, see
https://patchew.org/QEMU/20211022161448.81579-1-yaroshchuk2000@gmail.com/

> +    svc = IOServiceGetMatchingService(0, IOServiceMatching("AppleSMC"));
> +    if (svc == 0) {
> +        return false;
> +    }
> +
> +    ret = IOServiceOpen(svc, mach_task_self(), 0, &conn);
> +    if (ret != 0) {
> +        return false;
> +    }
> +
> +    for (params_in.key = 'OSK0'; params_in.key <= 'OSK1';
++params_in.key) {
> +        ret = IOConnectCallStructMethod(conn, 2, &params_in, size,
&params_out, &size);

Same about this magic number '2'.

> +        if (ret != 0) {
> +            return false;
> +        }
> +
> +        if (params_out.result != 0) {
> +            return false;
> +        }
> +        memcpy(osk, params_out.output, params_in.size);
> +
> +        osk += params_in.size;
> +     }
> +

Cleanup IOServiceClose and IOObjectReturn are not called at the
end of the procedure.

This is also mentioned in Phil Dennis-Jordan's instruction you noted (stage
5):
https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg02843.html

> +    return true;
> +#else
> +    return false;
> +#endif
> +}
> +
> [...]
>
> static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>  {
>      AppleSMCState *s = APPLE_SMC(dev);
> +    bool valid_osk = false;
>
>      memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops,
s,
>                            "applesmc-data", 1);
> @@ -331,8 +393,17 @@ static void applesmc_isa_realize(DeviceState *dev,
Error **errp)
>      isa_register_ioport(&s->parent_obj, &s->io_err,
>                          s->iobase + APPLESMC_ERR_PORT);
>
> -    if (!s->osk || (strlen(s->osk) != 64)) {
> -        warn_report("Using AppleSMC with invalid key");
> +    if (s->osk) {
> +        valid_osk = strlen(s->osk) == 64;
> +    } else {
> +        valid_osk = applesmc_read_osk((uint8_t *) default_osk);
> +        if (valid_osk) {
> +            warn_report("Using AppleSMC with host OSK");
> +            s->osk = default_osk;
> +        }
> +    }
> +    if (!valid_osk) {
> +        warn_report("Using AppleSMC with invalid OSK");
>          s->osk = default_osk;
>      }
> [...]

After the previous discussion we've decided (if i don't confuse anything)
to have a way to enable/disable host OSK reading with new property:
1. properties osk=$key and hostosk=on cannot be used together (fail hard)
2. for QEMU machine > 7.0 - hostosk=on by default.
    If unable to read - fail hard with error_setg.
3. for QEMU machine <= 7.0 - hostosk=off by default,
   the dummy OSK is used (as previously).

BTW since my patches lost 7.0, I planned to wait until compat machines
for 7.1 are added (after 7.0 release) and then rebase the patches,
adding required changes into `hw/core/machine.c`

Now we have two versions of host OSK forwarding implementations,
Pedro's (this one) and mine (
https://patchew.org/QEMU/20220113152836.60398-1-yaroshchuk2000@gmail.com/#)

Do we still want to add this feature? If yes - whose version is preferred?
(I'm still ready to work on this)

Regards,
Vlad

вс, 17 апр. 2022 г. в 04:36, Vladislav Yaroshchuk <yaroshchuk2000@gmail.com
>:

> Hi Pedro Torres,
>
> Please note this threads
> https://patchew.org/QEMU/20211022161448.81579-1-yaroshchuk2000@gmail.com/
> https://patchew.org/QEMU/20220113152836.60398-1-yaroshchuk2000@gmail.com/
>
> There was a discussion about how to preserve
> backward compatibility of emulated AppleSMC
> behaviour. The discussion has stopped, but
> if you're intended to see this feature working
> within QEMU - it'd be good to cc all the
> participans of previous threads :)
>
>
> Thanks,
>
> Vladislav Yaroshchuk
>
>

[-- Attachment #2: Type: text/html, Size: 6425 bytes --]