Re: [RFC] virtio-net: support modern-transtional devices

["Michael S. Tsirkin" <mst@redhat.com>]

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.

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