[PATCH kvmtool v4 0/3] aarch64: Add stolen time support

[PATCH kvmtool v4 0/3] aarch64: Add stolen time support

[Sebastian Ene]

These patches add support for stolen time functionality.

Patch #1 modifies the memory layout in arm-common/kvm-arch.h and adds a
new MMIO device, PVTIME after the RTC region. A new flag is added in
kvm-config.h that will be used to control [enable/disable] the pvtime
functionality.

Patch #2 moves the vCPU structure initialisation before the target->init()
call to allow early access to the kvm structure.

Patch #3 adds a new command line argument to disable the stolen time
functionality(by default is enabled).

 Changelog sinde v3:
 - Avoid the pvtime overlap with the PCI(AXI) region by moving the
   peripheral in the MMIO zone, after the RTC.
 - Split in a separate patch the change affecting the vCPU structure
   population
 - Added a new command line argument in a separate patch.

 Changelog since v2:
 - Moved the AARCH64_PVTIME_* definitions from arm-common/kvm-arch.h to
   arm64/pvtime.c as pvtime is only available for arm64.

 Changelog since v1:
 - Removed the pvtime.h header file and moved the definitions to kvm-cpu-arch.h
   Verified if the stolen time capability is supported before allocating
   and mapping the memory.

Sebastian Ene (3):
  aarch64: Add stolen time support
  aarch64: Populate the vCPU struct before target->init()
  Add --no-pvtime command line argument

 Makefile                               |  1 +
 arm/aarch64/arm-cpu.c                  |  1 +
 arm/aarch64/include/kvm/kvm-cpu-arch.h |  1 +
 arm/aarch64/pvtime.c                   | 94 ++++++++++++++++++++++++++
 arm/include/arm-common/kvm-arch.h      |  6 +-
 arm/kvm-cpu.c                          | 14 ++--
 builtin-run.c                          |  2 +
 include/kvm/kvm-config.h               |  1 +
 8 files changed, 112 insertions(+), 8 deletions(-)
 create mode 100644 arm/aarch64/pvtime.c

-- 
2.35.1.473.g83b2b277ed-goog

[PATCH kvmtool v4 1/3] aarch64: Add stolen time support

[Sebastian Ene]

This patch adds 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. Reserve a 64kb MMIO memory region after the RTC peripheral
to be used by pvtime. 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                               |  1 +
 arm/aarch64/arm-cpu.c                  |  1 +
 arm/aarch64/include/kvm/kvm-cpu-arch.h |  1 +
 arm/aarch64/pvtime.c                   | 94 ++++++++++++++++++++++++++
 arm/include/arm-common/kvm-arch.h      |  6 +-
 include/kvm/kvm-config.h               |  1 +
 6 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 arm/aarch64/pvtime.c

diff --git a/Makefile b/Makefile
index f251147..e9121dc 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
 
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index d7572b7..326fb20 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -22,6 +22,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;
+	kvm_cpu__setup_pvtime(vcpu);
 	return 0;
 }
 
diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
index 8dfb82e..b57d6e6 100644
--- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
+++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
@@ -19,5 +19,6 @@
 
 void kvm_cpu__select_features(struct kvm *kvm, struct kvm_vcpu_init *init);
 int kvm_cpu__configure_features(struct kvm_cpu *vcpu);
+void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu);
 
 #endif /* KVM__KVM_CPU_ARCH_H */
diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
new file mode 100644
index 0000000..8251f6a
--- /dev/null
+++ b/arm/aarch64/pvtime.c
@@ -0,0 +1,94 @@
+#include "kvm/kvm.h"
+#include "kvm/kvm-cpu.h"
+#include "kvm/util.h"
+
+#include <linux/byteorder.h>
+#include <linux/types.h>
+
+#define ARM_PVTIME_STRUCT_SIZE		(64)
+
+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__alloc_region(struct kvm *kvm)
+{
+	char *mem;
+	int ret = 0;
+
+	mem = mmap(NULL, ARM_PVTIME_MMIO_SIZE, PROT_RW,
+		   MAP_ANON_NORESERVE, -1, 0);
+	if (mem == MAP_FAILED)
+		return -ENOMEM;
+
+	ret = kvm__register_dev_mem(kvm, ARM_PVTIME_MMIO_BASE,
+				    ARM_PVTIME_MMIO_SIZE, mem);
+	if (ret) {
+		munmap(mem, ARM_PVTIME_MMIO_SIZE);
+		return ret;
+	}
+
+	pvtime_data.usr_mem = mem;
+	return ret;
+}
+
+static int pvtime__teardown_region(struct kvm *kvm)
+{
+	if (pvtime_data.usr_mem == NULL)
+		return 0;
+
+	kvm__destroy_mem(kvm, ARM_PVTIME_MMIO_BASE,
+			 ARM_PVTIME_MMIO_SIZE, pvtime_data.usr_mem);
+	munmap(pvtime_data.usr_mem, ARM_PVTIME_MMIO_SIZE);
+	pvtime_data.usr_mem = NULL;
+	return 0;
+}
+
+void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu)
+{
+	int ret;
+	u64 pvtime_guest_addr = ARM_PVTIME_MMIO_BASE + vcpu->cpu_id *
+		ARM_PVTIME_STRUCT_SIZE;
+	struct kvm_config *kvm_cfg = NULL;
+	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
+		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
+		.addr	= KVM_ARM_VCPU_PVTIME_IPA
+	};
+
+	BUG_ON(!vcpu);
+	BUG_ON(!vcpu->kvm);
+
+	kvm_cfg = &vcpu->kvm->cfg;
+	if (kvm_cfg && kvm_cfg->no_pvtime)
+		return;
+
+	if (!pvtime_data.is_supported)
+		return;
+
+	ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
+	if (ret)
+		goto out_err;
+
+	if (!pvtime_data.usr_mem) {
+		ret = pvtime__alloc_region(vcpu->kvm);
+		if (ret)
+			goto out_err;
+	}
+
+	pvtime_attr.addr = (u64)&pvtime_guest_addr;
+	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
+	if (!ret)
+		return;
+
+	pvtime__teardown_region(vcpu->kvm);
+out_err:
+	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..3f82663 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -15,7 +15,8 @@
  * |  PCI  |////| plat  |       |        |     |         |
  * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
  * | space |////| UART, |       |  MMIO  |     |  (AXI)  |
- * |       |////| RTC   |       |        |     |         |
+ * |       |////| RTC,  |       |        |     |         |
+ * |       |////| PVTIME|       |        |     |         |
  * +-------+----+-------+-------+--------+-----+---------+---......
  */
 
@@ -34,6 +35,9 @@
 #define ARM_RTC_MMIO_BASE	(ARM_UART_MMIO_BASE + ARM_UART_MMIO_SIZE)
 #define ARM_RTC_MMIO_SIZE	0x10000
 
+#define ARM_PVTIME_MMIO_BASE	(ARM_RTC_MMIO_BASE + ARM_RTC_MMIO_SIZE)
+#define ARM_PVTIME_MMIO_SIZE	SZ_64K
+
 #define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
 #define KVM_FLASH_MAX_SIZE	0x1000000
 
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 6a5720c..48adf27 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -62,6 +62,7 @@ struct kvm_config {
 	bool no_dhcp;
 	bool ioport_debug;
 	bool mmio_debug;
+	bool no_pvtime;
 };
 
 #endif
-- 
2.35.1.473.g83b2b277ed-goog

Re: [PATCH kvmtool v4 1/3] aarch64: Add stolen time support

[Alexandru Elisei]

Hi,

