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

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

[Ladi Prosek]

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 <lprosek@redhat.com>
---
 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));
         pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
     } else {
         /* pure virtio-1.0 */
-- 
2.13.5

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

[Michael S. Tsirkin]

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 <lprosek@redhat.com>

I wonder whether it's a problem that legacy devices ignore
the subsystem ID (that's part of spec).

> ---
>  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.

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

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 <mst@redhat.com> 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 <lprosek@redhat.com>
>
> 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

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

[Gerd Hoffmann]

> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> > 
> > 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.

Well, the subsystem *device* id is defined to be the virtio device id,
so it is certainly not ignored.  The subsystem *vendor* id is not used
as far I know (or ignored in the sense that it doesn't change driver
behavior), allowing to set that makes sense to me.

Possibly not only for virtio devices, most pci devices have 1af4:1100
as subsystem id, other vendors might want set it too for consistency.

cheers,
  Gerd

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

[Ladi Prosek]

On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> >
>> > 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.
>
> Well, the subsystem *device* id is defined to be the virtio device id,
> so it is certainly not ignored.  The subsystem *vendor* id is not used
> as far I know (or ignored in the sense that it doesn't change driver
> behavior), allowing to set that makes sense to me.

Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
The PCI spec seems to be using "subsystem ID" for the name of this
field. I understand that this ID must not change in legacy devices.

Interestingly, Windows appears to allow matching drivers only on the
full subsystem device ID + subsystem vendor ID 32-bit value, not on
only one of the two.

PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)

This might be a potential problem for a legacy driver that wants to
stay vendor-agnostic but I'm pretty sure there would be a reasonable
way of working around it. Actually, the device must use a designated
device ID (like 0x1000) in addition to the subsystem device ID so this
should be a non-issue altogether.

> Possibly not only for virtio devices, most pci devices have 1af4:1100
> as subsystem id, other vendors might want set it too for consistency.
>
> cheers,
>   Gerd

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

[Vadim Rozenfeld]

On 03/11/17 19:23, Ladi Prosek wrote:
> On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>> 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.
>> Well, the subsystem *device* id is defined to be the virtio device id,
>> so it is certainly not ignored.  The subsystem *vendor* id is not used
>> as far I know (or ignored in the sense that it doesn't change driver
>> behavior), allowing to set that makes sense to me.
> Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
> The PCI spec seems to be using "subsystem ID" for the name of this
> field. I understand that this ID must not change in legacy devices.
>
> Interestingly, Windows appears to allow matching drivers only on the
> full subsystem device ID + subsystem vendor ID 32-bit value, not on
> only one of the two.
>
> PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)
>
> This might be a potential problem for a legacy driver that wants to
> stay vendor-agnostic but I'm pretty sure there would be a reasonable
> way of working around it. Actually, the device must use a designated
> device ID (like 0x1000) in addition to the subsystem device ID so this
> should be a non-issue altogether.
>
For legacy drivers running on modified qemu it can be a problem.
For new virtio-win build we can keep both compatible (VID/DID) together
with full (VID/DID/SID/SVID) specified in .inf file

>> Possibly not only for virtio devices, most pci devices have 1af4:1100
>> as subsystem id, other vendors might want set it too for consistency.
>>
>> cheers,
>>    Gerd

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

[Michael S. Tsirkin]

On Fri, Nov 03, 2017 at 09:23:07AM +0100, Ladi Prosek wrote:
> On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> >
> >> > 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.
> >
> > Well, the subsystem *device* id is defined to be the virtio device id,
> > so it is certainly not ignored.  The subsystem *vendor* id is not used
> > as far I know (or ignored in the sense that it doesn't change driver
> > behavior), allowing to set that makes sense to me.
> 
> Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
> The PCI spec seems to be using "subsystem ID" for the name of this
> field.

Exactly.

> I understand that this ID must not change in legacy devices.

Interestingly the device ID is ignored except it must be within
a specific range of values.

