On 17/12/2023 14:20, Michael S. Tsirkin wrote:
On Sun, Dec 17, 2023 at 12:39:48PM +0200, Yishai Hadas wrote:
On 14/12/2023 18:40, Michael S. Tsirkin wrote:
On Thu, Dec 14, 2023 at 06:25:25PM +0200, Yishai Hadas wrote:
On 14/12/2023 18:15, Alex Williamson wrote:
On Thu, 14 Dec 2023 18:03:30 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
On 14/12/2023 17:05, Michael S. Tsirkin wrote:
On Thu, Dec 14, 2023 at 07:59:05AM -0700, Alex Williamson wrote:
On Thu, 14 Dec 2023 11:37:10 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
OK, if so, we can come with the below extra code.
Makes sense ?
I'll squash it as part of V8 to the relevant patch.
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 37a0035f8381..b652e91b9df4 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -794,6 +794,9 @@ bool virtio_pci_admin_has_legacy_io(struct pci_dev
*pdev)
struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
struct virtio_pci_device *vp_dev;
+#ifndef CONFIG_X86
+ return false;
+#endif
if (!virtio_dev)
return false;
Yishai
Isn't there going to be a bunch more dead code that compiler won't be
able to elide?
On my setup the compiler didn't complain about dead-code (I simulated it
by using ifdef CONFIG_X86 return false).
However, if we suspect that some compiler might complain, we can come
with the below instead.
Do you prefer that ?
index 37a0035f8381..53e29824d404 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -782,6 +782,7 @@ static void vp_modern_destroy_avq(struct
virtio_device *vdev)
BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
+#ifdef CONFIG_X86
/*
* virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
* commands are supported
@@ -807,6 +808,12 @@ bool virtio_pci_admin_has_legacy_io(struct pci_dev
*pdev)
return true;
return false;
}
+#else
+bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev)
+{
+ return false;
+}
+#endif
EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
Doesn't this also raise the question of the purpose of virtio-vfio-pci
on non-x86? Without any other features it offers nothing over vfio-pci
and we should likely adjust the Kconfig for x86 or COMPILE_TEST.
Thanks,
Alex
Kconfig dependency is what I had in mind, yes. The X86 specific code in
virtio_pci_modern.c can be moved to a separate file then use makefile
tricks to skip it on other platforms.
The next feature for that driver will be the live migration support over
virtio, once the specification which is WIP those day will be accepted.
The migration functionality is not X86 dependent and doesn't have the
legacy virtio driver limitations that enforced us to run only on X86.
So, by that time we may need to enable in VFIO the loading of
virtio-vfio-pci driver and put back the ifdef X86 inside VIRTIO, only on
the legacy IO API, as I did already in V8.
So using a KCONFIG solution in VFIO is a short term one, which will be
reverted just later on.
I understand the intent, but I don't think that justifies building a
driver that serves no purpose in the interim. IF and when migration
support becomes a reality, it's trivial to update the depends line.
OK, so I'll add a KCONFIG dependency on X86 as you suggested as part of V9
inside VFIO.
In addition, the virtio_pci_admin_has_legacy_io() API can be used in the
future not only by VFIO, this was one of the reasons to put it inside
VIRTIO.
Maybe this should be governed by a new Kconfig option which would be
selected by drivers like this. Thanks,
We can still keep the simple ifdef X86 inside VIRTIO for future users/usage
which is not only VFIO.
Michael,
Can that work for you ?
Yishai
Alex
I am not sure what is proposed exactly. General admin q infrastructure
can be kept as is. The legacy things however can never work outside X86.
Best way to limit it to x86 is to move it to a separate file and
only build that on X86. This way the only ifdef we need is where
we set the flags to enable legacy commands.
In VFIO we already agreed to add a dependency on X86 [1] as Alex asked.
As VIRTIO should be ready for other clients and be self contained, I thought
to keep things simple and just return false from
virtio_pci_admin_has_legacy_io() in non X86 systems as was sent in V8.
However, we can go with your approach as well and compile out all the legacy
IO stuff in non X86 systems by moving its code to a separate file (i.e.
virtio_pci_admin_legacy_io.c) and control this file upon the Makefile. In
addition, you suggested to control the 'supported_cmds' by an ifdef. This
will let the device know that we don't support legacy IO as well on non X86
systems.
Please be aware that the above approach requires another ifdef on the H file
which exposes the 6 exported symbols and some further changes inside virtio
as of making vp_modern_admin_cmd_exec() non static as now we move the legacy
IO stuff to another C file, etc.
Please see below how [2] it will look like.
If you prefer that, so OK, it will be part of V9.
Please let me know.
[1] diff --git a/drivers/vfio/pci/virtio/Kconfig
b/drivers/vfio/pci/virtio/Kconfig
index 050473b0e5df..a3e5d8ea22a0 100644
--- a/drivers/vfio/pci/virtio/Kconfig
+++ b/drivers/vfio/pci/virtio/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config VIRTIO_VFIO_PCI
tristate "VFIO support for VIRTIO NET PCI devices"
- depends on VIRTIO_PCI
+ depends on X86 && VIRTIO_PCI
select VFIO_PCI_CORE
help
This provides support for exposing VIRTIO NET VF devices which
support
[2] diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 8e98d24917cc..a73358bb4ebb 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
+virtio_pci-$(CONFIG_X86) += virtio_pci_admin_legacy_io.o
obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
index af676b3b9907..9963e5d0e881 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -158,4 +158,14 @@ void virtio_pci_modern_remove(struct virtio_pci_device
*);
struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
+#define VIRTIO_LEGACY_ADMIN_CMD_BITMAP \
+ (BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE) | \
+ BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ) | \
+ BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE) | \
+ BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
+ BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
+
I'd add something like:
#ifdef CONFIG_X86
#define VIRTIO_ADMIN_CMD_BITMAP VIRTIO_LEGACY_ADMIN_CMD_BITMAP
#else
#define VIRTIO_ADMIN_CMD_BITMAP 0
#endif
This new macro (i.e. VIRTIO_ADMIN_CMD_BITMAP) will hold in the future
another sets of supported bits (e.g. Live migration), right ?
Add a comment explaining why, please.
OK
How about the below ?
"As of some limitations in the legacy driver (e.g. lack of memory
barriers in ARM, endian-ness is broken in PPC) the supported legacy IO
commands are masked out on non x86 systems."
+int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
+ struct virtio_admin_cmd *cmd);
+
#endif
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 53e29824d404..defb6282e1d7 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -75,8 +75,8 @@ static int virtqueue_exec_admin_cmd(struct
virtio_pci_admin_vq *admin_vq,
return 0;
}
-static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
- struct virtio_admin_cmd *cmd)
+int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
+ struct virtio_admin_cmd *cmd)
{
struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat;
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -172,6 +172,9 @@ static void virtio_pci_admin_cmd_list_init(struct
virtio_device *virtio_dev)
if (ret)
goto end;
+#ifndef CONFIG_X86
+ *data &= ~(cpu_to_le64(VIRTIO_LEGACY_ADMIN_CMD_BITMAP));
+#endif
Then here we don't need an ifdef just use VIRTIO_ADMIN_CMD_BITMAP.
Following your suggestion, this will become,
*data &= cpu_to_le64(VIRTIO_ADMIN_CMD_BITMAP);
(no ifdef, and no ~).
Yishai
