* [PATCH kvmtool v7 0/3] aarch64: Add stolen time support @ 2022-03-02 14:07 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: maz, will, kvmarm These patches add support for stolen time functionality. Patch #1 moves the vCPU structure initialisation before the target->init() call to allow early access to the kvm structure from the vCPU during target->init(). Patch #2 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. Stolen time is enabled by default when the host supports KVM_CAP_STEAL_TIME. Patch #3 adds a new command line argument to disable the stolen time functionality(by default is enabled). Changelog since v6: - fix perror number of arguments Changelog since v5: - propagate the error code from the kvm_cpu__setup_pvtime() when the host supports KVM_CAP_STEAL_TIME but if fails to configure it for stolen time functionality. Sebastian Ene (3): aarch64: Populate the vCPU struct before target->init() aarch64: Add stolen time support Add --no-pvtime command line argument 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 +- arm/kvm-cpu.c | 14 ++-- builtin-run.c | 2 + include/kvm/kvm-config.h | 1 + 8 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 arm/aarch64/pvtime.c -- 2.35.1.574.g5d30c73bfb-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 0/3] aarch64: Add stolen time support @ 2022-03-02 14:07 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: qperret, maz, kvmarm, will, julien.thierry.kdev, Sebastian Ene These patches add support for stolen time functionality. Patch #1 moves the vCPU structure initialisation before the target->init() call to allow early access to the kvm structure from the vCPU during target->init(). Patch #2 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. Stolen time is enabled by default when the host supports KVM_CAP_STEAL_TIME. Patch #3 adds a new command line argument to disable the stolen time functionality(by default is enabled). Changelog since v6: - fix perror number of arguments Changelog since v5: - propagate the error code from the kvm_cpu__setup_pvtime() when the host supports KVM_CAP_STEAL_TIME but if fails to configure it for stolen time functionality. Sebastian Ene (3): aarch64: Populate the vCPU struct before target->init() aarch64: Add stolen time support Add --no-pvtime command line argument 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 +- arm/kvm-cpu.c | 14 ++-- builtin-run.c | 2 + include/kvm/kvm-config.h | 1 + 8 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 arm/aarch64/pvtime.c -- 2.35.1.574.g5d30c73bfb-goog ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 1/3] aarch64: Populate the vCPU struct before target->init() 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-02 14:07 ` Sebastian Ene -1 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: maz, will, kvmarm 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.574.g5d30c73bfb-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 1/3] aarch64: Populate the vCPU struct before target->init() @ 2022-03-02 14:07 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: qperret, maz, kvmarm, will, julien.thierry.kdev, 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.574.g5d30c73bfb-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 1/3] aarch64: Populate the vCPU struct before target->init() 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-02 14:21 ` Marc Zyngier -1 siblings, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2022-03-02 14:21 UTC (permalink / raw) To: Sebastian Ene; +Cc: will, kvmarm, kvm On Wed, 02 Mar 2022 14:07:33 +0000, Sebastian Ene <sebastianene@google.com> wrote: > > 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> Reviewed-by: Marc Zyngier <maz@kernel.org> M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 1/3] aarch64: Populate the vCPU struct before target->init() @ 2022-03-02 14:21 ` Marc Zyngier 0 siblings, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2022-03-02 14:21 UTC (permalink / raw) To: Sebastian Ene; +Cc: kvm, qperret, kvmarm, will, julien.thierry.kdev On Wed, 02 Mar 2022 14:07:33 +0000, Sebastian Ene <sebastianene@google.com> wrote: > > 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> Reviewed-by: Marc Zyngier <maz@kernel.org> M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-02 14:07 ` Sebastian Ene -1 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: maz, will, kvmarm 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 | 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); + +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) +{ + int ret; + bool has_stolen_time; + 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 + }; + + kvm_cfg = &vcpu->kvm->cfg; + if (kvm_cfg->no_pvtime) + return 0; + + if (!pvtime_data.is_supported) + return -ENOTSUP; + + has_stolen_time = kvm__supports_extension(vcpu->kvm, + KVM_CAP_STEAL_TIME); + if (!has_stolen_time) + return 0; + + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr); + if (ret) { + perror("KVM_HAS_DEVICE_ATTR failed\n"); + goto out_err; + } + + if (!pvtime_data.usr_mem) { + ret = pvtime__alloc_region(vcpu->kvm); + if (ret) { + perror("Failed allocating pvtime region\n"); + goto out_err; + } + } + + pvtime_attr.addr = (u64)&pvtime_guest_addr; + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr); + if (!ret) + return 0; + + perror("KVM_SET_DEVICE_ATTR failed\n"); + pvtime__teardown_region(vcpu->kvm); +out_err: + pvtime_data.is_supported = false; + return ret; +} 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.574.g5d30c73bfb-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 2/3] aarch64: Add stolen time support @ 2022-03-02 14:07 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: qperret, maz, kvmarm, will, julien.thierry.kdev, 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 | 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); + +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) +{ + int ret; + bool has_stolen_time; + 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 + }; + + kvm_cfg = &vcpu->kvm->cfg; + if (kvm_cfg->no_pvtime) + return 0; + + if (!pvtime_data.is_supported) + return -ENOTSUP; + + has_stolen_time = kvm__supports_extension(vcpu->kvm, + KVM_CAP_STEAL_TIME); + if (!has_stolen_time) + return 0; + + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr); + if (ret) { + perror("KVM_HAS_DEVICE_ATTR failed\n"); + goto out_err; + } + + if (!pvtime_data.usr_mem) { + ret = pvtime__alloc_region(vcpu->kvm); + if (ret) { + perror("Failed allocating pvtime region\n"); + goto out_err; + } + } + + pvtime_attr.addr = (u64)&pvtime_guest_addr; + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr); + if (!ret) + return 0; + + perror("KVM_SET_DEVICE_ATTR failed\n"); + pvtime__teardown_region(vcpu->kvm); +out_err: + pvtime_data.is_supported = false; + return ret; +} 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.574.g5d30c73bfb-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-02 14:41 ` Marc Zyngier -1 siblings, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2022-03-02 14:41 UTC (permalink / raw) To: Sebastian Ene; +Cc: will, kvmarm, kvm Hi Sebastian, On Wed, 02 Mar 2022 14:07:35 +0000, Sebastian Ene <sebastianene@google.com> 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 | 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. > + 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); > + > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) > +{ > + int ret; > + bool has_stolen_time; > + 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 > + }; > + > + kvm_cfg = &vcpu->kvm->cfg; > + if (kvm_cfg->no_pvtime) > + return 0; > + > + if (!pvtime_data.is_supported) > + return -ENOTSUP; It is a bit odd to have this hard failure if running on a system that doesn't have pvtime. It forces the user to alter their command-line, which is a bit annoying. I'd rather have a soft-fail here. > + > + has_stolen_time = kvm__supports_extension(vcpu->kvm, > + KVM_CAP_STEAL_TIME); > + if (!has_stolen_time) > + return 0; > + > + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr); > + if (ret) { > + perror("KVM_HAS_DEVICE_ATTR failed\n"); > + goto out_err; > + } > + > + if (!pvtime_data.usr_mem) { > + ret = pvtime__alloc_region(vcpu->kvm); > + if (ret) { > + perror("Failed allocating pvtime region\n"); > + goto out_err; > + } > + } > + > + pvtime_attr.addr = (u64)&pvtime_guest_addr; > + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr); > + if (!ret) > + return 0; > + > + perror("KVM_SET_DEVICE_ATTR failed\n"); > + pvtime__teardown_region(vcpu->kvm); > +out_err: > + pvtime_data.is_supported = false; > + return ret; > +} > 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 Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support @ 2022-03-02 14:41 ` Marc Zyngier 0 siblings, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2022-03-02 14:41 UTC (permalink / raw) To: Sebastian Ene; +Cc: kvm, qperret, kvmarm, will, julien.thierry.kdev Hi Sebastian, On Wed, 02 Mar 2022 14:07:35 +0000, Sebastian Ene <sebastianene@google.com> 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 | 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. > + 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); > + > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) > +{ > + int ret; > + bool has_stolen_time; > + 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 > + }; > + > + kvm_cfg = &vcpu->kvm->cfg; > + if (kvm_cfg->no_pvtime) > + return 0; > + > + if (!pvtime_data.is_supported) > + return -ENOTSUP; It is a bit odd to have this hard failure if running on a system that doesn't have pvtime. It forces the user to alter their command-line, which is a bit annoying. I'd rather have a soft-fail here. > + > + has_stolen_time = kvm__supports_extension(vcpu->kvm, > + KVM_CAP_STEAL_TIME); > + if (!has_stolen_time) > + return 0; > + > + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr); > + if (ret) { > + perror("KVM_HAS_DEVICE_ATTR failed\n"); > + goto out_err; > + } > + > + if (!pvtime_data.usr_mem) { > + ret = pvtime__alloc_region(vcpu->kvm); > + if (ret) { > + perror("Failed allocating pvtime region\n"); > + goto out_err; > + } > + } > + > + pvtime_attr.addr = (u64)&pvtime_guest_addr; > + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr); > + if (!ret) > + return 0; > + > + perror("KVM_SET_DEVICE_ATTR failed\n"); > + pvtime__teardown_region(vcpu->kvm); > +out_err: > + pvtime_data.is_supported = false; > + return ret; > +} > 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 Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-02 14:41 ` Marc Zyngier (?) @ 2022-03-03 12:01 ` Sebastian Ene 2022-03-03 17:51 ` Marc Zyngier -1 siblings, 1 reply; 20+ messages in thread From: Sebastian Ene @ 2022-03-03 12:01 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, qperret, will, julien.thierry.kdev On Wed, Mar 02, 2022 at 02:41:07PM +0000, Marc Zyngier wrote: > Hi Sebastian, > Hello Marc, > On Wed, 02 Mar 2022 14:07:35 +0000, > Sebastian Ene <sebastianene@google.com> 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 | 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. > I will update the pointer to void * type. > > +}; > > + > > +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. > I will remove the 'MMIO' part of the name: ARM_PVTIME_SIZE and ARM_PVTIME_BASE will be the new definitions. Initially I thought to keep the 'MMIO' name as this ram portion is placed after RTC_MMIO region but I agree that it's 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. > Thanks for the explanation ! Nice catch. I will update the call and use kvm__register_ram(). > > + 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); > > + > > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) > > +{ > > + int ret; > > + bool has_stolen_time; > > + 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 > > + }; > > + > > + kvm_cfg = &vcpu->kvm->cfg; > > + if (kvm_cfg->no_pvtime) > > + return 0; > > + > > + if (!pvtime_data.is_supported) > > + return -ENOTSUP; > > It is a bit odd to have this hard failure if running on a system that > doesn't have pvtime. It forces the user to alter their command-line, > which is a bit annoying. I'd rather have a soft-fail here. > The flag 'is_supported' is set to false when we support pvtime but we fail to configure it. We verify that we support pvtime by calling the check extension KVM_CAP_STEAL_TIME. I think the naming is odd here for the flag name. It should be : 'is_failed_cfg'. > > + > > + has_stolen_time = kvm__supports_extension(vcpu->kvm, > > + KVM_CAP_STEAL_TIME); > > + if (!has_stolen_time) > > + return 0; > > + > > + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr); > > + if (ret) { > > + perror("KVM_HAS_DEVICE_ATTR failed\n"); > > + goto out_err; > > + } > > + > > + if (!pvtime_data.usr_mem) { > > + ret = pvtime__alloc_region(vcpu->kvm); > > + if (ret) { > > + perror("Failed allocating pvtime region\n"); > > + goto out_err; > > + } > > + } > > + > > + pvtime_attr.addr = (u64)&pvtime_guest_addr; > > + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr); > > + if (!ret) > > + return 0; > > + > > + perror("KVM_SET_DEVICE_ATTR failed\n"); > > + pvtime__teardown_region(vcpu->kvm); > > +out_err: > > + pvtime_data.is_supported = false; > > + return ret; > > +} > > 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 > > Thanks, > Thanks for the review, > M. > Sebastian > -- > Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-03 12:01 ` Sebastian Ene @ 2022-03-03 17:51 ` Marc Zyngier 2022-03-07 11:46 ` Sebastian Ene 0 siblings, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2022-03-03 17:51 UTC (permalink / raw) To: Sebastian Ene; +Cc: kvm, qperret, will, julien.thierry.kdev On Thu, 03 Mar 2022 12:01:10 +0000, Sebastian Ene <sebastianene@google.com> wrote: > > > > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) > > > +{ > > > + int ret; > > > + bool has_stolen_time; > > > + 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 > > > + }; > > > + > > > + kvm_cfg = &vcpu->kvm->cfg; > > > + if (kvm_cfg->no_pvtime) > > > + return 0; > > > + > > > + if (!pvtime_data.is_supported) > > > + return -ENOTSUP; > > > > It is a bit odd to have this hard failure if running on a system that > > doesn't have pvtime. It forces the user to alter their command-line, > > which is a bit annoying. I'd rather have a soft-fail here. > > > > The flag 'is_supported' is set to false when we support pvtime but we > fail to configure it. We verify that we support pvtime by calling the check > extension KVM_CAP_STEAL_TIME. I think the naming is odd here for the > flag name. It should be : 'is_failed_cfg'. Ah, I see. Yes, the name is misleading. > > > > + > > > + has_stolen_time = kvm__supports_extension(vcpu->kvm, > > > + KVM_CAP_STEAL_TIME); > > > + if (!has_stolen_time) > > > + return 0; Here, you could force no_pvtime to 1, and avoid checking for each vcpu once you detected that the host is not equipped to deal with it. Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-03 17:51 ` Marc Zyngier @ 2022-03-07 11:46 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-07 11:46 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, qperret, will, julien.thierry.kdev On Thu, Mar 03, 2022 at 05:51:36PM +0000, Marc Zyngier wrote: > On Thu, 03 Mar 2022 12:01:10 +0000, > Sebastian Ene <sebastianene@google.com> wrote: Hi, > > > > > > +int kvm_cpu__setup_pvtime(struct kvm_cpu *vcpu) > > > > +{ > > > > + int ret; > > > > + bool has_stolen_time; > > > > + 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 > > > > + }; > > > > + > > > > + kvm_cfg = &vcpu->kvm->cfg; > > > > + if (kvm_cfg->no_pvtime) > > > > + return 0; > > > > + > > > > + if (!pvtime_data.is_supported) > > > > + return -ENOTSUP; > > > > > > It is a bit odd to have this hard failure if running on a system that > > > doesn't have pvtime. It forces the user to alter their command-line, > > > which is a bit annoying. I'd rather have a soft-fail here. > > > > > > > The flag 'is_supported' is set to false when we support pvtime but we > > fail to configure it. We verify that we support pvtime by calling the check > > extension KVM_CAP_STEAL_TIME. I think the naming is odd here for the > > flag name. It should be : 'is_failed_cfg'. > > Ah, I see. Yes, the name is misleading. > I will update the name for this to: 'is_failed_cfg'. > > > > > > + > > > > + has_stolen_time = kvm__supports_extension(vcpu->kvm, > > > > + KVM_CAP_STEAL_TIME); > > > > + if (!has_stolen_time) > > > > + return 0; > > Here, you could force no_pvtime to 1, and avoid checking for each vcpu > once you detected that the host is not equipped to deal with it. > Good point ! I will do this. Thanks, Sebastian > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-02 14:41 ` Marc Zyngier @ 2022-03-07 10:52 ` Alexandru Elisei -1 siblings, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-03-07 10:52 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, will, kvmarm 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@google.com> 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 | 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support @ 2022-03-07 10:52 ` Alexandru Elisei 0 siblings, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-03-07 10:52 UTC (permalink / raw) To: Marc Zyngier Cc: Sebastian Ene, kvm, qperret, kvmarm, will, julien.thierry.kdev 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@google.com> 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 | 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-07 11:46 ` Alexandru Elisei -1 siblings, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-03-07 11:46 UTC (permalink / raw) To: Sebastian Ene; +Cc: maz, will, kvmarm, kvm 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@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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support @ 2022-03-07 11:46 ` Alexandru Elisei 0 siblings, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-03-07 11:46 UTC (permalink / raw) To: Sebastian Ene; +Cc: kvm, maz, will, kvmarm 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@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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool v7 2/3] aarch64: Add stolen time support 2022-03-07 11:46 ` Alexandru Elisei (?) @ 2022-03-07 14:55 ` Sebastian Ene -1 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-07 14:55 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, qperret, will, julien.thierry.kdev, maz 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@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 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 3/3] Add --no-pvtime command line argument 2022-03-02 14:07 ` Sebastian Ene @ 2022-03-02 14:07 ` Sebastian Ene -1 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: maz, will, kvmarm 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.574.g5d30c73bfb-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH kvmtool v7 3/3] Add --no-pvtime command line argument @ 2022-03-02 14:07 ` Sebastian Ene 0 siblings, 0 replies; 20+ messages in thread From: Sebastian Ene @ 2022-03-02 14:07 UTC (permalink / raw) To: kvm; +Cc: qperret, maz, kvmarm, will, julien.thierry.kdev, 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.574.g5d30c73bfb-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-03-07 14:55 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-02 14:07 [PATCH kvmtool v7 0/3] aarch64: Add stolen time support Sebastian Ene 2022-03-02 14:07 ` Sebastian Ene 2022-03-02 14:07 ` [PATCH kvmtool v7 1/3] aarch64: Populate the vCPU struct before target->init() Sebastian Ene 2022-03-02 14:07 ` Sebastian Ene 2022-03-02 14:21 ` Marc Zyngier 2022-03-02 14:21 ` Marc Zyngier 2022-03-02 14:07 ` [PATCH kvmtool v7 2/3] aarch64: Add stolen time support Sebastian Ene 2022-03-02 14:07 ` Sebastian Ene 2022-03-02 14:41 ` Marc Zyngier 2022-03-02 14:41 ` Marc Zyngier 2022-03-03 12:01 ` Sebastian Ene 2022-03-03 17:51 ` Marc Zyngier 2022-03-07 11:46 ` Sebastian Ene 2022-03-07 10:52 ` Alexandru Elisei 2022-03-07 10:52 ` Alexandru Elisei 2022-03-07 11:46 ` Alexandru Elisei 2022-03-07 11:46 ` Alexandru Elisei 2022-03-07 14:55 ` Sebastian Ene 2022-03-02 14:07 ` [PATCH kvmtool v7 3/3] Add --no-pvtime command line argument Sebastian Ene 2022-03-02 14:07 ` Sebastian Ene
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.