On Thu, Feb 24, 2022 at 04:51:03PM +0000, Sebastian Ene wrote:
> This patch adds 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. Reserve a 64kb MMIO memory region after the RTC peripheral
> to be used by pvtime. 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                               |  1 +
>  arm/aarch64/arm-cpu.c                  |  1 +
>  arm/aarch64/include/kvm/kvm-cpu-arch.h |  1 +
>  arm/aarch64/pvtime.c                   | 94 ++++++++++++++++++++++++++
>  arm/include/arm-common/kvm-arch.h      |  6 +-
>  include/kvm/kvm-config.h               |  1 +
>  6 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 arm/aarch64/pvtime.c
> 
> diff --git a/Makefile b/Makefile
> index f251147..e9121dc 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
>  
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index d7572b7..326fb20 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -22,6 +22,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;
> +	kvm_cpu__setup_pvtime(vcpu);
>  	return 0;
>  }
>  
> diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> index 8dfb82e..b57d6e6 100644
> --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> @@ -19,5 +19,6 @@
>  
>  void kvm_cpu__select_features(struct kvm *kvm, struct kvm_vcpu_init *init);
>  int kvm_cpu__configure_features(struct kvm_cpu *vcpu);
> +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu);
>  
>  #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> new file mode 100644
> index 0000000..8251f6a
> --- /dev/null
> +++ b/arm/aarch64/pvtime.c
> @@ -0,0 +1,94 @@
> +#include "kvm/kvm.h"
> +#include "kvm/kvm-cpu.h"
> +#include "kvm/util.h"
> +
> +#include <linux/byteorder.h>
> +#include <linux/types.h>
> +
> +#define ARM_PVTIME_STRUCT_SIZE		(64)
> +
> +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__alloc_region(struct kvm *kvm)
> +{
> +	char *mem;
> +	int ret = 0;
> +
> +	mem = mmap(NULL, ARM_PVTIME_MMIO_SIZE, PROT_RW,
> +		   MAP_ANON_NORESERVE, -1, 0);
> +	if (mem == MAP_FAILED)
> +		return -ENOMEM;

Hm... man 2 mmap lists a few dozen error codes, why use -ENOMEM here instead of
-errno? This just makes debugging harder.

> +
> +	ret = kvm__register_dev_mem(kvm, ARM_PVTIME_MMIO_BASE,
> +				    ARM_PVTIME_MMIO_SIZE, mem);
> +	if (ret) {
> +		munmap(mem, ARM_PVTIME_MMIO_SIZE);
> +		return ret;
> +	}
> +
> +	pvtime_data.usr_mem = mem;
> +	return ret;
> +}
> +
> +static int pvtime__teardown_region(struct kvm *kvm)
> +{
> +	if (pvtime_data.usr_mem == NULL)
> +		return 0;
> +
> +	kvm__destroy_mem(kvm, ARM_PVTIME_MMIO_BASE,
> +			 ARM_PVTIME_MMIO_SIZE, pvtime_data.usr_mem);
> +	munmap(pvtime_data.usr_mem, ARM_PVTIME_MMIO_SIZE);
> +	pvtime_data.usr_mem = NULL;
> +	return 0;
> +}
> +
> +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu)
> +{
> +	int ret;
> +	u64 pvtime_guest_addr = ARM_PVTIME_MMIO_BASE + vcpu->cpu_id *

That's trange, cpu_id is not initialized here because target->init() is called
before setting up the cpu_id. The following patch in the series, "aarch64:
Populate the vCPU struct before target->init()" should come before this one.

> +		ARM_PVTIME_STRUCT_SIZE;
> +	struct kvm_config *kvm_cfg = NULL;
> +	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> +		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
> +		.addr	= KVM_ARM_VCPU_PVTIME_IPA
> +	};
> +
> +	BUG_ON(!vcpu);
> +	BUG_ON(!vcpu->kvm);
> +
> +	kvm_cfg = &vcpu->kvm->cfg;
> +	if (kvm_cfg && kvm_cfg->no_pvtime)

If you move the next patch in the series before this one, all the above checks
will not be needed and should be removed.

In general, each patch in a series should be able to build properly and run a VM
without errors. This is to help users when bisecting [1]. If I build kvmtool
from this patch and I try to run a VM I get the error:

Error: BUG at arm/aarch64/pvtime.c:65

[1] https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#separate-your-changes

> +		return;
> +
> +	if (!pvtime_data.is_supported)
> +		return;
> +
> +	ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> +	if (ret)
> +		goto out_err;

You should check that pvtime is supported by checking the KVM_CAP_STEAL_TIME on
the VM fd, as that's how capabilities are advertised by KVM.

> +
> +	if (!pvtime_data.usr_mem) {
> +		ret = pvtime__alloc_region(vcpu->kvm);

pvtime__alloc_region() can fail pretty catastrophically, is it ok to ignore it
and go on? I would have expected kvm_cpu__setup_pvtime() to return an error
which is then propagated to target->init().

> +		if (ret)
> +			goto out_err;
> +	}
> +
> +	pvtime_attr.addr = (u64)&pvtime_guest_addr;
> +	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> +	if (!ret)
> +		return;
> +
> +	pvtime__teardown_region(vcpu->kvm);
> +out_err:
> +	pvtime_data.is_supported = false;
> +}
> +
> +dev_exit(pvtime__teardown_region);

It is customary to put the dev_exit() exactly after the function it refers to.

> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index c645ac0..3f82663 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -15,7 +15,8 @@
>   * |  PCI  |////| plat  |       |        |     |         |
>   * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
>   * | space |////| UART, |       |  MMIO  |     |  (AXI)  |
> - * |       |////| RTC   |       |        |     |         |
> + * |       |////| RTC,  |       |        |     |         |
> + * |       |////| PVTIME|       |        |     |         |
>   * +-------+----+-------+-------+--------+-----+---------+---......
>   */
>  
> @@ -34,6 +35,9 @@
>  #define ARM_RTC_MMIO_BASE	(ARM_UART_MMIO_BASE + ARM_UART_MMIO_SIZE)
>  #define ARM_RTC_MMIO_SIZE	0x10000
>  
> +#define ARM_PVTIME_MMIO_BASE	(ARM_RTC_MMIO_BASE + ARM_RTC_MMIO_SIZE)
> +#define ARM_PVTIME_MMIO_SIZE	SZ_64K

This looks good.

Thanks,
Alex

> +
>  #define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
>  #define KVM_FLASH_MAX_SIZE	0x1000000
>  
> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> index 6a5720c..48adf27 100644
> --- a/include/kvm/kvm-config.h
> +++ b/include/kvm/kvm-config.h
> @@ -62,6 +62,7 @@ struct kvm_config {
>  	bool no_dhcp;
>  	bool ioport_debug;
>  	bool mmio_debug;
> +	bool no_pvtime;
>  };
>  
>  #endif
> -- 
> 2.35.1.473.g83b2b277ed-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvmtool v4 1/3] aarch64: Add stolen time support

[Sebastian Ene]

On Fri, Feb 25, 2022 at 11:55:03AM +0000, Alexandru Elisei wrote:
> Hi,
> 

Hi,

> On Thu, Feb 24, 2022 at 04:51:03PM +0000, Sebastian Ene wrote:
> > This patch adds 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. Reserve a 64kb MMIO memory region after the RTC peripheral
> > to be used by pvtime. 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                               |  1 +
> >  arm/aarch64/arm-cpu.c                  |  1 +
> >  arm/aarch64/include/kvm/kvm-cpu-arch.h |  1 +
> >  arm/aarch64/pvtime.c                   | 94 ++++++++++++++++++++++++++
> >  arm/include/arm-common/kvm-arch.h      |  6 +-
> >  include/kvm/kvm-config.h               |  1 +
> >  6 files changed, 103 insertions(+), 1 deletion(-)
> >  create mode 100644 arm/aarch64/pvtime.c
> > 
> > diff --git a/Makefile b/Makefile
> > index f251147..e9121dc 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
> >  
> > diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> > index d7572b7..326fb20 100644
> > --- a/arm/aarch64/arm-cpu.c
> > +++ b/arm/aarch64/arm-cpu.c
> > @@ -22,6 +22,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;
> > +	kvm_cpu__setup_pvtime(vcpu);
> >  	return 0;
> >  }
> >  
> > diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > index 8dfb82e..b57d6e6 100644
> > --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > @@ -19,5 +19,6 @@
> >  
> >  void kvm_cpu__select_features(struct kvm *kvm, struct kvm_vcpu_init *init);
> >  int kvm_cpu__configure_features(struct kvm_cpu *vcpu);
> > +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu);
> >  
> >  #endif /* KVM__KVM_CPU_ARCH_H */
> > diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> > new file mode 100644
> > index 0000000..8251f6a
> > --- /dev/null
> > +++ b/arm/aarch64/pvtime.c
> > @@ -0,0 +1,94 @@
> > +#include "kvm/kvm.h"
> > +#include "kvm/kvm-cpu.h"
> > +#include "kvm/util.h"
> > +
> > +#include <linux/byteorder.h>
> > +#include <linux/types.h>
> > +
> > +#define ARM_PVTIME_STRUCT_SIZE		(64)
> > +
> > +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__alloc_region(struct kvm *kvm)
> > +{
> > +	char *mem;
> > +	int ret = 0;
> > +
> > +	mem = mmap(NULL, ARM_PVTIME_MMIO_SIZE, PROT_RW,
> > +		   MAP_ANON_NORESERVE, -1, 0);
> > +	if (mem == MAP_FAILED)
> > +		return -ENOMEM;
> 
> Hm... man 2 mmap lists a few dozen error codes, why use -ENOMEM here instead of
> -errno? This just makes debugging harder.
> 

I will return the -errno error code here.

> > +
> > +	ret = kvm__register_dev_mem(kvm, ARM_PVTIME_MMIO_BASE,
> > +				    ARM_PVTIME_MMIO_SIZE, mem);
> > +	if (ret) {
> > +		munmap(mem, ARM_PVTIME_MMIO_SIZE);
> > +		return ret;
> > +	}
> > +
> > +	pvtime_data.usr_mem = mem;
> > +	return ret;
> > +}
> > +
> > +static int pvtime__teardown_region(struct kvm *kvm)
> > +{
> > +	if (pvtime_data.usr_mem == NULL)
> > +		return 0;
> > +
> > +	kvm__destroy_mem(kvm, ARM_PVTIME_MMIO_BASE,
> > +			 ARM_PVTIME_MMIO_SIZE, pvtime_data.usr_mem);
> > +	munmap(pvtime_data.usr_mem, ARM_PVTIME_MMIO_SIZE);
> > +	pvtime_data.usr_mem = NULL;
> > +	return 0;
> > +}
> > +
> > +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu)
> > +{
> > +	int ret;
> > +	u64 pvtime_guest_addr = ARM_PVTIME_MMIO_BASE + vcpu->cpu_id *
> 
> That's trange, cpu_id is not initialized here because target->init() is called
> before setting up the cpu_id. The following patch in the series, "aarch64:
> Populate the vCPU struct before target->init()" should come before this one.
> 
> > +		ARM_PVTIME_STRUCT_SIZE;
> > +	struct kvm_config *kvm_cfg = NULL;
> > +	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> > +		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
> > +		.addr	= KVM_ARM_VCPU_PVTIME_IPA
> > +	};
> > +
> > +	BUG_ON(!vcpu);
> > +	BUG_ON(!vcpu->kvm);
> > +
> > +	kvm_cfg = &vcpu->kvm->cfg;
> > +	if (kvm_cfg && kvm_cfg->no_pvtime)
> 
> If you move the next patch in the series before this one, all the above checks
> will not be needed and should be removed.
> 
> In general, each patch in a series should be able to build properly and run a VM
> without errors. This is to help users when bisecting [1]. If I build kvmtool
> from this patch and I try to run a VM I get the error:
> 
> Error: BUG at arm/aarch64/pvtime.c:65
> 
> [1] https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#separate-your-changes
> 

