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

[Sebastian Ene <sebastianene@google.com>]

On Wed, Feb 16, 2022 at 04:59:10PM +0000, Marc Zyngier wrote:
> Hi Sebastian,
> 
> Thanks for looking into this. A few comments below.
> 

Hello Marc,

Thanks for taking the time to review my patch.

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

Right, ACK I will revert this 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.
>

I think that's a good ideea, I will do this.

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

I will fix the typo.

> > +{
> > +	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.
> 

Good catch ! I will do this, thanks.

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

I will update the `AARCH64_PVTIME_IPA_MAX_SIZE` defintion with `SZ_64K`.

> > +#define AARCH64_PROTECTED_VM_FW_MAX_SIZE	(0x200000)
> 
> This definitely looks like something that shouldn't be there. Yet.
> 

I agree, I will remove it for the moment.

> > +#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?
> 

The setup of the pvtime is done during the vcpu initialisation and I
need to access the kvm structure during that time.

> >  	if (kvm_cpu__configure_features(vcpu))
> >  		die("Unable to configure requested vcpu features");
> 
> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...