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

[Vladislav Yaroshchuk]

Hi, Daniel!

Ok, thank you - then I'll wait until compat machines
for 7.1 are added (after release) and send a new
patch.

Regards,

Vladislav

вт, 19 апр. 2022 г. в 19:03, Daniel P. Berrangé <berrange@redhat.com>:

On Sun, Apr 17, 2022 at 04:43:14PM +0300, Vladislav Yaroshchuk wrote:
> 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)

I prefer yours, since the feature is introspectable by mgmt apps,
given the existance of the 'hostosk' property

With regards,
Daniel
--
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|