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

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

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()?

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