* [RFC] virtio-net: support modern-transtional devices
@ 2023-05-26 18:15 Zhu Lingshan
2023-05-28 11:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Zhu Lingshan @ 2023-05-26 18:15 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
Current virtio-net only probes a device with VIRITO_ID_NET == 1.
For a modern-transtional virtio-net device which has a transtional
device id 0x1000 and acts as a modern device, current virtio-pci
modern driver will assign the sub-device-id to its mdev->id.device,
which may not be 0x1, this sub-device-id is up to the vendor.
That means virtio-net driver doesn't probe a modern-transitonal
virtio-net with a sub-device-id other than 0x1, which is a bug.
Other types of devices also have similar issues, like virito-blk.
I propose to fix this problem of modern-transitonal device
whith this solution, all in the modern code path:
1) assign the device id to mdev->id.device
2) add transitional device ids in the virtio-net(and others) probe table.
Comments are welcome!
Thanks!
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
drivers/net/virtio_net.c | 1 +
drivers/virtio/virtio_pci_modern_dev.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 56ca1d270304..6b45d8602a6b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
+ { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
};
diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index 869cb46bef96..80846e1195ce 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
/* Transitional devices: use the PCI subsystem device id as
* virtio device id, same as legacy driver always did.
*/
- mdev->id.device = pci_dev->subsystem_device;
+ mdev->id.device = pci_dev->device;
} else {
/* Modern devices: simply use PCI device id, but start from 0x1040. */
mdev->id.device = pci_dev->device - 0x1040;
--
2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-26 18:15 [RFC] virtio-net: support modern-transtional devices Zhu Lingshan
@ 2023-05-28 11:28 ` Michael S. Tsirkin
2023-05-29 6:19 ` Zhu, Lingshan
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-05-28 11:28 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: virtualization
On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>
> For a modern-transtional virtio-net device which has a transtional
> device id 0x1000 and acts as a modern device, current virtio-pci
> modern driver will assign the sub-device-id to its mdev->id.device,
> which may not be 0x1, this sub-device-id is up to the vendor.
>
> That means virtio-net driver doesn't probe a modern-transitonal
> virtio-net with a sub-device-id other than 0x1, which is a bug.
No, the bug is in the device. Legacy linux drivers always looked at
sub device id (other OSes might differ). So it makes no sense
for a transitional device to have sub-device-id other than 0x1.
Don't have time to look at spec but I think you will find it there.
> Other types of devices also have similar issues, like virito-blk.
>
> I propose to fix this problem of modern-transitonal device
> whith this solution, all in the modern code path:
> 1) assign the device id to mdev->id.device
> 2) add transitional device ids in the virtio-net(and others) probe table.
>
> Comments are welcome!
>
> Thanks!
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/net/virtio_net.c | 1 +
> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 56ca1d270304..6b45d8602a6b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
> { 0 },
> };
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index 869cb46bef96..80846e1195ce 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> /* Transitional devices: use the PCI subsystem device id as
> * virtio device id, same as legacy driver always did.
> */
> - mdev->id.device = pci_dev->subsystem_device;
> + mdev->id.device = pci_dev->device;
> } else {
> /* Modern devices: simply use PCI device id, but start from 0x1040. */
> mdev->id.device = pci_dev->device - 0x1040;
> --
> 2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-28 11:28 ` Michael S. Tsirkin
@ 2023-05-29 6:19 ` Zhu, Lingshan
2023-05-29 6:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Zhu, Lingshan @ 2023-05-29 6:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
> On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
>> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>>
>> For a modern-transtional virtio-net device which has a transtional
>> device id 0x1000 and acts as a modern device, current virtio-pci
>> modern driver will assign the sub-device-id to its mdev->id.device,
>> which may not be 0x1, this sub-device-id is up to the vendor.
>>
>> That means virtio-net driver doesn't probe a modern-transitonal
>> virtio-net with a sub-device-id other than 0x1, which is a bug.
> No, the bug is in the device. Legacy linux drivers always looked at
> sub device id (other OSes might differ). So it makes no sense
> for a transitional device to have sub-device-id other than 0x1.
> Don't have time to look at spec but I think you will find it there.
That is true for a software emulated transitional device,
because there is only "generation" of instance in the hypervisor,
that allowing it to ensure its sub-device-id always be 0x01,
and it fits VIRTIO_ID_NET.
However, a vendor may produce multiple generations of transitional
hardware. The sub-device-id is up to the vendor, and it is the
only way to for a driver to identify a device, other IDs are all
fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
So the sub-device-id has to be unique and differ from others, can not
always be 0x01.
I propose this fix, all changes are for modern-transitional devices in
modern
code path, not for legacy nor legacy-transitional.
Thanks
>
>
>> Other types of devices also have similar issues, like virito-blk.
>>
>> I propose to fix this problem of modern-transitonal device
>> whith this solution, all in the modern code path:
>> 1) assign the device id to mdev->id.device
>> 2) add transitional device ids in the virtio-net(and others) probe table.
>>
>> Comments are welcome!
>>
>> Thanks!
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/net/virtio_net.c | 1 +
>> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 56ca1d270304..6b45d8602a6b 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>>
>> static struct virtio_device_id id_table[] = {
>> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
>> { 0 },
>> };
>>
>> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
>> index 869cb46bef96..80846e1195ce 100644
>> --- a/drivers/virtio/virtio_pci_modern_dev.c
>> +++ b/drivers/virtio/virtio_pci_modern_dev.c
>> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>> /* Transitional devices: use the PCI subsystem device id as
>> * virtio device id, same as legacy driver always did.
>> */
>> - mdev->id.device = pci_dev->subsystem_device;
>> + mdev->id.device = pci_dev->device;
>> } else {
>> /* Modern devices: simply use PCI device id, but start from 0x1040. */
>> mdev->id.device = pci_dev->device - 0x1040;
>> --
>> 2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 6:19 ` Zhu, Lingshan
@ 2023-05-29 6:38 ` Michael S. Tsirkin
2023-05-29 8:07 ` Zhu, Lingshan
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-05-29 6:38 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: virtualization
On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
>
>
> On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
> > On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
> > > Current virtio-net only probes a device with VIRITO_ID_NET == 1.
> > >
> > > For a modern-transtional virtio-net device which has a transtional
> > > device id 0x1000 and acts as a modern device, current virtio-pci
> > > modern driver will assign the sub-device-id to its mdev->id.device,
> > > which may not be 0x1, this sub-device-id is up to the vendor.
> > >
> > > That means virtio-net driver doesn't probe a modern-transitonal
> > > virtio-net with a sub-device-id other than 0x1, which is a bug.
> > No, the bug is in the device. Legacy linux drivers always looked at
> > sub device id (other OSes might differ). So it makes no sense
> > for a transitional device to have sub-device-id other than 0x1.
> > Don't have time to look at spec but I think you will find it there.
> That is true for a software emulated transitional device,
> because there is only "generation" of instance in the hypervisor,
> that allowing it to ensure its sub-device-id always be 0x01,
> and it fits VIRTIO_ID_NET.
>
> However, a vendor may produce multiple generations of transitional
> hardware. The sub-device-id is up to the vendor, and it is the
> only way to for a driver to identify a device, other IDs are all
> fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
That is one of the issues with legacy virtio, yes.
> So the sub-device-id has to be unique and differ from others, can not always
> be 0x01.
If you are trying to build a device and want to create a safe way to
identify it without breaking legacy drivers, then
VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
For example you can have:
struct virtio_pci_vndr_data {
u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
u8 cap_next; /* Generic PCI field: next ptr. */
u8 cap_len; /* Generic PCI field: capability length */
u8 cfg_type; /* Identifies the structure. */
u16 vendor_id; /* Identifies the vendor-specific format. */
u16 device_generation; /* Device generation */
};
> I propose this fix, all changes are for modern-transitional devices in
> modern
> code path, not for legacy nor legacy-transitional.
>
> Thanks
But what good is this fix? If you just want the modern driver to bind
and ignore legacy just create a modern device, you can play
with subsystem id and vendor to your heart's content then.
If you are using transitional then presumably you want
legacy drives to bind, they will not bind if subsystem device
id changes.
> >
> >
> > > Other types of devices also have similar issues, like virito-blk.
> > >
> > > I propose to fix this problem of modern-transitonal device
> > > whith this solution, all in the modern code path:
> > > 1) assign the device id to mdev->id.device
> > > 2) add transitional device ids in the virtio-net(and others) probe table.
> > >
> > > Comments are welcome!
> > >
> > > Thanks!
> > >
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > > drivers/net/virtio_net.c | 1 +
> > > drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 56ca1d270304..6b45d8602a6b 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
> > > static struct virtio_device_id id_table[] = {
> > > { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> > > + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
> > > { 0 },
> > > };
> > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > > index 869cb46bef96..80846e1195ce 100644
> > > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > > @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > > /* Transitional devices: use the PCI subsystem device id as
> > > * virtio device id, same as legacy driver always did.
> > > */
> > > - mdev->id.device = pci_dev->subsystem_device;
> > > + mdev->id.device = pci_dev->device;
> > > } else {
> > > /* Modern devices: simply use PCI device id, but start from 0x1040. */
> > > mdev->id.device = pci_dev->device - 0x1040;
> > > --
> > > 2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 6:38 ` Michael S. Tsirkin
@ 2023-05-29 8:07 ` Zhu, Lingshan
2023-05-29 10:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Zhu, Lingshan @ 2023-05-29 8:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
> On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
>>
>> On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
>>> On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
>>>> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>>>>
>>>> For a modern-transtional virtio-net device which has a transtional
>>>> device id 0x1000 and acts as a modern device, current virtio-pci
>>>> modern driver will assign the sub-device-id to its mdev->id.device,
>>>> which may not be 0x1, this sub-device-id is up to the vendor.
>>>>
>>>> That means virtio-net driver doesn't probe a modern-transitonal
>>>> virtio-net with a sub-device-id other than 0x1, which is a bug.
>>> No, the bug is in the device. Legacy linux drivers always looked at
>>> sub device id (other OSes might differ). So it makes no sense
>>> for a transitional device to have sub-device-id other than 0x1.
>>> Don't have time to look at spec but I think you will find it there.
>> That is true for a software emulated transitional device,
>> because there is only "generation" of instance in the hypervisor,
>> that allowing it to ensure its sub-device-id always be 0x01,
>> and it fits VIRTIO_ID_NET.
>>
>> However, a vendor may produce multiple generations of transitional
>> hardware. The sub-device-id is up to the vendor, and it is the
>> only way to for a driver to identify a device, other IDs are all
>> fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
> That is one of the issues with legacy virtio, yes.
>
>
>
>> So the sub-device-id has to be unique and differ from others, can not always
>> be 0x01.
>
> If you are trying to build a device and want to create a safe way to
> identify it without breaking legacy drivers, then
> VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
> For example you can have:
>
> struct virtio_pci_vndr_data {
> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> u8 cap_next; /* Generic PCI field: next ptr. */
> u8 cap_len; /* Generic PCI field: capability length */
> u8 cfg_type; /* Identifies the structure. */
> u16 vendor_id; /* Identifies the vendor-specific format. */
> u16 device_generation; /* Device generation */
> };
This can be a solution for sure.
>
>> I propose this fix, all changes are for modern-transitional devices in
>> modern
>> code path, not for legacy nor legacy-transitional.
>>
>> Thanks
> But what good is this fix? If you just want the modern driver to bind
> and ignore legacy just create a modern device, you can play
> with subsystem id and vendor to your heart's content then.
Not sure who but there are some use-cases require
transnational devices than modern devices,
I don't like this neither.
>
> If you are using transitional then presumably you want
> legacy drives to bind, they will not bind if subsystem device
> id changes.
well actually it is a transitional device and act as a
modern device by default, so modern driver will probe.
I think this fix is common and easy, just let virtio-net
probe transitional device id 0x1000 just like it probes
modern device id 0x1. This is a once for all fix.
This fix only affects modern-transitional devices in modern code path,
legacy is untouched.
Thanks
>
>
>>>
>>>> Other types of devices also have similar issues, like virito-blk.
>>>>
>>>> I propose to fix this problem of modern-transitonal device
>>>> whith this solution, all in the modern code path:
>>>> 1) assign the device id to mdev->id.device
>>>> 2) add transitional device ids in the virtio-net(and others) probe table.
>>>>
>>>> Comments are welcome!
>>>>
>>>> Thanks!
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>> drivers/net/virtio_net.c | 1 +
>>>> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
>>>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 56ca1d270304..6b45d8602a6b 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>>>> static struct virtio_device_id id_table[] = {
>>>> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>>>> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
>>>> { 0 },
>>>> };
>>>> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
>>>> index 869cb46bef96..80846e1195ce 100644
>>>> --- a/drivers/virtio/virtio_pci_modern_dev.c
>>>> +++ b/drivers/virtio/virtio_pci_modern_dev.c
>>>> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>>>> /* Transitional devices: use the PCI subsystem device id as
>>>> * virtio device id, same as legacy driver always did.
>>>> */
>>>> - mdev->id.device = pci_dev->subsystem_device;
>>>> + mdev->id.device = pci_dev->device;
>>>> } else {
>>>> /* Modern devices: simply use PCI device id, but start from 0x1040. */
>>>> mdev->id.device = pci_dev->device - 0x1040;
>>>> --
>>>> 2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 8:07 ` Zhu, Lingshan
@ 2023-05-29 10:12 ` Michael S. Tsirkin
2023-05-29 10:41 ` Zhu, Lingshan
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-05-29 10:12 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: virtualization
On Mon, May 29, 2023 at 04:07:42PM +0800, Zhu, Lingshan wrote:
>
>
> On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
> > On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
> > >
> > > On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
> > > > On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
> > > > > Current virtio-net only probes a device with VIRITO_ID_NET == 1.
> > > > >
> > > > > For a modern-transtional virtio-net device which has a transtional
> > > > > device id 0x1000 and acts as a modern device, current virtio-pci
> > > > > modern driver will assign the sub-device-id to its mdev->id.device,
> > > > > which may not be 0x1, this sub-device-id is up to the vendor.
> > > > >
> > > > > That means virtio-net driver doesn't probe a modern-transitonal
> > > > > virtio-net with a sub-device-id other than 0x1, which is a bug.
> > > > No, the bug is in the device. Legacy linux drivers always looked at
> > > > sub device id (other OSes might differ). So it makes no sense
> > > > for a transitional device to have sub-device-id other than 0x1.
> > > > Don't have time to look at spec but I think you will find it there.
> > > That is true for a software emulated transitional device,
> > > because there is only "generation" of instance in the hypervisor,
> > > that allowing it to ensure its sub-device-id always be 0x01,
> > > and it fits VIRTIO_ID_NET.
> > >
> > > However, a vendor may produce multiple generations of transitional
> > > hardware. The sub-device-id is up to the vendor, and it is the
> > > only way to for a driver to identify a device, other IDs are all
> > > fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
> > That is one of the issues with legacy virtio, yes.
> >
> >
> >
> > > So the sub-device-id has to be unique and differ from others, can not always
> > > be 0x01.
> >
> > If you are trying to build a device and want to create a safe way to
> > identify it without breaking legacy drivers, then
> > VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
> > For example you can have:
> >
> > struct virtio_pci_vndr_data {
> > u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> > u8 cap_next; /* Generic PCI field: next ptr. */
> > u8 cap_len; /* Generic PCI field: capability length */
> > u8 cfg_type; /* Identifies the structure. */
> > u16 vendor_id; /* Identifies the vendor-specific format. */
> > u16 device_generation; /* Device generation */
> > };
> This can be a solution for sure.
> >
> > > I propose this fix, all changes are for modern-transitional devices in
> > > modern
> > > code path, not for legacy nor legacy-transitional.
> > >
> > > Thanks
> > But what good is this fix? If you just want the modern driver to bind
> > and ignore legacy just create a modern device, you can play
> > with subsystem id and vendor to your heart's content then.
> Not sure who but there are some use-cases require
> transnational devices than modern devices,
> I don't like this neither.
> >
> > If you are using transitional then presumably you want
> > legacy drives to bind, they will not bind if subsystem device
> > id changes.
> well actually it is a transitional device and act as a
> modern device by default, so modern driver will probe.
>
> I think this fix is common and easy, just let virtio-net
> probe transitional device id 0x1000 just like it probes
> modern device id 0x1. This is a once for all fix.
>
> This fix only affects modern-transitional devices in modern code path,
> legacy is untouched.
>
> Thanks
The point of having transitional as opposed to modern is to allow
legacy drivers. If you don't need legacy just use a non transitional
device.
Your device is out of spec:
Transitional devices MUST have the PCI Subsystem Device ID
matching the Virtio Device ID, as indicated in section \ref{sec:Device Types}.
So you will have to explain why the setup you are describing
makes any sense at all before we consider this a fix.
> >
> >
> > > >
> > > > > Other types of devices also have similar issues, like virito-blk.
> > > > >
> > > > > I propose to fix this problem of modern-transitonal device
> > > > > whith this solution, all in the modern code path:
> > > > > 1) assign the device id to mdev->id.device
> > > > > 2) add transitional device ids in the virtio-net(and others) probe table.
> > > > >
> > > > > Comments are welcome!
> > > > >
> > > > > Thanks!
> > > > >
> > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > ---
> > > > > drivers/net/virtio_net.c | 1 +
> > > > > drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> > > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 56ca1d270304..6b45d8602a6b 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
> > > > > static struct virtio_device_id id_table[] = {
> > > > > { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> > > > > + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
> > > > > { 0 },
> > > > > };
> > > > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > > > > index 869cb46bef96..80846e1195ce 100644
> > > > > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > > > > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > > > > @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > > > > /* Transitional devices: use the PCI subsystem device id as
> > > > > * virtio device id, same as legacy driver always did.
> > > > > */
> > > > > - mdev->id.device = pci_dev->subsystem_device;
> > > > > + mdev->id.device = pci_dev->device;
> > > > > } else {
> > > > > /* Modern devices: simply use PCI device id, but start from 0x1040. */
> > > > > mdev->id.device = pci_dev->device - 0x1040;
> > > > > --
> > > > > 2.39.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 10:12 ` Michael S. Tsirkin
@ 2023-05-29 10:41 ` Zhu, Lingshan
2023-05-29 12:04 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Zhu, Lingshan @ 2023-05-29 10:41 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 6403 bytes --]
On 5/29/2023 6:12 PM, Michael S. Tsirkin wrote:
> On Mon, May 29, 2023 at 04:07:42PM +0800, Zhu, Lingshan wrote:
>>
>> On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
>>> On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
>>>> On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
>>>>> On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
>>>>>> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>>>>>>
>>>>>> For a modern-transtional virtio-net device which has a transtional
>>>>>> device id 0x1000 and acts as a modern device, current virtio-pci
>>>>>> modern driver will assign the sub-device-id to its mdev->id.device,
>>>>>> which may not be 0x1, this sub-device-id is up to the vendor.
>>>>>>
>>>>>> That means virtio-net driver doesn't probe a modern-transitonal
>>>>>> virtio-net with a sub-device-id other than 0x1, which is a bug.
>>>>> No, the bug is in the device. Legacy linux drivers always looked at
>>>>> sub device id (other OSes might differ). So it makes no sense
>>>>> for a transitional device to have sub-device-id other than 0x1.
>>>>> Don't have time to look at spec but I think you will find it there.
>>>> That is true for a software emulated transitional device,
>>>> because there is only "generation" of instance in the hypervisor,
>>>> that allowing it to ensure its sub-device-id always be 0x01,
>>>> and it fits VIRTIO_ID_NET.
>>>>
>>>> However, a vendor may produce multiple generations of transitional
>>>> hardware. The sub-device-id is up to the vendor, and it is the
>>>> only way to for a driver to identify a device, other IDs are all
>>>> fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
>>> That is one of the issues with legacy virtio, yes.
>>>
>>>
>>>
>>>> So the sub-device-id has to be unique and differ from others, can not always
>>>> be 0x01.
>>> If you are trying to build a device and want to create a safe way to
>>> identify it without breaking legacy drivers, then
>>> VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
>>> For example you can have:
>>>
>>> struct virtio_pci_vndr_data {
>>> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
>>> u8 cap_next; /* Generic PCI field: next ptr. */
>>> u8 cap_len; /* Generic PCI field: capability length */
>>> u8 cfg_type; /* Identifies the structure. */
>>> u16 vendor_id; /* Identifies the vendor-specific format. */
>>> u16 device_generation; /* Device generation */
>>> };
>> This can be a solution for sure.
>>>> I propose this fix, all changes are for modern-transitional devices in
>>>> modern
>>>> code path, not for legacy nor legacy-transitional.
>>>>
>>>> Thanks
>>> But what good is this fix? If you just want the modern driver to bind
>>> and ignore legacy just create a modern device, you can play
>>> with subsystem id and vendor to your heart's content then.
>> Not sure who but there are some use-cases require
>> transnational devices than modern devices,
>> I don't like this neither.
>>> If you are using transitional then presumably you want
>>> legacy drives to bind, they will not bind if subsystem device
>>> id changes.
>> well actually it is a transitional device and act as a
>> modern device by default, so modern driver will probe.
>>
>> I think this fix is common and easy, just let virtio-net
>> probe transitional device id 0x1000 just like it probes
>> modern device id 0x1. This is a once for all fix.
>>
>> This fix only affects modern-transitional devices in modern code path,
>> legacy is untouched.
>>
>> Thanks
> The point of having transitional as opposed to modern is to allow
> legacy drivers. If you don't need legacy just use a non transitional
> device.
>
> Your device is out of spec:
> Transitional devices MUST have the PCI Subsystem Device ID
> matching the Virtio Device ID, as indicated in section \ref{sec:Device Types}.
OK, thanks for point this out. Since the spec says so, I assume
transitional is almost legacy.
However the spec also says:
Transitional Device a device supporting both drivers conforming to this
specification, and allowing legacy drivers.
The transitional devices have their own device id, like 0x1000 indicates
it is a network device.
Then why the sub-device-id has to be 0x1 in the spec? Is it because we
have the driver first?
Thanks
>
> So you will have to explain why the setup you are describing
> makes any sense at all before we consider this a fix.
>
>
>
>>>
>>>>>> Other types of devices also have similar issues, like virito-blk.
>>>>>>
>>>>>> I propose to fix this problem of modern-transitonal device
>>>>>> whith this solution, all in the modern code path:
>>>>>> 1) assign the device id to mdev->id.device
>>>>>> 2) add transitional device ids in the virtio-net(and others) probe table.
>>>>>>
>>>>>> Comments are welcome!
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan<lingshan.zhu@intel.com>
>>>>>> ---
>>>>>> drivers/net/virtio_net.c | 1 +
>>>>>> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
>>>>>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>> index 56ca1d270304..6b45d8602a6b 100644
>>>>>> --- a/drivers/net/virtio_net.c
>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>>>>>> static struct virtio_device_id id_table[] = {
>>>>>> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>>>>>> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
>>>>>> { 0 },
>>>>>> };
>>>>>> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
>>>>>> index 869cb46bef96..80846e1195ce 100644
>>>>>> --- a/drivers/virtio/virtio_pci_modern_dev.c
>>>>>> +++ b/drivers/virtio/virtio_pci_modern_dev.c
>>>>>> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>>>>>> /* Transitional devices: use the PCI subsystem device id as
>>>>>> * virtio device id, same as legacy driver always did.
>>>>>> */
>>>>>> - mdev->id.device = pci_dev->subsystem_device;
>>>>>> + mdev->id.device = pci_dev->device;
>>>>>> } else {
>>>>>> /* Modern devices: simply use PCI device id, but start from 0x1040. */
>>>>>> mdev->id.device = pci_dev->device - 0x1040;
>>>>>> --
>>>>>> 2.39.1
[-- Attachment #1.2: Type: text/html, Size: 9116 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 10:41 ` Zhu, Lingshan
@ 2023-05-29 12:04 ` Michael S. Tsirkin
2023-05-29 13:13 ` Zhu, Lingshan
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-05-29 12:04 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: virtualization
On Mon, May 29, 2023 at 06:41:54PM +0800, Zhu, Lingshan wrote:
>
>
> On 5/29/2023 6:12 PM, Michael S. Tsirkin wrote:
>
> On Mon, May 29, 2023 at 04:07:42PM +0800, Zhu, Lingshan wrote:
>
>
> On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
>
> On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
>
> On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
>
> On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
>
> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>
> For a modern-transtional virtio-net device which has a transtional
> device id 0x1000 and acts as a modern device, current virtio-pci
> modern driver will assign the sub-device-id to its mdev->id.device,
> which may not be 0x1, this sub-device-id is up to the vendor.
>
> That means virtio-net driver doesn't probe a modern-transitonal
> virtio-net with a sub-device-id other than 0x1, which is a bug.
>
> No, the bug is in the device. Legacy linux drivers always looked at
> sub device id (other OSes might differ). So it makes no sense
> for a transitional device to have sub-device-id other than 0x1.
> Don't have time to look at spec but I think you will find it there.
>
> That is true for a software emulated transitional device,
> because there is only "generation" of instance in the hypervisor,
> that allowing it to ensure its sub-device-id always be 0x01,
> and it fits VIRTIO_ID_NET.
>
> However, a vendor may produce multiple generations of transitional
> hardware. The sub-device-id is up to the vendor, and it is the
> only way to for a driver to identify a device, other IDs are all
> fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
>
> That is one of the issues with legacy virtio, yes.
>
>
>
>
> So the sub-device-id has to be unique and differ from others, can not always
> be 0x01.
>
> If you are trying to build a device and want to create a safe way to
> identify it without breaking legacy drivers, then
> VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
> For example you can have:
>
> struct virtio_pci_vndr_data {
> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> u8 cap_next; /* Generic PCI field: next ptr. */
> u8 cap_len; /* Generic PCI field: capability length */
> u8 cfg_type; /* Identifies the structure. */
> u16 vendor_id; /* Identifies the vendor-specific format. */
> u16 device_generation; /* Device generation */
> };
>
> This can be a solution for sure.
>
> I propose this fix, all changes are for modern-transitional devices in
> modern
> code path, not for legacy nor legacy-transitional.
>
> Thanks
>
> But what good is this fix? If you just want the modern driver to bind
> and ignore legacy just create a modern device, you can play
> with subsystem id and vendor to your heart's content then.
>
> Not sure who but there are some use-cases require
> transnational devices than modern devices,
> I don't like this neither.
>
> If you are using transitional then presumably you want
> legacy drives to bind, they will not bind if subsystem device
> id changes.
>
> well actually it is a transitional device and act as a
> modern device by default, so modern driver will probe.
>
> I think this fix is common and easy, just let virtio-net
> probe transitional device id 0x1000 just like it probes
> modern device id 0x1. This is a once for all fix.
>
> This fix only affects modern-transitional devices in modern code path,
> legacy is untouched.
>
> Thanks
>
> The point of having transitional as opposed to modern is to allow
> legacy drivers. If you don't need legacy just use a non transitional
> device.
>
> Your device is out of spec:
> Transitional devices MUST have the PCI Subsystem Device ID
> matching the Virtio Device ID, as indicated in section \ref{sec:Device Types}.
>
> OK, thanks for point this out. Since the spec says so, I assume transitional is
> almost legacy.
>
> However the spec also says:
> Transitional Device a device supporting both drivers conforming to this
> specification, and allowing legacy drivers.
>
> The transitional devices have their own device id, like 0x1000 indicates it is
> a network device.
>
> Then why the sub-device-id has to be 0x1 in the spec? Is it because we have the
> driver first?
>
> Thanks
yes, for example windows drivers:
PCI\VEN_1AF4&DEV_1000&SUBSYS_0001_INX_SUBSYS_VENDOR_ID&REV_00
Rusty originally thought drivers can ignore device id completely, and
just use subsystem id. Something something ... a maze of twisty abstractions ...
but it turned out it does not work e.g. for windows.
>
>
>
> So you will have to explain why the setup you are describing
> makes any sense at all before we consider this a fix.
>
>
>
>
>
>
> Other types of devices also have similar issues, like virito-blk.
>
> I propose to fix this problem of modern-transitonal device
> whith this solution, all in the modern code path:
> 1) assign the device id to mdev->id.device
> 2) add transitional device ids in the virtio-net(and others) probe table.
>
> Comments are welcome!
>
> Thanks!
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/net/virtio_net.c | 1 +
> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 56ca1d270304..6b45d8602a6b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
> { 0 },
> };
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index 869cb46bef96..80846e1195ce 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> /* Transitional devices: use the PCI subsystem device id as
> * virtio device id, same as legacy driver always did.
> */
> - mdev->id.device = pci_dev->subsystem_device;
> + mdev->id.device = pci_dev->device;
> } else {
> /* Modern devices: simply use PCI device id, but start from 0x1040. */
> mdev->id.device = pci_dev->device - 0x1040;
> --
> 2.39.1
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 12:04 ` Michael S. Tsirkin
@ 2023-05-29 13:13 ` Zhu, Lingshan
2023-05-29 13:36 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Zhu, Lingshan @ 2023-05-29 13:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
On 5/29/2023 8:04 PM, Michael S. Tsirkin wrote:
> On Mon, May 29, 2023 at 06:41:54PM +0800, Zhu, Lingshan wrote:
>>
>> On 5/29/2023 6:12 PM, Michael S. Tsirkin wrote:
>>
>> On Mon, May 29, 2023 at 04:07:42PM +0800, Zhu, Lingshan wrote:
>>
>>
>> On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
>>
>> On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
>>
>> On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
>>
>> On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
>>
>> Current virtio-net only probes a device with VIRITO_ID_NET == 1.
>>
>> For a modern-transtional virtio-net device which has a transtional
>> device id 0x1000 and acts as a modern device, current virtio-pci
>> modern driver will assign the sub-device-id to its mdev->id.device,
>> which may not be 0x1, this sub-device-id is up to the vendor.
>>
>> That means virtio-net driver doesn't probe a modern-transitonal
>> virtio-net with a sub-device-id other than 0x1, which is a bug.
>>
>> No, the bug is in the device. Legacy linux drivers always looked at
>> sub device id (other OSes might differ). So it makes no sense
>> for a transitional device to have sub-device-id other than 0x1.
>> Don't have time to look at spec but I think you will find it there.
>>
>> That is true for a software emulated transitional device,
>> because there is only "generation" of instance in the hypervisor,
>> that allowing it to ensure its sub-device-id always be 0x01,
>> and it fits VIRTIO_ID_NET.
>>
>> However, a vendor may produce multiple generations of transitional
>> hardware. The sub-device-id is up to the vendor, and it is the
>> only way to for a driver to identify a device, other IDs are all
>> fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
>>
>> That is one of the issues with legacy virtio, yes.
>>
>>
>>
>>
>> So the sub-device-id has to be unique and differ from others, can not always
>> be 0x01.
>>
>> If you are trying to build a device and want to create a safe way to
>> identify it without breaking legacy drivers, then
>> VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
>> For example you can have:
>>
>> struct virtio_pci_vndr_data {
>> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
>> u8 cap_next; /* Generic PCI field: next ptr. */
>> u8 cap_len; /* Generic PCI field: capability length */
>> u8 cfg_type; /* Identifies the structure. */
>> u16 vendor_id; /* Identifies the vendor-specific format. */
>> u16 device_generation; /* Device generation */
>> };
>>
>> This can be a solution for sure.
>>
>> I propose this fix, all changes are for modern-transitional devices in
>> modern
>> code path, not for legacy nor legacy-transitional.
>>
>> Thanks
>>
>> But what good is this fix? If you just want the modern driver to bind
>> and ignore legacy just create a modern device, you can play
>> with subsystem id and vendor to your heart's content then.
>>
>> Not sure who but there are some use-cases require
>> transnational devices than modern devices,
>> I don't like this neither.
>>
>> If you are using transitional then presumably you want
>> legacy drives to bind, they will not bind if subsystem device
>> id changes.
>>
>> well actually it is a transitional device and act as a
>> modern device by default, so modern driver will probe.
>>
>> I think this fix is common and easy, just let virtio-net
>> probe transitional device id 0x1000 just like it probes
>> modern device id 0x1. This is a once for all fix.
>>
>> This fix only affects modern-transitional devices in modern code path,
>> legacy is untouched.
>>
>> Thanks
>>
>> The point of having transitional as opposed to modern is to allow
>> legacy drivers. If you don't need legacy just use a non transitional
>> device.
>>
>> Your device is out of spec:
>> Transitional devices MUST have the PCI Subsystem Device ID
>> matching the Virtio Device ID, as indicated in section \ref{sec:Device Types}.
>>
>> OK, thanks for point this out. Since the spec says so, I assume transitional is
>> almost legacy.
>>
>> However the spec also says:
>> Transitional Device a device supporting both drivers conforming to this
>> specification, and allowing legacy drivers.
>>
>> The transitional devices have their own device id, like 0x1000 indicates it is
>> a network device.
>>
>> Then why the sub-device-id has to be 0x1 in the spec? Is it because we have the
>> driver first?
>>
>> Thanks
> yes, for example windows drivers:
>
> PCI\VEN_1AF4&DEV_1000&SUBSYS_0001_INX_SUBSYS_VENDOR_ID&REV_00
>
> Rusty originally thought drivers can ignore device id completely, and
> just use subsystem id. Something something ... a maze of twisty abstractions ...
> but it turned out it does not work e.g. for windows.
If so I think it is beyond our control to fix this issue in Windows,
thus we can
not change the spec about transitional sub-device-id.
I think I can only let it be so. I can try the vendor cap instead.
Thanks
>
>>
>>
>> So you will have to explain why the setup you are describing
>> makes any sense at all before we consider this a fix.
>>
>>
>>
>>
>>
>>
>> Other types of devices also have similar issues, like virito-blk.
>>
>> I propose to fix this problem of modern-transitonal device
>> whith this solution, all in the modern code path:
>> 1) assign the device id to mdev->id.device
>> 2) add transitional device ids in the virtio-net(and others) probe table.
>>
>> Comments are welcome!
>>
>> Thanks!
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/net/virtio_net.c | 1 +
>> drivers/virtio/virtio_pci_modern_dev.c | 2 +-
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 56ca1d270304..6b45d8602a6b 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>> static struct virtio_device_id id_table[] = {
>> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>> + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
>> { 0 },
>> };
>> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
>> index 869cb46bef96..80846e1195ce 100644
>> --- a/drivers/virtio/virtio_pci_modern_dev.c
>> +++ b/drivers/virtio/virtio_pci_modern_dev.c
>> @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>> /* Transitional devices: use the PCI subsystem device id as
>> * virtio device id, same as legacy driver always did.
>> */
>> - mdev->id.device = pci_dev->subsystem_device;
>> + mdev->id.device = pci_dev->device;
>> } else {
>> /* Modern devices: simply use PCI device id, but start from 0x1040. */
>> mdev->id.device = pci_dev->device - 0x1040;
>> --
>> 2.39.1
>>
>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] virtio-net: support modern-transtional devices
2023-05-29 13:13 ` Zhu, Lingshan
@ 2023-05-29 13:36 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-05-29 13:36 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: virtualization
On Mon, May 29, 2023 at 09:13:09PM +0800, Zhu, Lingshan wrote:
>
>
> On 5/29/2023 8:04 PM, Michael S. Tsirkin wrote:
> > On Mon, May 29, 2023 at 06:41:54PM +0800, Zhu, Lingshan wrote:
> > >
> > > On 5/29/2023 6:12 PM, Michael S. Tsirkin wrote:
> > >
> > > On Mon, May 29, 2023 at 04:07:42PM +0800, Zhu, Lingshan wrote:
> > >
> > >
> > > On 5/29/2023 2:38 PM, Michael S. Tsirkin wrote:
> > >
> > > On Mon, May 29, 2023 at 02:19:36PM +0800, Zhu, Lingshan wrote:
> > >
> > > On 5/28/2023 7:28 PM, Michael S. Tsirkin wrote:
> > >
> > > On Sat, May 27, 2023 at 02:15:42AM +0800, Zhu Lingshan wrote:
> > >
> > > Current virtio-net only probes a device with VIRITO_ID_NET == 1.
> > >
> > > For a modern-transtional virtio-net device which has a transtional
> > > device id 0x1000 and acts as a modern device, current virtio-pci
> > > modern driver will assign the sub-device-id to its mdev->id.device,
> > > which may not be 0x1, this sub-device-id is up to the vendor.
> > >
> > > That means virtio-net driver doesn't probe a modern-transitonal
> > > virtio-net with a sub-device-id other than 0x1, which is a bug.
> > >
> > > No, the bug is in the device. Legacy linux drivers always looked at
> > > sub device id (other OSes might differ). So it makes no sense
> > > for a transitional device to have sub-device-id other than 0x1.
> > > Don't have time to look at spec but I think you will find it there.
> > >
> > > That is true for a software emulated transitional device,
> > > because there is only "generation" of instance in the hypervisor,
> > > that allowing it to ensure its sub-device-id always be 0x01,
> > > and it fits VIRTIO_ID_NET.
> > >
> > > However, a vendor may produce multiple generations of transitional
> > > hardware. The sub-device-id is up to the vendor, and it is the
> > > only way to for a driver to identify a device, other IDs are all
> > > fixed as 0x1af4, 0x1000 and 0x8086 for Intel.
> > >
> > > That is one of the issues with legacy virtio, yes.
> > >
> > >
> > >
> > >
> > > So the sub-device-id has to be unique and differ from others, can not always
> > > be 0x01.
> > >
> > > If you are trying to build a device and want to create a safe way to
> > > identify it without breaking legacy drivers, then
> > > VIRTIO_PCI_CAP_VENDOR_CFG has been designed for things like this.
> > > For example you can have:
> > >
> > > struct virtio_pci_vndr_data {
> > > u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> > > u8 cap_next; /* Generic PCI field: next ptr. */
> > > u8 cap_len; /* Generic PCI field: capability length */
> > > u8 cfg_type; /* Identifies the structure. */
> > > u16 vendor_id; /* Identifies the vendor-specific format. */
> > > u16 device_generation; /* Device generation */
> > > };
> > >
> > > This can be a solution for sure.
> > >
> > > I propose this fix, all changes are for modern-transitional devices in
> > > modern
> > > code path, not for legacy nor legacy-transitional.
> > >
> > > Thanks
> > >
> > > But what good is this fix? If you just want the modern driver to bind
> > > and ignore legacy just create a modern device, you can play
> > > with subsystem id and vendor to your heart's content then.
> > >
> > > Not sure who but there are some use-cases require
> > > transnational devices than modern devices,
> > > I don't like this neither.
> > >
> > > If you are using transitional then presumably you want
> > > legacy drives to bind, they will not bind if subsystem device
> > > id changes.
> > >
> > > well actually it is a transitional device and act as a
> > > modern device by default, so modern driver will probe.
> > >
> > > I think this fix is common and easy, just let virtio-net
> > > probe transitional device id 0x1000 just like it probes
> > > modern device id 0x1. This is a once for all fix.
> > >
> > > This fix only affects modern-transitional devices in modern code path,
> > > legacy is untouched.
> > >
> > > Thanks
> > >
> > > The point of having transitional as opposed to modern is to allow
> > > legacy drivers. If you don't need legacy just use a non transitional
> > > device.
> > >
> > > Your device is out of spec:
> > > Transitional devices MUST have the PCI Subsystem Device ID
> > > matching the Virtio Device ID, as indicated in section \ref{sec:Device Types}.
> > >
> > > OK, thanks for point this out. Since the spec says so, I assume transitional is
> > > almost legacy.
> > >
> > > However the spec also says:
> > > Transitional Device a device supporting both drivers conforming to this
> > > specification, and allowing legacy drivers.
> > >
> > > The transitional devices have their own device id, like 0x1000 indicates it is
> > > a network device.
> > >
> > > Then why the sub-device-id has to be 0x1 in the spec? Is it because we have the
> > > driver first?
> > >
> > > Thanks
> > yes, for example windows drivers:
> >
> > PCI\VEN_1AF4&DEV_1000&SUBSYS_0001_INX_SUBSYS_VENDOR_ID&REV_00
> >
> > Rusty originally thought drivers can ignore device id completely, and
> > just use subsystem id. Something something ... a maze of twisty abstractions ...
> > but it turned out it does not work e.g. for windows.
> If so I think it is beyond our control to fix this issue in Windows,
Well it's under our control and we did change it, what we can't change
is history and installed guests. It's easier to find ways that do
not break them than to break them and then try and fix them.
> thus we
> can
> not change the spec about transitional sub-device-id.
>
> I think I can only let it be so. I can try the vendor cap instead.
>
> Thanks
Sounds good.
> >
> > >
> > >
> > > So you will have to explain why the setup you are describing
> > > makes any sense at all before we consider this a fix.
> > >
> > >
> > >
> > >
> > >
> > >
> > > Other types of devices also have similar issues, like virito-blk.
> > >
> > > I propose to fix this problem of modern-transitonal device
> > > whith this solution, all in the modern code path:
> > > 1) assign the device id to mdev->id.device
> > > 2) add transitional device ids in the virtio-net(and others) probe table.
> > >
> > > Comments are welcome!
> > >
> > > Thanks!
> > >
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > > drivers/net/virtio_net.c | 1 +
> > > drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 56ca1d270304..6b45d8602a6b 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -4250,6 +4250,7 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
> > > static struct virtio_device_id id_table[] = {
> > > { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> > > + { VIRTIO_TRANS_ID_NET, VIRTIO_DEV_ANY_ID },
> > > { 0 },
> > > };
> > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > > index 869cb46bef96..80846e1195ce 100644
> > > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > > @@ -229,7 +229,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > > /* Transitional devices: use the PCI subsystem device id as
> > > * virtio device id, same as legacy driver always did.
> > > */
> > > - mdev->id.device = pci_dev->subsystem_device;
> > > + mdev->id.device = pci_dev->device;
> > > } else {
> > > /* Modern devices: simply use PCI device id, but start from 0x1040. */
> > > mdev->id.device = pci_dev->device - 0x1040;
> > > --
> > > 2.39.1
> > >
> > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-29 13:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-26 18:15 [RFC] virtio-net: support modern-transtional devices Zhu Lingshan
2023-05-28 11:28 ` Michael S. Tsirkin
2023-05-29 6:19 ` Zhu, Lingshan
2023-05-29 6:38 ` Michael S. Tsirkin
2023-05-29 8:07 ` Zhu, Lingshan
2023-05-29 10:12 ` Michael S. Tsirkin
2023-05-29 10:41 ` Zhu, Lingshan
2023-05-29 12:04 ` Michael S. Tsirkin
2023-05-29 13:13 ` Zhu, Lingshan
2023-05-29 13:36 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).