I will move the patch that handles the structure initialisation before
this one and I will remove those checks. Thanks for the suggestion !

> > +		return;
> > +
> > +	if (!pvtime_data.is_supported)
> > +		return;
> > +
> > +	ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> > +	if (ret)
> > +		goto out_err;
> 
> You should check that pvtime is supported by checking the KVM_CAP_STEAL_TIME on
> the VM fd, as that's how capabilities are advertised by KVM.
> 

I thought that it is sufficient to verify that it has the device
attributes for *PVTIME. I will add this check too, thanks for the
suggestion !

> > +
> > +	if (!pvtime_data.usr_mem) {
> > +		ret = pvtime__alloc_region(vcpu->kvm);
> 
> pvtime__alloc_region() can fail pretty catastrophically, is it ok to ignore it
> and go on? I would have expected kvm_cpu__setup_pvtime() to return an error
> which is then propagated to target->init().
>

Hmm, I didn't propagate the error code from kvm_cpu__setup_pvtime()
because if this feature is not supported it will hit:"Unable to
initialise vcpu".

But I guess that now we can propagate the error code as we have '--no-pvtime'
command argument. What do you think ?

> > +		if (ret)
> > +			goto out_err;
> > +	}
> > +
> > +	pvtime_attr.addr = (u64)&pvtime_guest_addr;
> > +	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> > +	if (!ret)
> > +		return;
> > +
> > +	pvtime__teardown_region(vcpu->kvm);
> > +out_err:
> > +	pvtime_data.is_supported = false;
> > +}
> > +
> > +dev_exit(pvtime__teardown_region);
> 
> It is customary to put the dev_exit() exactly after the function it refers to.
> 

I will move this.

> > diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> > index c645ac0..3f82663 100644
> > --- a/arm/include/arm-common/kvm-arch.h
> > +++ b/arm/include/arm-common/kvm-arch.h
> > @@ -15,7 +15,8 @@
> >   * |  PCI  |////| plat  |       |        |     |         |
> >   * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
> >   * | space |////| UART, |       |  MMIO  |     |  (AXI)  |
> > - * |       |////| RTC   |       |        |     |         |
> > + * |       |////| RTC,  |       |        |     |         |
> > + * |       |////| PVTIME|       |        |     |         |
> >   * +-------+----+-------+-------+--------+-----+---------+---......
> >   */
> >  
> > @@ -34,6 +35,9 @@
> >  #define ARM_RTC_MMIO_BASE	(ARM_UART_MMIO_BASE + ARM_UART_MMIO_SIZE)
> >  #define ARM_RTC_MMIO_SIZE	0x10000
> >  
> > +#define ARM_PVTIME_MMIO_BASE	(ARM_RTC_MMIO_BASE + ARM_RTC_MMIO_SIZE)
> > +#define ARM_PVTIME_MMIO_SIZE	SZ_64K
> 
> This looks good.
> 
> Thanks,
> Alex
>

Thanks for the review,
Sebastian

> > +
> >  #define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
> >  #define KVM_FLASH_MAX_SIZE	0x1000000
> >  
> > diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> > index 6a5720c..48adf27 100644
> > --- a/include/kvm/kvm-config.h
> > +++ b/include/kvm/kvm-config.h
> > @@ -62,6 +62,7 @@ struct kvm_config {
> >  	bool no_dhcp;
> >  	bool ioport_debug;
> >  	bool mmio_debug;
> > +	bool no_pvtime;
> >  };
> >  
> >  #endif
> > -- 
> > 2.35.1.473.g83b2b277ed-goog
> > 
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvmtool v4 1/3] aarch64: Add stolen time support

[Alexandru Elisei]

Hi,

