On Mon, Mar 07, 2022 at 11:46:09AM +0000, Alexandru Elisei wrote:
> Hi,
>
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 <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;
> > +};
> > +
> > +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 for the feedback, I will take the first approach as 'pvtime'
is not really a device. I did this as part of patch v8.
Thanks,
Sebastian
> Thanks,
> Alex
