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 <[email protected]>
> ---
> 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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