On Fri, Feb 25, 2022 at 01:21:26PM +0000, Sebastian Ene wrote:
> On Fri, Feb 25, 2022 at 11:55:03AM +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> 
> Hi,
> 
> > On Thu, Feb 24, 2022 at 04:51:03PM +0000, Sebastian Ene wrote:
> > > This patch adds 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. Reserve a 64kb MMIO memory region after the RTC peripheral
> > > to be used by pvtime. 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                               |  1 +
> > >  arm/aarch64/arm-cpu.c                  |  1 +
> > >  arm/aarch64/include/kvm/kvm-cpu-arch.h |  1 +
> > >  arm/aarch64/pvtime.c                   | 94 ++++++++++++++++++++++++++
> > >  arm/include/arm-common/kvm-arch.h      |  6 +-
> > >  include/kvm/kvm-config.h               |  1 +
> > >  6 files changed, 103 insertions(+), 1 deletion(-)
> > >  create mode 100644 arm/aarch64/pvtime.c
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index f251147..e9121dc 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
> > >  
> > > diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> > > index d7572b7..326fb20 100644
> > > --- a/arm/aarch64/arm-cpu.c
> > > +++ b/arm/aarch64/arm-cpu.c
> > > @@ -22,6 +22,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;
> > > +	kvm_cpu__setup_pvtime(vcpu);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > > index 8dfb82e..b57d6e6 100644
> > > --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > > +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> > > @@ -19,5 +19,6 @@
> > >  
> > >  void kvm_cpu__select_features(struct kvm *kvm, struct kvm_vcpu_init *init);
> > >  int kvm_cpu__configure_features(struct kvm_cpu *vcpu);
> > > +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu);
> > >  
> > >  #endif /* KVM__KVM_CPU_ARCH_H */
> > > diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> > > new file mode 100644
> > > index 0000000..8251f6a
> > > --- /dev/null
> > > +++ b/arm/aarch64/pvtime.c
> > > @@ -0,0 +1,94 @@
> > > +#include "kvm/kvm.h"
> > > +#include "kvm/kvm-cpu.h"
> > > +#include "kvm/util.h"
> > > +
> > > +#include <linux/byteorder.h>
> > > +#include <linux/types.h>
> > > +
> > > +#define ARM_PVTIME_STRUCT_SIZE		(64)
> > > +
> > > +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__alloc_region(struct kvm *kvm)
> > > +{
> > > +	char *mem;
> > > +	int ret = 0;
> > > +
> > > +	mem = mmap(NULL, ARM_PVTIME_MMIO_SIZE, PROT_RW,
> > > +		   MAP_ANON_NORESERVE, -1, 0);
> > > +	if (mem == MAP_FAILED)
> > > +		return -ENOMEM;
> > 
> > Hm... man 2 mmap lists a few dozen error codes, why use -ENOMEM here instead of
> > -errno? This just makes debugging harder.
> > 
> 
> I will return the -errno error code here.
> 
> > > +
> > > +	ret = kvm__register_dev_mem(kvm, ARM_PVTIME_MMIO_BASE,
> > > +				    ARM_PVTIME_MMIO_SIZE, mem);
> > > +	if (ret) {
> > > +		munmap(mem, ARM_PVTIME_MMIO_SIZE);
> > > +		return ret;
> > > +	}
> > > +
> > > +	pvtime_data.usr_mem = mem;
> > > +	return ret;
> > > +}
> > > +
> > > +static int pvtime__teardown_region(struct kvm *kvm)
> > > +{
> > > +	if (pvtime_data.usr_mem == NULL)
> > > +		return 0;
> > > +
> > > +	kvm__destroy_mem(kvm, ARM_PVTIME_MMIO_BASE,
> > > +			 ARM_PVTIME_MMIO_SIZE, pvtime_data.usr_mem);
> > > +	munmap(pvtime_data.usr_mem, ARM_PVTIME_MMIO_SIZE);
> > > +	pvtime_data.usr_mem = NULL;
> > > +	return 0;
> > > +}
> > > +
> > > +void kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu)
> > > +{
> > > +	int ret;
> > > +	u64 pvtime_guest_addr = ARM_PVTIME_MMIO_BASE + vcpu->cpu_id *
> > 
> > That's trange, cpu_id is not initialized here because target->init() is called
> > before setting up the cpu_id. The following patch in the series, "aarch64:
> > Populate the vCPU struct before target->init()" should come before this one.
> > 
> > > +		ARM_PVTIME_STRUCT_SIZE;
> > > +	struct kvm_config *kvm_cfg = NULL;
> > > +	struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> > > +		.group	= KVM_ARM_VCPU_PVTIME_CTRL,
> > > +		.addr	= KVM_ARM_VCPU_PVTIME_IPA
> > > +	};
> > > +
> > > +	BUG_ON(!vcpu);
> > > +	BUG_ON(!vcpu->kvm);
> > > +
> > > +	kvm_cfg = &vcpu->kvm->cfg;
> > > +	if (kvm_cfg && kvm_cfg->no_pvtime)
> > 
> > If you move the next patch in the series before this one, all the above checks
> > will not be needed and should be removed.
> > 
> > In general, each patch in a series should be able to build properly and run a VM
> > without errors. This is to help users when bisecting [1]. If I build kvmtool
> > from this patch and I try to run a VM I get the error:
> > 
> > Error: BUG at arm/aarch64/pvtime.c:65
> > 
> > [1] https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#separate-your-changes
> > 
> 
> I will move the patch that handles the structure initialisation before
> this one and I will remove those checks. Thanks for the suggestion !
> 
> > > +		return;
> > > +
> > > +	if (!pvtime_data.is_supported)
> > > +		return;
> > > +
> > > +	ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> > > +	if (ret)
> > > +		goto out_err;
> > 
> > You should check that pvtime is supported by checking the KVM_CAP_STEAL_TIME on
> > the VM fd, as that's how capabilities are advertised by KVM.
> > 
> 
> I thought that it is sufficient to verify that it has the device
> attributes for *PVTIME. I will add this check too, thanks for the
> suggestion !
> 
> > > +
> > > +	if (!pvtime_data.usr_mem) {
> > > +		ret = pvtime__alloc_region(vcpu->kvm);
> > 
> > pvtime__alloc_region() can fail pretty catastrophically, is it ok to ignore it
> > and go on? I would have expected kvm_cpu__setup_pvtime() to return an error
> > which is then propagated to target->init().
> >
> 
> Hmm, I didn't propagate the error code from kvm_cpu__setup_pvtime()
> because if this feature is not supported it will hit:"Unable to
> initialise vcpu".

That's totally fine. If the feature is not supported by KVM, then return 0
from kvm_cpu__setup_pvtime(). But if the capability is supported by KVM and
kvmtool encounters and error when it tries to configure it, then kvmtool
should abort and complain loudly about it.

For example, kvmtool already unconditionally tries to enable SVE if KVM
supports it (look at kvm_cpu__arch_init() ->
kvm_cpu__configure_features()), and it dies if it cannot finalize the VCPU
(which is the final stage in SVE configuration).

> 
> But I guess that now we can propagate the error code as we have '--no-pvtime'
> command argument. What do you think ?

Yes, please propagate the error.

Thanks,
Alex

> 
> > > +		if (ret)
> > > +			goto out_err;
> > > +	}
> > > +
> > > +	pvtime_attr.addr = (u64)&pvtime_guest_addr;
> > > +	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> > > +	if (!ret)
> > > +		return;
> > > +
> > > +	pvtime__teardown_region(vcpu->kvm);
> > > +out_err:
> > > +	pvtime_data.is_supported = false;
> > > +}
> > > +
> > > +dev_exit(pvtime__teardown_region);
> > 
> > It is customary to put the dev_exit() exactly after the function it refers to.
> > 
> 
> I will move this.
> 
> > > diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> > > index c645ac0..3f82663 100644
> > > --- a/arm/include/arm-common/kvm-arch.h
> > > +++ b/arm/include/arm-common/kvm-arch.h
> > > @@ -15,7 +15,8 @@
> > >   * |  PCI  |////| plat  |       |        |     |         |
> > >   * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
> > >   * | space |////| UART, |       |  MMIO  |     |  (AXI)  |
> > > - * |       |////| RTC   |       |        |     |         |
> > > + * |       |////| RTC,  |       |        |     |         |
> > > + * |       |////| PVTIME|       |        |     |         |
> > >   * +-------+----+-------+-------+--------+-----+---------+---......
> > >   */
> > >  
> > > @@ -34,6 +35,9 @@
> > >  #define ARM_RTC_MMIO_BASE	(ARM_UART_MMIO_BASE + ARM_UART_MMIO_SIZE)
> > >  #define ARM_RTC_MMIO_SIZE	0x10000
> > >  
> > > +#define ARM_PVTIME_MMIO_BASE	(ARM_RTC_MMIO_BASE + ARM_RTC_MMIO_SIZE)
> > > +#define ARM_PVTIME_MMIO_SIZE	SZ_64K
> > 
> > This looks good.
> > 
> > Thanks,
> > Alex
> >
> 
> Thanks for the review,
> Sebastian
> 
> > > +
> > >  #define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
> > >  #define KVM_FLASH_MAX_SIZE	0x1000000
> > >  
> > > diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> > > index 6a5720c..48adf27 100644
> > > --- a/include/kvm/kvm-config.h
> > > +++ b/include/kvm/kvm-config.h
> > > @@ -62,6 +62,7 @@ struct kvm_config {
> > >  	bool no_dhcp;
> > >  	bool ioport_debug;
> > >  	bool mmio_debug;
> > > +	bool no_pvtime;
> > >  };
> > >  
> > >  #endif
> > > -- 
> > > 2.35.1.473.g83b2b277ed-goog
> > > 
> > > _______________________________________________
> > > kvmarm mailing list
> > > kvmarm@lists.cs.columbia.edu
> > > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH kvmtool v4 2/3] aarch64: Populate the vCPU struct before target->init()

[Sebastian Ene]

Move the vCPU structure initialisation before the target->init() call to
 keep a reference to the kvm structure during init().
This is required by the pvtime peripheral to reserve a memory region
while the vCPU is beeing initialised.

Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 arm/kvm-cpu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

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;
-
 	if (kvm_cpu__configure_features(vcpu))
 		die("Unable to configure requested vcpu features");
 
-- 
2.35.1.473.g83b2b277ed-goog

[PATCH kvmtool v4 3/3] Add --no-pvtime command line argument

[Sebastian Ene]

The command line argument disables the stolen time functionality when is
specified.

Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 builtin-run.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin-run.c b/builtin-run.c
index 9a1a0c1..7c8be9d 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -128,6 +128,8 @@ void kvm_run_set_wrapper_sandbox(void)
 			" rootfs"),					\
 	OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path",	\
 			"Hugetlbfs path"),				\
+	OPT_BOOLEAN('\0', "no-pvtime", &(cfg)->no_pvtime, "Disable"	\
+			" stolen time"),				\
 									\
 	OPT_GROUP("Kernel options:"),					\
 	OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel",	\
-- 
2.35.1.473.g83b2b277ed-goog