> Interestingly, Windows appears to allow matching drivers only on the
> full subsystem device ID + subsystem vendor ID 32-bit value, not on
> only one of the two.
> 
> PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)
> 
> This might be a potential problem for a legacy driver that wants to
> stay vendor-agnostic but I'm pretty sure there would be a reasonable
> way of working around it. Actually, the device must use a designated
> device ID (like 0x1000) in addition to the subsystem device ID so this
> should be a non-issue altogether.

The original virtio spec wasn't really workable for windows. It requires
ignoring everything except the vendor ID, *range* of device IDs
(specific ID is ignored) and subsystem ID.

So in my humble opinion the right thing for people to do is simply to
avoid legacy devices. Is something preventing that?

> > Possibly not only for virtio devices, most pci devices have 1af4:1100
> > as subsystem id, other vendors might want set it too for consistency.
> >
> > cheers,
> >   Gerd

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

[Ladi Prosek]

On Fri, Nov 3, 2017 at 4:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Nov 03, 2017 at 09:23:07AM +0100, Ladi Prosek wrote:
>> On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >
>> >> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> >> >
>> >> > 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.
>> >
>> > Well, the subsystem *device* id is defined to be the virtio device id,
>> > so it is certainly not ignored.  The subsystem *vendor* id is not used
>> > as far I know (or ignored in the sense that it doesn't change driver
>> > behavior), allowing to set that makes sense to me.
>>
>> Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
>> The PCI spec seems to be using "subsystem ID" for the name of this
>> field.
>
> Exactly.
>
>> I understand that this ID must not change in legacy devices.
>
> Interestingly the device ID is ignored except it must be within
> a specific range of values.
>
>> Interestingly, Windows appears to allow matching drivers only on the
>> full subsystem device ID + subsystem vendor ID 32-bit value, not on
>> only one of the two.
>>
>> PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)
>>
>> This might be a potential problem for a legacy driver that wants to
>> stay vendor-agnostic but I'm pretty sure there would be a reasonable
>> way of working around it. Actually, the device must use a designated
>> device ID (like 0x1000) in addition to the subsystem device ID so this
>> should be a non-issue altogether.
>
> The original virtio spec wasn't really workable for windows. It requires
> ignoring everything except the vendor ID, *range* of device IDs
> (specific ID is ignored) and subsystem ID.

The original spec has been superseded though, no? The current spec
prescribes fixed device IDs for transitional devices (4.1.2.1 Device
Requirements: PCI Device Discovery).

> So in my humble opinion the right thing for people to do is simply to
> avoid legacy devices. Is something preventing that?

The same reasons why the concept of transitional devices exists at all?

Anyway, I'm not quite sure why we're spending so much time discussing
this. The spec says "subsystem vendor MAY change". QEMU has this
unfortunate statement which makes it not as straightforward as it
should be. Removing the statement has zero impact on QEMU behavior. So
we either remove it now or whoever actually needs to change the
subsystem vendor ID will do it.

>> > Possibly not only for virtio devices, most pci devices have 1af4:1100
>> > as subsystem id, other vendors might want set it too for consistency.
>> >
>> > cheers,
>> >   Gerd

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

[Gerd Hoffmann]

  Hi,

> > So in my humble opinion the right thing for people to do is simply
> > to
> > avoid legacy devices. Is something preventing that?
> 
> The same reasons why the concept of transitional devices exists at
> all?

We discussing future driver versions running on future qemu versions,
so we should have virtio 1.0 support on both ends.  So IMO the question
 isn't that silly ...

But by default qemu uses transitional by default for non-express
devices, i.e. for all i440fx machines types.  Which implies virtio-
legacy compatible pci ids.

If we want change this we need a transitional/modern virtio config
switch.  Not only in qemu, but for the whole management stack too.
I don't think it is a good idea to go that route.

cheers,
  Gerd

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

[Michael S. Tsirkin]

