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 > >
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 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 >> >>