Re: [PATCH kvmtool 1/1] Add virtio-transport option and deprecate force-pci and virtio-legacy.

[Rajnesh Kanwal <rkanwal@rivosinc.com>]

On Fri, Mar 10, 2023 at 3:04 PM Alexandru Elisei
<alexandru.elisei@arm.com> wrote:
>
> Hi,
>
> Thank you for doing this!
>
> The patch looks good, some nitpicks below.
>
> On Mon, Mar 06, 2023 at 12:03:29PM +0000, Rajnesh Kanwal wrote:
> > This is a follow-up patch for [0] which introduced --force-pci option
>
> "which proposed the --force-pci [..]"? The way you have worded it makes it
> sound, at least to me, like the patch was already merged.
>
> > for riscv. As per the discussion it was concluded to add virtio-tranport
> > option taking in four options (pci, pci-legacy, mmio, mmio-legacy).
> >
> > With this change force-pci and virtio-legacy are both deprecated and
> > arm's default transport changes from MMIO to PCI as agreed in [0].
> > This is also true for riscv.
> >
> > Nothing changes for other architectures.
> >
> > [0]: https://lore.kernel.org/all/20230118172007.408667-1-rkanwal@rivosinc.com/
> >
> > Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
> > ---
> >  arm/include/arm-common/kvm-arch.h        |  5 ----
> >  arm/include/arm-common/kvm-config-arch.h |  8 +++----
> >  builtin-run.c                            | 11 +++++++--
> >  include/kvm/kvm-config.h                 |  2 +-
> >  include/kvm/kvm.h                        |  6 +----
> >  include/kvm/virtio.h                     |  2 ++
> >  riscv/include/kvm/kvm-arch.h             |  3 ---
> >  virtio/core.c                            | 29 ++++++++++++++++++++++++
> >  8 files changed, 46 insertions(+), 20 deletions(-)
> >
> > diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> > index b2ae373..60eec02 100644
> > --- a/arm/include/arm-common/kvm-arch.h
> > +++ b/arm/include/arm-common/kvm-arch.h
> > @@ -80,11 +80,6 @@
> >
> >  #define KVM_VM_TYPE          0
> >
> > -#define VIRTIO_DEFAULT_TRANS(kvm)                                    \
> > -     ((kvm)->cfg.arch.virtio_trans_pci ?                             \
> > -      ((kvm)->cfg.virtio_legacy ? VIRTIO_PCI_LEGACY : VIRTIO_PCI) :  \
> > -      ((kvm)->cfg.virtio_legacy ? VIRTIO_MMIO_LEGACY : VIRTIO_MMIO))
> > -
> >  #define VIRTIO_RING_ENDIAN   (VIRTIO_ENDIAN_LE | VIRTIO_ENDIAN_BE)
> >
> >  #define ARCH_HAS_PCI_EXP     1
> > diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> > index 9949bfe..2e620fd 100644
> > --- a/arm/include/arm-common/kvm-config-arch.h
> > +++ b/arm/include/arm-common/kvm-config-arch.h
> > @@ -7,7 +7,6 @@ struct kvm_config_arch {
> >       const char      *dump_dtb_filename;
> >       const char      *vcpu_affinity;
> >       unsigned int    force_cntfrq;
> > -     bool            virtio_trans_pci;
> >       bool            aarch32_guest;
> >       bool            has_pmuv3;
> >       bool            mte_disabled;
> > @@ -28,9 +27,10 @@ int irqchip_parser(const struct option *opt, const char *arg, int unset);
> >                    "Specify Generic Timer frequency in guest DT to "          \
> >                    "work around buggy secure firmware *Firmware should be "   \
> >                    "updated to program CNTFRQ correctly*"),                   \
> > -     OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,                \
> > -                 "Force virtio devices to use PCI as their default "         \
> > -                 "transport"),                                               \
> > +     OPT_CALLBACK_NOOPT('\0', "force-pci", NULL, '\0',                       \
>
> Couldn't you pass &(cfg)->virtio_transport here for the third parameter instead
> of NULL as you do for the other options, to avoid special casing force-pci in
> virtio_tranport_parser()?
>

The problem here is that the cfg parameter here is actually
`&kvm->cfg.arch` whereas
`virtio_transport` lives in `kvm->cfg`. This happens in the `OPT_ARCH` macro.

     #define OPT_ARCH(cmd, cfg)                  \
             OPT_ARCH_##cmd(OPT_GROUP("Arch-specific options:"), &(cfg)->arch)

Thanks for reviewing. I have incorporated all of your feedback in v2.
https://lore.kernel.org/all/20230315171238.300572-1-rkanwal@rivosinc.com/

> > +                        "Force virtio devices to use PCI as their default "  \
> > +                        "transport [Deprecated: Use --virtio-transport "     \
>
> Small detail, but the usual way of adding a note to a help text is to use
> curved paranthesis ( "()", see?) instead of square brackets. kvmtool does that
> for the help text for kaslr-seed (see
> arm/aarch64/include/kvm/kvm-config-arch.h). The man pages also use paranthesis.
>
> > +                        "option instead]", virtio_tranport_parser, kvm),     \
>
> Looks to me like the function name wants to be virtio_tran**s**port_parser()
> (emphasis added), and the current name (without the 's') is a typo.
>
> >          OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,                               \
> >                    "[gicv2|gicv2m|gicv3|gicv3-its]",                          \
> >                    "Type of interrupt controller to emulate in the guest",    \
> > diff --git a/builtin-run.c b/builtin-run.c
> > index bb7e6e8..50e8796 100644
> > --- a/builtin-run.c
> > +++ b/builtin-run.c
> > @@ -200,8 +200,15 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
> >                       " rootfs"),                                     \
> >       OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path",   \
> >                       "Hugetlbfs path"),                              \
> > -     OPT_BOOLEAN('\0', "virtio-legacy", &(cfg)->virtio_legacy,       \
> > -                 "Use legacy virtio transport"),                     \
> > +     OPT_CALLBACK_NOOPT('\0', "virtio-legacy",                       \
> > +                        &(cfg)->virtio_transport, '\0',              \
> > +                        "Use legacy virtio transport [Deprecated:"   \
> > +                        " Use --virtio-transport option instead]",   \
> > +                        virtio_tranport_parser, NULL),               \
> > +     OPT_CALLBACK('\0', "virtio-transport", &(cfg)->virtio_transport,\
> > +                  "[pci|pci-legacy|mmio|mmio-legacy]",               \
> > +                  "Type of virtio transport",                        \
> > +                  virtio_tranport_parser, NULL),                     \
> >                                                                       \
> >       OPT_GROUP("Kernel options:"),                                   \
> >       OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel",    \
> > diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> > index 368e6c7..592b035 100644
> > --- a/include/kvm/kvm-config.h
> > +++ b/include/kvm/kvm-config.h
> > @@ -64,7 +64,7 @@ struct kvm_config {
> >       bool no_dhcp;
> >       bool ioport_debug;
> >       bool mmio_debug;
> > -     bool virtio_legacy;
> > +     int virtio_transport;
>
> I was about to suggest changing this to enum virtio_trans virtio_transport,
> but that means including virtio.h in this file, which leads to header
> dependency hell. Let's leave that alone for now :)
>
> >  };
> >
> >  #endif
> > diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> > index 3872dc6..7015def 100644
> > --- a/include/kvm/kvm.h
> > +++ b/include/kvm/kvm.h
> > @@ -45,11 +45,7 @@ struct kvm_cpu;
> >  typedef void (*mmio_handler_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data,
> >                               u32 len, u8 is_write, void *ptr);
> >
> > -/* Archs can override this in kvm-arch.h */
> > -#ifndef VIRTIO_DEFAULT_TRANS
> > -#define VIRTIO_DEFAULT_TRANS(kvm) \
> > -     ((kvm)->cfg.virtio_legacy ? VIRTIO_PCI_LEGACY : VIRTIO_PCI)
> > -#endif
> > +#define VIRTIO_DEFAULT_TRANS(kvm) (kvm)->cfg.virtio_transport
>
> Well, the purpose of the define was to allow architectures to override it,
> the way arm did it.
>
> Since all architectures behave the same way now and there is no need for an
> override, how about we drop the macro altogether? We can also remove the
> virtio_trans parameter from virtio_init(), because it already has a
> reference to kvm.
>
> >
> >  enum {
> >       KVM_VMSTATE_RUNNING,
> > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> > index 94bddef..4a733f5 100644
> > --- a/include/kvm/virtio.h
> > +++ b/include/kvm/virtio.h
> > @@ -248,4 +248,6 @@ void virtio_set_guest_features(struct kvm *kvm, struct virtio_device *vdev,
> >  void virtio_notify_status(struct kvm *kvm, struct virtio_device *vdev,
> >                         void *dev, u8 status);
> >
> > +int virtio_tranport_parser(const struct option *opt, const char *arg, int unset);
> > +
> >  #endif /* KVM__VIRTIO_H */
> > diff --git a/riscv/include/kvm/kvm-arch.h b/riscv/include/kvm/kvm-arch.h
> > index 1e130f5..4106099 100644
> > --- a/riscv/include/kvm/kvm-arch.h
> > +++ b/riscv/include/kvm/kvm-arch.h
> > @@ -46,9 +46,6 @@
> >
> >  #define KVM_VM_TYPE          0
> >
> > -#define VIRTIO_DEFAULT_TRANS(kvm) \
> > -     ((kvm)->cfg.virtio_legacy ? VIRTIO_MMIO_LEGACY : VIRTIO_MMIO)
> > -
> >  #define VIRTIO_RING_ENDIAN   VIRTIO_ENDIAN_LE
> >
> >  #define ARCH_HAS_PCI_EXP     1
> > diff --git a/virtio/core.c b/virtio/core.c
> > index ea0e5b6..4b863c7 100644
> > --- a/virtio/core.c
> > +++ b/virtio/core.c
> > @@ -21,6 +21,35 @@ const char* virtio_trans_name(enum virtio_trans trans)
> >       return "unknown";
> >  }
> >
> > +int virtio_tranport_parser(const struct option *opt, const char *arg, int unset)
>
> If --virtio-transport is not specified on the kvmtool command line, then
> the default transport is set to VIRTIO_PCI, because that is the first
> member in the virtio_trans enum, and struct kvm is initialized to 0 in
> kvm__new() when it's allocated with calloc.
>
> The above can be obscure for someone who is not familiar with the code. I
> think making the default explicit, by setting kvm->cfg.virtio_transport =
> VIRTIO_PCI in kvm_cmd_run_init(), before the command line arguments are
> parsed, would be clearer.
>
> Thanks,
> Alex
>
> > +{
> > +     enum virtio_trans *type = opt->value;
> > +
> > +     if (!strcmp(opt->long_name, "virtio-transport")) {
> > +             if (!strcmp(arg, "pci")) {
> > +                     *type = VIRTIO_PCI;
> > +             } else if (!strcmp(arg, "pci-legacy")) {
> > +                     *type = VIRTIO_PCI_LEGACY;
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
> > +             } else if (!strcmp(arg, "mmio")) {
> > +                     *type = VIRTIO_MMIO;
> > +             } else if (!strcmp(arg, "mmio-legacy")) {
> > +                     *type = VIRTIO_MMIO_LEGACY;
> > +#endif
> > +             } else {
> > +                     pr_err("virtio-transport: unknown type \"%s\"\n", arg);
> > +                     return -1;
> > +             }
> > +     } else if (!strcmp(opt->long_name, "virtio-legacy")) {
> > +             *type = VIRTIO_PCI_LEGACY;
> > +     } else if (!strcmp(opt->long_name, "force-pci")) {
> > +             struct kvm *kvm = opt->ptr;
> > +             kvm->cfg.virtio_transport = VIRTIO_PCI;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  void virt_queue__used_idx_advance(struct virt_queue *queue, u16 jump)
> >  {
> >       u16 idx = virtio_guest_to_host_u16(queue, queue->vring.used->idx);
> > --
> > 2.25.1
> >

Thanks
Rajnesh