Re: [PATCH kvmtool] aarch64: Add stolen time support

[Marc Zyngier <maz@kernel.org>]

Hi Sebastian,

Thanks for looking into this. A few comments below.

On 2022-02-16 16:16, Sebastian Ene wrote:
> This patch add support for stolen time by sharing a memory region
> with the guest which will be used by the hypervisor to store the stolen
> time information. The exact format of the structure stored by the
> hypervisor is described in the ARM DEN0057A document.
> 
> Signed-off-by: Sebastian Ene <sebastianene@google.com>
> ---
>  Makefile                          |  3 +-
>  arm/aarch64/arm-cpu.c             |  2 +
>  arm/aarch64/include/kvm/pvtime.h  |  6 +++
>  arm/aarch64/pvtime.c              | 83 +++++++++++++++++++++++++++++++
>  arm/include/arm-common/kvm-arch.h |  6 +++
>  arm/kvm-cpu.c                     | 14 +++---
>  6 files changed, 106 insertions(+), 8 deletions(-)
>  create mode 100644 arm/aarch64/include/kvm/pvtime.h
>  create mode 100644 arm/aarch64/pvtime.c
> 
> diff --git a/Makefile b/Makefile
> index f251147..282ae99 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -182,6 +182,7 @@ ifeq ($(ARCH), arm64)
>  	OBJS		+= arm/aarch64/arm-cpu.o
>  	OBJS		+= arm/aarch64/kvm-cpu.o
>  	OBJS		+= arm/aarch64/kvm.o
> +	OBJS		+= arm/aarch64/pvtime.o
>  	ARCH_INCLUDE	:= $(HDRS_ARM_COMMON)
>  	ARCH_INCLUDE	+= -Iarm/aarch64/include
> 
> @@ -582,4 +583,4 @@ ifneq ($(MAKECMDGOALS),clean)
> 
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> -endif
> \ No newline at end of file
> +endif

Spurious change?

> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index d7572b7..80bf83a 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -2,6 +2,7 @@
>  #include "kvm/kvm.h"
>  #include "kvm/kvm-cpu.h"
>  #include "kvm/util.h"
> +#include "kvm/pvtime.h"
> 
>  #include "arm-common/gic.h"
>  #include "arm-common/timer.h"
> @@ -22,6 +23,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm 
> *kvm)
>  static int arm_cpu__vcpu_init(struct kvm_cpu *vcpu)
>  {
>  	vcpu->generate_fdt_nodes = generate_fdt_nodes;
> +	pvtime__setup_vcpu(vcpu);
>  	return 0;
>  }
> 
> diff --git a/arm/aarch64/include/kvm/pvtime.h 
> b/arm/aarch64/include/kvm/pvtime.h
> new file mode 100644
> index 0000000..c31f019
> --- /dev/null
> +++ b/arm/aarch64/include/kvm/pvtime.h
> @@ -0,0 +1,6 @@
> +#ifndef KVM__PVTIME_H
> +#define KVM__PVTIME_H
> +
> +void pvtime__setup_vcpu(struct kvm_cpu *vcpu);
> +
> +#endif /* KVM__PVTIME_H */

How about sticking this in kvm-cpu-arch.h instead? A whole new include 
file
for just a prototype isn't totally warranted.

> diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> new file mode 100644
> index 0000000..eb92388
> --- /dev/null
> +++ b/arm/aarch64/pvtime.c
> @@ -0,0 +1,83 @@
> +#include "kvm/kvm.h"
> +#include "kvm/kvm-cpu.h"
> +#include "kvm/util.h"
> +#include "kvm/pvtime.h"
> +
> +#include <linux/byteorder.h>
> +#include <linux/types.h>
> +
> +struct pvtime_data_priv {
> +	bool	is_supported;
> +	char	*usr_mem;
> +};
> +
> +static struct pvtime_data_priv pvtime_data = {
> +	.is_supported	= true,
> +	.usr_mem	= NULL
> +};
> +
> +static int pvtime__aloc_region(struct kvm *kvm)

s/aloc/alloc/ ?

