[Libguestfs] [COMMON PATCH v2 1/4] inject_virtio_win: match only vendor/device

[Andrey Drobyshev]

On 3/8/23 12:47, Laszlo Ersek wrote:
> On 3/7/23 20:40, Andrey Drobyshev wrote:
>> From: Roman Kagan <rkagan at virtuozzo.com>
>>
>> Since different hypervisor vendors are allowed to use their own vendor-id
>> as PCI subsystem-vendor-id for virtio devices, during v2v conversion it
>> makes sense to only match the vendor/device and not the full device "path"
>> in the Windows registry.  This way the code will remain universal but will
>> work for different hypervisor vendors.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
>> Originally-by: Roman Kagan <rkagan at virtuozzo.com>
>> ---
>>  mlcustomize/inject_virtio_win.ml | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml
>> index 4e977b3..8d72c5d 100644
>> --- a/mlcustomize/inject_virtio_win.ml
>> +++ b/mlcustomize/inject_virtio_win.ml
>> @@ -110,10 +110,10 @@ and get_inspection g root =
>>      virtio_win = ""; was_set = false }
>>  
>>  let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}"
>> -let viostor_legacy_pciid = "VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00"
>> -let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01"
>> -let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00"
>> -let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01"
>> +let viostor_legacy_pciid = "VEN_1AF4&DEV_1001"
>> +let viostor_modern_pciid = "VEN_1AF4&DEV_1042"
>> +let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004"
>> +let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048"
>>  
>>  let rec inject_virtio_win_drivers ({ g } as t) reg =
>>    (* Copy the virtio drivers to the guest. *)
> 
> I wonder whether dropping the revision ID as well is a good idea. In the
> virtio-1.0 spec, we have under 4.1.2.1 Device Requirements: PCI Device
> Discovery:
> 
>     The PCI Subsystem Vendor ID and the PCI Subsystem Device ID MAY
>     reflect the PCI Vendor and Device ID of the environment (for
>     informational purposes by the driver).
> 
>     Non-transitional devices SHOULD have a PCI Device ID in the range
>     0x1040 to 0x107f. Non-transitional devices SHOULD have a PCI
>     Revision ID of 1 or higher. Non-transitional devices SHOULD have a
>     PCI Subsystem Device ID of 0x40 or higher.
> 
> And in virtio-0.9.5, we have under 2.1 PCI Discovery:
> 
>     [...] The device must also have a Revision ID of 0 to
>     match this speci
cation. [...]
> 
> So I think relaxing the PCI Revision ID is not necessary. But, it
> shouldn't cause problems in practice either, I think. It won't allow for
> cross-binding (the PCI Device IDs remain distinct).
> 
> Acked-by: Laszlo Ersek <lersek at redhat.com>
> 

I agree that omitting both Subsystem Vendor ID and Revision ID isn't an
obvious solution.  Since each subsystem is allowed to have its own
Vendor ID, an alternative would be to determine on the fly which
hypervisor we're dealing with, and choose one of the predefined Vendor
IDs accordingly.  I figured that skipping them entirely shouldn't do any
harm, and thus this solution is more practical.

As for the Revision IDs, relaxing them indeed doesn't seem to be
necessary.  I have now tried leaving them there as is, and VM is still
able to boot off that controller as expected.  Like so:

> @@ -110,10 +110,10 @@ and get_inspection g root =
>      virtio_win = ""; was_set = false }
>  
>  let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}"
> -let viostor_legacy_pciid = "VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00"
> -let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01"
> -let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00"
> -let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01"
> +let viostor_legacy_pciid = "VEN_1AF4&DEV_1001&REV_00"
> +let viostor_modern_pciid = "VEN_1AF4&DEV_1042&REV_01"
> +let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&REV_00"
> +let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&REV_01"
>  
>  let rec inject_virtio_win_drivers ({ g } as t) reg =
>    (* Copy the virtio drivers to the guest. *)

So we may as well not omit them, if you prefer.