Hi,

On Wed, Mar 02, 2022 at 02:07:35PM +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 <sebastian...@google.com>
> ---
>  Makefile                               |   1 +
>  arm/aarch64/arm-cpu.c                  |   2 +-
>  arm/aarch64/include/kvm/kvm-cpu-arch.h |   1 +
>  arm/aarch64/pvtime.c                   | 103 +++++++++++++++++++++++++
>  arm/include/arm-common/kvm-arch.h      |   6 +-
>  include/kvm/kvm-config.h               |   1 +
>  6 files changed, 112 insertions(+), 2 deletions(-)
>  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..7e4a3c1 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -22,7 +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;
> -     return 0;
> +     return kvm_cpu__setup_pvtime(vcpu);
>  }
>  
>  static struct kvm_arm_target target_generic_v8 = {
> diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h 
> b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> index 8dfb82e..2b2c1ff 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);
> +int 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..fdde683
> --- /dev/null
> +++ b/arm/aarch64/pvtime.c
> @@ -0,0 +1,103 @@
> +#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 -errno;
> +
> +     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;
> +}
> +
> +dev_exit(pvtime__teardown_region);

This looks awkward: pvtime initialization is done in kvm_cpu__arch_init(), but
teardown is done in the device exit stage.

I think it would be better to choose one approach and stick with it: (1) keep
initialization in kvm_cpu__arch_init() and move teardown to kvm_cpu__delete();
or (2) treat pvtime as a device, move the code to hw/pvtime.c, compile the file
only for arm64 and move initialization to dev_init() (and keep teardown in
dev_exit()).

I have no preference for either, but I think a consistent approach to enabling
pvtime is desirable.

Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reply via email to