On Mon, Nov 06, 2017 at 10:18:07AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > So in my humble opinion the right thing for people to do is simply
> > > to
> > > avoid legacy devices. Is something preventing that?
> > 
> > The same reasons why the concept of transitional devices exists at
> > all?
> 
> We discussing future driver versions running on future qemu versions,
> so we should have virtio 1.0 support on both ends.  So IMO the question
>  isn't that silly ...
> 
> But by default qemu uses transitional by default for non-express
> devices, i.e. for all i440fx machines types.  Which implies virtio-
> legacy compatible pci ids.
> 
> If we want change this we need a transitional/modern virtio config
> switch.  Not only in qemu, but for the whole management stack too.
> I don't think it is a good idea to go that route.
> 
> cheers,
>   Gerd

I think we already have that in libvirt.

Right now we just ignore the subsystem IDs if supplied but
maybe the right thing to do is to just disable legacy
mode if they are supplied.

Seems cleaner than ignoring the subsystem id but using the
subsystem vendor id.

-- 
MST

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

[Michael S. Tsirkin]

On Mon, Nov 06, 2017 at 10:02:54AM +0100, Ladi Prosek wrote:
> On Fri, Nov 3, 2017 at 4:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Nov 03, 2017 at 09:23:07AM +0100, Ladi Prosek wrote:
> >> On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> >
> >> >> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> >> >
> >> >> > 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.
> >> >
> >> > Well, the subsystem *device* id is defined to be the virtio device id,
> >> > so it is certainly not ignored.  The subsystem *vendor* id is not used
> >> > as far I know (or ignored in the sense that it doesn't change driver
> >> > behavior), allowing to set that makes sense to me.
> >>
> >> Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
> >> The PCI spec seems to be using "subsystem ID" for the name of this
> >> field.
> >
> > Exactly.
> >
> >> I understand that this ID must not change in legacy devices.
> >
> > Interestingly the device ID is ignored except it must be within
> > a specific range of values.
> >
> >> Interestingly, Windows appears to allow matching drivers only on the
> >> full subsystem device ID + subsystem vendor ID 32-bit value, not on
> >> only one of the two.
> >>
> >> PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)
> >>
> >> This might be a potential problem for a legacy driver that wants to
> >> stay vendor-agnostic but I'm pretty sure there would be a reasonable
> >> way of working around it. Actually, the device must use a designated
> >> device ID (like 0x1000) in addition to the subsystem device ID so this
> >> should be a non-issue altogether.
> >
> > The original virtio spec wasn't really workable for windows. It requires
> > ignoring everything except the vendor ID, *range* of device IDs
> > (specific ID is ignored) and subsystem ID.
> 
> The original spec has been superseded though, no? The current spec
> prescribes fixed device IDs for transitional devices (4.1.2.1 Device
> Requirements: PCI Device Discovery).

I agree, it's reasonable to ignore this part in the old spec.


> > So in my humble opinion the right thing for people to do is simply to
> > avoid legacy devices. Is something preventing that?
> 
> The same reasons why the concept of transitional devices exists at all?

It's there to support old drivers. If you are changing vendor id
that is supposedly so you can ship new drivers.

> Anyway, I'm not quite sure why we're spending so much time discussing
> this. The spec says "subsystem vendor MAY change". QEMU has this
> unfortunate statement which makes it not as straightforward as it
> should be. Removing the statement has zero impact on QEMU behavior. So
> we either remove it now or whoever actually needs to change the
> subsystem vendor ID will do it.

I am fine with your patch as such, but I just realized that anyone
tweaking subsystem vendor id in the past will get a different value now.

Worth worrying about or do we need to make it depend on
a machine type?


> >> > Possibly not only for virtio devices, most pci devices have 1af4:1100
> >> > as subsystem id, other vendors might want set it too for consistency.
> >> >
> >> > cheers,
> >> >   Gerd

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

[Ladi Prosek]