> +{
> +	char *mem;
> +	int ret = 0;
> +
> +	mem = mmap(NULL, AARCH64_PVTIME_IPA_MAX_SIZE, PROT_RW,
> +		   MAP_ANON_NORESERVE, -1, 0);
> +	if (mem == MAP_FAILED)
> +		return -ENOMEM;
> +
> +	ret = kvm__register_dev_mem(kvm, AARCH64_PVTIME_IPA_START,
> +				    AARCH64_PVTIME_IPA_MAX_SIZE, mem);
> +	if (ret) {
> +		munmap(mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> +		return ret;
> +	}
> +
> +	pvtime_data.usr_mem = mem;
> +	return ret;
> +}
> +
> +static int pvtime__teardown_region(struct kvm *kvm)
> +{
> +	kvm__destroy_mem(kvm, AARCH64_PVTIME_IPA_START,
> +			 AARCH64_PVTIME_IPA_MAX_SIZE, pvtime_data.usr_mem);
> +	munmap(pvtime_data.usr_mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> +	pvtime_data.usr_mem = NULL;
> +	return 0;
> +}
> +
> +void pvtime__setup_vcpu(struct kvm_cpu *vcpu)
> +{
> +	int ret;
> +	u64 pvtime_guest_addr = AARCH64_PVTIME_IPA_START + vcpu->cpu_id *
> +		AARCH64_PVTIME_SIZE;
> +	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> +		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
> +		.addr	= KVM_ARM_VCPU_PVTIME_IPA
> +	};
> +
> +	if (!pvtime_data.is_supported)
> +		return;
> +
> +	if (!pvtime_data.usr_mem) {
> +		ret = pvtime__aloc_region(vcpu->kvm);
> +		if (ret)
> +			goto out_err_alloc;
> +	}
> +
> +	ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> +	if (ret)
> +		goto out_err_attr;

You should check for the stolen time capability before allocating and 
mapping
the memory.

> +
> +	pvtime_attr.addr = (u64)&pvtime_guest_addr;
> +	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> +	if (!ret)
> +		return;
> +
> +out_err_attr:
> +	pvtime__teardown_region(vcpu->kvm);
> +out_err_alloc:
> +	pvtime_data.is_supported = false;
> +}
> +
> +dev_exit(pvtime__teardown_region);
> diff --git a/arm/include/arm-common/kvm-arch.h
> b/arm/include/arm-common/kvm-arch.h
> index c645ac0..7b683d6 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -54,6 +54,12 @@
>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> 
> +#define AARCH64_PVTIME_IPA_MAX_SIZE		(0x10000)

SZ_64K?

> +#define AARCH64_PROTECTED_VM_FW_MAX_SIZE	(0x200000)

This definitely looks like something that shouldn't be there. Yet.

> +#define AARCH64_PVTIME_IPA_START	(ARM_MEMORY_AREA - \
> +					 AARCH64_PROTECTED_VM_FW_MAX_SIZE - \
> +					 AARCH64_PVTIME_IPA_MAX_SIZE)
> +#define AARCH64_PVTIME_SIZE		(64)
> 
>  #define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
>  #define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index 6a2408c..84ac1e9 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -116,6 +116,13 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm
> *kvm, unsigned long cpu_id)
>  			die("Unable to find matching target");
>  	}
> 
> +	/* Populate the vcpu structure. */
> +	vcpu->kvm		= kvm;
> +	vcpu->cpu_id		= cpu_id;
> +	vcpu->cpu_type		= vcpu_init.target;
> +	vcpu->cpu_compatible	= target->compatible;
> +	vcpu->is_running	= true;
> +
>  	if (err || target->init(vcpu))
>  		die("Unable to initialise vcpu");
> 
> @@ -125,13 +132,6 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm
> *kvm, unsigned long cpu_id)
>  		vcpu->ring = (void *)vcpu->kvm_run +
>  			     (coalesced_offset * PAGE_SIZE);
> 
> -	/* Populate the vcpu structure. */
> -	vcpu->kvm		= kvm;
> -	vcpu->cpu_id		= cpu_id;
> -	vcpu->cpu_type		= vcpu_init.target;
> -	vcpu->cpu_compatible	= target->compatible;
> -	vcpu->is_running	= true;
> -

What is the reason for moving these assignments around?

>  	if (kvm_cpu__configure_features(vcpu))
>  		die("Unable to configure requested vcpu features");

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...