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, ¶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-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