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 at 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
Michael S. Tsirkin
2023-May-29 10:12 UTC
[RFC] virtio-net: support modern-transtional devices
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. > > ThanksThe 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 at 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