On Mon, Nov 6, 2017 at 5:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Nov 06, 2017 at 10:02:54AM +0100, Ladi Prosek wrote:
>> On Fri, Nov 3, 2017 at 4:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Nov 03, 2017 at 09:23:07AM +0100, Ladi Prosek wrote:
>> >> On Fri, Nov 3, 2017 at 8:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >> >
>> >> >> > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> >> >> >
>> >> >> > 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.
>> >> >
>> >> > Well, the subsystem *device* id is defined to be the virtio device id,
>> >> > so it is certainly not ignored.  The subsystem *vendor* id is not used
>> >> > as far I know (or ignored in the sense that it doesn't change driver
>> >> > behavior), allowing to set that makes sense to me.
>> >>
>> >> Yes, thanks, I'm assuming that Michael meant the subsystem device ID.
>> >> The PCI spec seems to be using "subsystem ID" for the name of this
>> >> field.
>> >
>> > Exactly.
>> >
>> >> I understand that this ID must not change in legacy devices.
>> >
>> > Interestingly the device ID is ignored except it must be within
>> > a specific range of values.
>> >
>> >> Interestingly, Windows appears to allow matching drivers only on the
>> >> full subsystem device ID + subsystem vendor ID 32-bit value, not on
>> >> only one of the two.
>> >>
>> >> PCI\VEN_v(4)&DEV_d(4)&SUBSYS_s(4)n(4)&REV_r(2)
>> >>
>> >> This might be a potential problem for a legacy driver that wants to
>> >> stay vendor-agnostic but I'm pretty sure there would be a reasonable
>> >> way of working around it. Actually, the device must use a designated
>> >> device ID (like 0x1000) in addition to the subsystem device ID so this
>> >> should be a non-issue altogether.
>> >
>> > The original virtio spec wasn't really workable for windows. It requires
>> > ignoring everything except the vendor ID, *range* of device IDs
>> > (specific ID is ignored) and subsystem ID.
>>
>> The original spec has been superseded though, no? The current spec
>> prescribes fixed device IDs for transitional devices (4.1.2.1 Device
>> Requirements: PCI Device Discovery).
>
> I agree, it's reasonable to ignore this part in the old spec.
>
>
>> > So in my humble opinion the right thing for people to do is simply to
>> > avoid legacy devices. Is something preventing that?
>>
>> The same reasons why the concept of transitional devices exists at all?
>
> It's there to support old drivers. If you are changing vendor id
> that is supposedly so you can ship new drivers.

That's true. And for Windows guests specifically we know that the
license change happened long after modern Virtio was implemented and
stabilized so vendors should be able to eliminate legacy Virtio.

I can imagine that it will still be more common to expose transitional
devices for some time, though. Like in all kinds of cloud set ups
where it's maybe not practical to change the configuration of virtual
devices based on the guest OS and maximum compatibility outweighs the
advantages of modern Virtio.

>> Anyway, I'm not quite sure why we're spending so much time discussing
>> this. The spec says "subsystem vendor MAY change". QEMU has this
>> unfortunate statement which makes it not as straightforward as it
>> should be. Removing the statement has zero impact on QEMU behavior. So
>> we either remove it now or whoever actually needs to change the
>> subsystem vendor ID will do it.
>
> I am fine with your patch as such, but I just realized that anyone
> tweaking subsystem vendor id in the past will get a different value now.
>
> Worth worrying about or do we need to make it depend on
> a machine type?

If subsystem vendor ID could be tweaked without a code change, then
yes, we wouldn't want to break anyone. But private code changes have
the inherent risk of being broken by upstream changes; I'd say we
shouldn't worry about that.

>> >> > Possibly not only for virtio devices, most pci devices have 1af4:1100
>> >> > as subsystem id, other vendors might want set it too for consistency.
>> >> >
>> >> > cheers,
>> >> >   Gerd

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

[Michael S. Tsirkin]

On Fri, Nov 03, 2017 at 07:25:16AM +0100, Ladi Prosek wrote:
> On Thu, Nov 2, 2017 at 3:52 PM, Michael S. Tsirkin <mst@redhat.com> 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 <lprosek@redhat.com>
> >
> > 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.

The code just below your patch overwrites the subsystem ID even if
supplied by user, and the reason is that legacy drivers
relied on this.


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