Re: [Qemu-devel] [PATCH] virtio-pci: Don't force Subsystem Vendor ID = Vendor ID

[Ladi Prosek]

On Thu, Nov 2, 2017 at 3:52 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Nov 02, 2017 at 02:31:15PM +0100, Ladi Prosek wrote:
>> The statement being removed doesn't change anything as virtio PCI devices 
>> already
>> have Subsystem Vendor ID set to pci_default_sub_vendor_id (0x1af4), same as 
>> Vendor
>> ID. And the Virtio spec does not require the two to be equal, either:
>>
>>   "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)."
>>
>> Background:
>>
>> Following the recent virtio-win licensing change, several vendors are 
>> planning to
>> ship their own certified version of Windows guest Virtio drivers, 
>> potentially taking
>> advantage of Windows Update as a distribution channel. It is therefore 
>> critical that
>> each vendor uses their own PCI Subsystem Vendor ID for Virtio devices to 
>> prevent
>> drivers from other vendors binding to it.
>>
>> This would be trivially done by adding:
>>
>>   k->subsystem_vendor_id = ...
>>
>> to virtio_pci_class_init(). Except for the problematic statement deleted by 
>> this
>> patch, which reverts the Subsystem Vendor ID back to 0x1af4 for legacy 
>> devices for
>> no good reason.
>>
>> Signed-off-by: Ladi Prosek <address@hidden>
>
> I wonder whether it's a problem that legacy devices ignore
> the subsystem ID (that's part of spec).

I don't understand this comment. I don't see anything in the spec
related to ignoring the subsystem ID.

>> ---
>>  hw/virtio/virtio-pci.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index e92837c42b..cb74aa3d85 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1589,8 +1589,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
>> Error **errp)
>>              return ;
>>          }
>>          /* legacy and transitional */
>> -        pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
>> -                     pci_get_word(config + PCI_VENDOR_ID));
>
>
> This happens to work because the default subsystem vendor id
> within qemu is 0x1af4. Let's add a comment to that end.

Sure, will do.

>>          pci_set_word(config + PCI_SUBSYSTEM_ID, 
>> virtio_bus_get_vdev_id(bus));
>>      } else {
>>          /* pure virtio-1.0 */
>> --
>> 2.13.5