Michael S. Tsirkin
2023-May-29 06:38 UTC
[RFC] virtio-net: support modern-transtional devices
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. > > ThanksBut 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 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/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