Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf <graf@amazon.com>]

On 25.02.22 15:12, Jason A. Donenfeld wrote:
> Hi Alex,
>
> On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
>>> +static const struct acpi_device_id vmgenid_ids[] = {
>>> +       { "VMGENID", 0 },
>>> +       { "QEMUVGID", 0 },
>>
>> According to the VMGenID spec[1], you can only rely on _CID and _DDN for
>> matching. They both contain "VM_Gen_Counter". The list above contains
>> _HID values which are not an official identifier for the VMGenID device.
>>
>> IIRC the ACPI device match logic does match _CID in addition to _HID.
>> However, it is limited to 8 characters. Let me paste an experimental
>> hack I did back then to do the _CID matching instead.
>>
>> [1]
>> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>>
>>
>> Alex
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 1682f8b454a2..452443d79d87 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device
>> *device,
>>            /* First, check the ACPI/PNP IDs provided by the caller. */
>>            if (acpi_ids) {
>>                for (id = acpi_ids; id->id[0] || id->cls; id++) {
>> -                if (id->id[0] && !strcmp((char *)id->id, hwid->id))
>> +                if (id->id[0] && !strncmp((char *)id->id, hwid->id,
>> ACPI_ID_LEN - 1))
>>                        goto out_acpi_match;
>>                    if (id->cls && __acpi_match_device_cls(id, hwid))
>>                        goto out_acpi_match;
>> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
>> index 75a787da8aad..0bfa422cf094 100644
>> --- a/drivers/virt/vmgenid.c
>> +++ b/drivers/virt/vmgenid.c
>> @@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device
>> *device, u32 event)
>>    }
>>
>>    static const struct acpi_device_id vmgenid_ids[] = {
>> -    {"QEMUVGID", 0},
>> +    /* This really is VM_Gen_Counter, but we can only match 8 characters */
>> +    {"VM_GEN_C", 0},
>>        {"", 0},
>>    };
> I recall this part of the old thread. From what I understood, using
> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
> technically in-spec. Ard noted that relying on _CID like that is
> technically an ACPI spec notification. So we're between one spec and
> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
> changes, as mentioned, appears to work fine in my testing.
>
> However, with that said, I think supporting this via "VM_Gen_Counter"
> would be a better eventual thing to do, but will require acks and
> changes from the ACPI maintainers. Do you think you could prepare your
> patch proposal above as something on-top of my tree [1]? And if you can
> convince the ACPI maintainers that that's okay, then I'll happily take
> the patch.


Sure, let me send the ACPI patch stand alone. No need to include the 
VMGenID change in there.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879