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

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Wed, Mar 15, 2023 at 05:25:27PM +0000, Rajnesh Kanwal wrote:
> 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)

Ah, I missed that, thank you for pointing it out!

Alex

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