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....@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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
