Michael S. Tsirkin
2023-May-28 11:28 UTC
[RFC] virtio-net: support modern-transtional devices
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 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
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 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