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 <[email protected]> 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 <[email protected]> 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
Add a comment explaining why, please.
> +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.
> sg_init_one(&data_sg, data, sizeof(*data));
> cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE);
> cmd.data_sg = &data_sg;
> @@ -775,257 +778,6 @@ static void vp_modern_destroy_avq(struct virtio_device
> *vdev)
> vp_dev->del_vq(&vp_dev->admin_vq.info);
> }
>
> -#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))
> -
> -#ifdef CONFIG_X86
> -/*
> - * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
> - * commands are supported
> - * @dev: VF pci_dev
> - *
> - * Returns true on success.
> - */
> -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;
> -
> - if (!virtio_dev)
> - return false;
> -
> - if (!virtio_has_feature(virtio_dev, VIRTIO_F_ADMIN_VQ))
> - return false;
>
>
> <other deletion to the new file>
> <other deletion to the new file>
> ..
> ..
>
> diff --git a/drivers/virtio/virtio_pci_admin_legacy_io.c
> b/drivers/virtio/virtio_pci_admin_legacy_io.c
> new file mode 100644
> index 000000000000..c48eaaa7c086
> --- /dev/null
> +++ b/drivers/virtio/virtio_pci_admin_legacy_io.c
> @@ -0,0 +1,244 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + */
> +
> +#include "virtio_pci_common.h"
> +
> +/*
> + * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
> + * commands are supported
> + * @dev: VF pci_dev
> + *
> + * Returns true on success.
> + */
> +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;
> +
> + if (!virtio_dev)
> + return false;
> +
> + if (!virtio_has_feature(virtio_dev, VIRTIO_F_ADMIN_VQ))
> + return false;
> +
> + vp_dev = to_vp_device(virtio_dev);
> +
> + if ((vp_dev->admin_vq.supported_cmds &
> VIRTIO_LEGACY_ADMIN_CMD_BITMAP) ==
> + VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
>
>
> <other legacy IO code>
> <other legacy IO code>
> ...
> ...
>
>
> diff --git a/include/linux/virtio_pci_admin.h
> b/include/linux/virtio_pci_admin.h
> index 446ced8cb050..0c9c1f336d3f 100644
> --- a/include/linux/virtio_pci_admin.h
> +++ b/include/linux/virtio_pci_admin.h
> @@ -5,6 +5,7 @@
> #include <linux/types.h>
> #include <linux/pci.h>
>
> +#ifdef CONFIG_X86
> bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev);
> int virtio_pci_admin_legacy_common_io_write(struct pci_dev *pdev, u8
> offset,
> u8 size, u8 *buf);
> @@ -17,5 +18,6 @@ int virtio_pci_admin_legacy_device_io_read(struct pci_dev
> *pdev, u8 offset,
> int virtio_pci_admin_legacy_io_notify_info(struct pci_dev *pdev,
> u8 req_bar_flags, u8 *bar,
> u64 *bar_offset);
> +#endif
>
> Yishai
