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é <berra...@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-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 :|
>
>
