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-yaroshchuk2...@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, ¶ms_in, size,
> ¶ms_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-yaroshchuk2...@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 :|
