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