Hi, On Wed, Mar 02, 2022 at 02:41:07PM +0000, Marc Zyngier wrote: > Hi Sebastian, > > On Wed, 02 Mar 2022 14:07:35 +0000, > Sebastian Ene <sebastianene@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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; > > Consider using void * for pointers that do not have any particular > semantics associated to them. > > > +}; > > + > > +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, > > I sort of object to the 'MMIO' part of the name. The spec is quite > clear that this should be normal memory. That's purely cosmetic, but > still a bit confusing. > > > + 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); > > This, on the other side, is wrong. Since the pvtime pages are memory, > mapping them with device attributes will do the wrong thing (the > hypervisor will write to a cacheable mapping, and the guest will > bypass the cache due to the S2 override that you provide here). > > kvm__register_ram() is more likely to lead to the behaviour you'd > expect. The type of memory only has meaning for kvmtool, it doesn't affect the memslot that is created. Both kvm__register_dev_mem() and kvm__register_ram() create the exact same type of memslot. But yes, I agree, this should be kvm__kvm_register_ram(), because this matches the behaviour described in Documentation/virt/kvm/arm/pvtime.rst, where this shared region is "[..] a reserved region of the normal memory given to the guest". Thanks, Alex