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 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-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230529/6a7ac192/attachment-0001.html>
Michael S. Tsirkin
2023-May-29 12:04 UTC
[RFC] virtio-net: support modern-transtional devices
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? > > Thanksyes, 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 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 > >