* [PATCH kvmtool] aarch64: Add stolen time support
@ 2022-02-16 16:16 Sebastian Ene
2022-02-16 16:59 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Ene @ 2022-02-16 16:16 UTC (permalink / raw)
To: kvm; +Cc: qperret, maz, kvmarm, will, julien.thierry.kdev
This patch add 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. 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 | 3 +-
arm/aarch64/arm-cpu.c | 2 +
arm/aarch64/include/kvm/pvtime.h | 6 +++
arm/aarch64/pvtime.c | 83 +++++++++++++++++++++++++++++++
arm/include/arm-common/kvm-arch.h | 6 +++
arm/kvm-cpu.c | 14 +++---
6 files changed, 106 insertions(+), 8 deletions(-)
create mode 100644 arm/aarch64/include/kvm/pvtime.h
create mode 100644 arm/aarch64/pvtime.c
diff --git a/Makefile b/Makefile
index f251147..282ae99 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
@@ -582,4 +583,4 @@ ifneq ($(MAKECMDGOALS),clean)
KVMTOOLS-VERSION-FILE:
@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
-endif
\ No newline at end of file
+endif
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index d7572b7..80bf83a 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -2,6 +2,7 @@
#include "kvm/kvm.h"
#include "kvm/kvm-cpu.h"
#include "kvm/util.h"
+#include "kvm/pvtime.h"
#include "arm-common/gic.h"
#include "arm-common/timer.h"
@@ -22,6 +23,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;
+ pvtime__setup_vcpu(vcpu);
return 0;
}
diff --git a/arm/aarch64/include/kvm/pvtime.h b/arm/aarch64/include/kvm/pvtime.h
new file mode 100644
index 0000000..c31f019
--- /dev/null
+++ b/arm/aarch64/include/kvm/pvtime.h
@@ -0,0 +1,6 @@
+#ifndef KVM__PVTIME_H
+#define KVM__PVTIME_H
+
+void pvtime__setup_vcpu(struct kvm_cpu *vcpu);
+
+#endif /* KVM__PVTIME_H */
diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
new file mode 100644
index 0000000..eb92388
--- /dev/null
+++ b/arm/aarch64/pvtime.c
@@ -0,0 +1,83 @@
+#include "kvm/kvm.h"
+#include "kvm/kvm-cpu.h"
+#include "kvm/util.h"
+#include "kvm/pvtime.h"
+
+#include <linux/byteorder.h>
+#include <linux/types.h>
+
+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__aloc_region(struct kvm *kvm)
+{
+ char *mem;
+ int ret = 0;
+
+ mem = mmap(NULL, AARCH64_PVTIME_IPA_MAX_SIZE, PROT_RW,
+ MAP_ANON_NORESERVE, -1, 0);
+ if (mem == MAP_FAILED)
+ return -ENOMEM;
+
+ ret = kvm__register_dev_mem(kvm, AARCH64_PVTIME_IPA_START,
+ AARCH64_PVTIME_IPA_MAX_SIZE, mem);
+ if (ret) {
+ munmap(mem, AARCH64_PVTIME_IPA_MAX_SIZE);
+ return ret;
+ }
+
+ pvtime_data.usr_mem = mem;
+ return ret;
+}
+
+static int pvtime__teardown_region(struct kvm *kvm)
+{
+ kvm__destroy_mem(kvm, AARCH64_PVTIME_IPA_START,
+ AARCH64_PVTIME_IPA_MAX_SIZE, pvtime_data.usr_mem);
+ munmap(pvtime_data.usr_mem, AARCH64_PVTIME_IPA_MAX_SIZE);
+ pvtime_data.usr_mem = NULL;
+ return 0;
+}
+
+void pvtime__setup_vcpu(struct kvm_cpu *vcpu)
+{
+ int ret;
+ u64 pvtime_guest_addr = AARCH64_PVTIME_IPA_START + vcpu->cpu_id *
+ AARCH64_PVTIME_SIZE;
+ struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
+ .group = KVM_ARM_VCPU_PVTIME_CTRL,
+ .addr = KVM_ARM_VCPU_PVTIME_IPA
+ };
+
+ if (!pvtime_data.is_supported)
+ return;
+
+ if (!pvtime_data.usr_mem) {
+ ret = pvtime__aloc_region(vcpu->kvm);
+ if (ret)
+ goto out_err_alloc;
+ }
+
+ ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
+ if (ret)
+ goto out_err_attr;
+
+ pvtime_attr.addr = (u64)&pvtime_guest_addr;
+ ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
+ if (!ret)
+ return;
+
+out_err_attr:
+ pvtime__teardown_region(vcpu->kvm);
+out_err_alloc:
+ 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..7b683d6 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -54,6 +54,12 @@
#define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
+#define AARCH64_PVTIME_IPA_MAX_SIZE (0x10000)
+#define AARCH64_PROTECTED_VM_FW_MAX_SIZE (0x200000)
+#define AARCH64_PVTIME_IPA_START (ARM_MEMORY_AREA - \
+ AARCH64_PROTECTED_VM_FW_MAX_SIZE - \
+ AARCH64_PVTIME_IPA_MAX_SIZE)
+#define AARCH64_PVTIME_SIZE (64)
#define ARM_LOMAP_MAX_MEMORY ((1ULL << 32) - ARM_MEMORY_AREA)
#define ARM_HIMAP_MAX_MEMORY ((1ULL << 40) - ARM_MEMORY_AREA)
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.265.g69c8d7142f-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH kvmtool] aarch64: Add stolen time support
2022-02-16 16:16 [PATCH kvmtool] aarch64: Add stolen time support Sebastian Ene
@ 2022-02-16 16:59 ` Marc Zyngier
2022-02-17 15:05 ` Sebastian Ene
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2022-02-16 16:59 UTC (permalink / raw)
To: Sebastian Ene; +Cc: kvm, qperret, kvmarm, will, julien.thierry.kdev
Hi Sebastian,
Thanks for looking into this. A few comments below.
On 2022-02-16 16:16, Sebastian Ene wrote:
> This patch add 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. 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 | 3 +-
> arm/aarch64/arm-cpu.c | 2 +
> arm/aarch64/include/kvm/pvtime.h | 6 +++
> arm/aarch64/pvtime.c | 83 +++++++++++++++++++++++++++++++
> arm/include/arm-common/kvm-arch.h | 6 +++
> arm/kvm-cpu.c | 14 +++---
> 6 files changed, 106 insertions(+), 8 deletions(-)
> create mode 100644 arm/aarch64/include/kvm/pvtime.h
> create mode 100644 arm/aarch64/pvtime.c
>
> diff --git a/Makefile b/Makefile
> index f251147..282ae99 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
>
> @@ -582,4 +583,4 @@ ifneq ($(MAKECMDGOALS),clean)
>
> KVMTOOLS-VERSION-FILE:
> @$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> -endif
> \ No newline at end of file
> +endif
Spurious change?
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index d7572b7..80bf83a 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -2,6 +2,7 @@
> #include "kvm/kvm.h"
> #include "kvm/kvm-cpu.h"
> #include "kvm/util.h"
> +#include "kvm/pvtime.h"
>
> #include "arm-common/gic.h"
> #include "arm-common/timer.h"
> @@ -22,6 +23,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;
> + pvtime__setup_vcpu(vcpu);
> return 0;
> }
>
> diff --git a/arm/aarch64/include/kvm/pvtime.h
> b/arm/aarch64/include/kvm/pvtime.h
> new file mode 100644
> index 0000000..c31f019
> --- /dev/null
> +++ b/arm/aarch64/include/kvm/pvtime.h
> @@ -0,0 +1,6 @@
> +#ifndef KVM__PVTIME_H
> +#define KVM__PVTIME_H
> +
> +void pvtime__setup_vcpu(struct kvm_cpu *vcpu);
> +
> +#endif /* KVM__PVTIME_H */
How about sticking this in kvm-cpu-arch.h instead? A whole new include
file
for just a prototype isn't totally warranted.
> diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> new file mode 100644
> index 0000000..eb92388
> --- /dev/null
> +++ b/arm/aarch64/pvtime.c
> @@ -0,0 +1,83 @@
> +#include "kvm/kvm.h"
> +#include "kvm/kvm-cpu.h"
> +#include "kvm/util.h"
> +#include "kvm/pvtime.h"
> +
> +#include <linux/byteorder.h>
> +#include <linux/types.h>
> +
> +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__aloc_region(struct kvm *kvm)
s/aloc/alloc/ ?
> +{
> + char *mem;
> + int ret = 0;
> +
> + mem = mmap(NULL, AARCH64_PVTIME_IPA_MAX_SIZE, PROT_RW,
> + MAP_ANON_NORESERVE, -1, 0);
> + if (mem == MAP_FAILED)
> + return -ENOMEM;
> +
> + ret = kvm__register_dev_mem(kvm, AARCH64_PVTIME_IPA_START,
> + AARCH64_PVTIME_IPA_MAX_SIZE, mem);
> + if (ret) {
> + munmap(mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> + return ret;
> + }
> +
> + pvtime_data.usr_mem = mem;
> + return ret;
> +}
> +
> +static int pvtime__teardown_region(struct kvm *kvm)
> +{
> + kvm__destroy_mem(kvm, AARCH64_PVTIME_IPA_START,
> + AARCH64_PVTIME_IPA_MAX_SIZE, pvtime_data.usr_mem);
> + munmap(pvtime_data.usr_mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> + pvtime_data.usr_mem = NULL;
> + return 0;
> +}
> +
> +void pvtime__setup_vcpu(struct kvm_cpu *vcpu)
> +{
> + int ret;
> + u64 pvtime_guest_addr = AARCH64_PVTIME_IPA_START + vcpu->cpu_id *
> + AARCH64_PVTIME_SIZE;
> + struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> + .group = KVM_ARM_VCPU_PVTIME_CTRL,
> + .addr = KVM_ARM_VCPU_PVTIME_IPA
> + };
> +
> + if (!pvtime_data.is_supported)
> + return;
> +
> + if (!pvtime_data.usr_mem) {
> + ret = pvtime__aloc_region(vcpu->kvm);
> + if (ret)
> + goto out_err_alloc;
> + }
> +
> + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> + if (ret)
> + goto out_err_attr;
You should check for the stolen time capability before allocating and
mapping
the memory.
> +
> + pvtime_attr.addr = (u64)&pvtime_guest_addr;
> + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> + if (!ret)
> + return;
> +
> +out_err_attr:
> + pvtime__teardown_region(vcpu->kvm);
> +out_err_alloc:
> + 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..7b683d6 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -54,6 +54,12 @@
> #define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
> (ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>
> +#define AARCH64_PVTIME_IPA_MAX_SIZE (0x10000)
SZ_64K?
> +#define AARCH64_PROTECTED_VM_FW_MAX_SIZE (0x200000)
This definitely looks like something that shouldn't be there. Yet.
> +#define AARCH64_PVTIME_IPA_START (ARM_MEMORY_AREA - \
> + AARCH64_PROTECTED_VM_FW_MAX_SIZE - \
> + AARCH64_PVTIME_IPA_MAX_SIZE)
> +#define AARCH64_PVTIME_SIZE (64)
>
> #define ARM_LOMAP_MAX_MEMORY ((1ULL << 32) - ARM_MEMORY_AREA)
> #define ARM_HIMAP_MAX_MEMORY ((1ULL << 40) - ARM_MEMORY_AREA)
> 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;
> -
What is the reason for moving these assignments around?
> if (kvm_cpu__configure_features(vcpu))
> die("Unable to configure requested vcpu features");
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH kvmtool] aarch64: Add stolen time support
2022-02-16 16:59 ` Marc Zyngier
@ 2022-02-17 15:05 ` Sebastian Ene
0 siblings, 0 replies; 3+ messages in thread
From: Sebastian Ene @ 2022-02-17 15:05 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, qperret, kvmarm, will, julien.thierry.kdev
On Wed, Feb 16, 2022 at 04:59:10PM +0000, Marc Zyngier wrote:
> Hi Sebastian,
>
> Thanks for looking into this. A few comments below.
>
Hello Marc,
Thanks for taking the time to review my patch.
> On 2022-02-16 16:16, Sebastian Ene wrote:
> > This patch add 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. 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 | 3 +-
> > arm/aarch64/arm-cpu.c | 2 +
> > arm/aarch64/include/kvm/pvtime.h | 6 +++
> > arm/aarch64/pvtime.c | 83 +++++++++++++++++++++++++++++++
> > arm/include/arm-common/kvm-arch.h | 6 +++
> > arm/kvm-cpu.c | 14 +++---
> > 6 files changed, 106 insertions(+), 8 deletions(-)
> > create mode 100644 arm/aarch64/include/kvm/pvtime.h
> > create mode 100644 arm/aarch64/pvtime.c
> >
> > diff --git a/Makefile b/Makefile
> > index f251147..282ae99 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
> >
> > @@ -582,4 +583,4 @@ ifneq ($(MAKECMDGOALS),clean)
> >
> > KVMTOOLS-VERSION-FILE:
> > @$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> > -endif
> > \ No newline at end of file
> > +endif
>
> Spurious change?
>
Right, ACK I will revert this change.
> > diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> > index d7572b7..80bf83a 100644
> > --- a/arm/aarch64/arm-cpu.c
> > +++ b/arm/aarch64/arm-cpu.c
> > @@ -2,6 +2,7 @@
> > #include "kvm/kvm.h"
> > #include "kvm/kvm-cpu.h"
> > #include "kvm/util.h"
> > +#include "kvm/pvtime.h"
> >
> > #include "arm-common/gic.h"
> > #include "arm-common/timer.h"
> > @@ -22,6 +23,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;
> > + pvtime__setup_vcpu(vcpu);
> > return 0;
> > }
> >
> > diff --git a/arm/aarch64/include/kvm/pvtime.h
> > b/arm/aarch64/include/kvm/pvtime.h
> > new file mode 100644
> > index 0000000..c31f019
> > --- /dev/null
> > +++ b/arm/aarch64/include/kvm/pvtime.h
> > @@ -0,0 +1,6 @@
> > +#ifndef KVM__PVTIME_H
> > +#define KVM__PVTIME_H
> > +
> > +void pvtime__setup_vcpu(struct kvm_cpu *vcpu);
> > +
> > +#endif /* KVM__PVTIME_H */
>
> How about sticking this in kvm-cpu-arch.h instead? A whole new include file
> for just a prototype isn't totally warranted.
>
I think that's a good ideea, I will do this.
> > diff --git a/arm/aarch64/pvtime.c b/arm/aarch64/pvtime.c
> > new file mode 100644
> > index 0000000..eb92388
> > --- /dev/null
> > +++ b/arm/aarch64/pvtime.c
> > @@ -0,0 +1,83 @@
> > +#include "kvm/kvm.h"
> > +#include "kvm/kvm-cpu.h"
> > +#include "kvm/util.h"
> > +#include "kvm/pvtime.h"
> > +
> > +#include <linux/byteorder.h>
> > +#include <linux/types.h>
> > +
> > +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__aloc_region(struct kvm *kvm)
>
> s/aloc/alloc/ ?
>
I will fix the typo.
> > +{
> > + char *mem;
> > + int ret = 0;
> > +
> > + mem = mmap(NULL, AARCH64_PVTIME_IPA_MAX_SIZE, PROT_RW,
> > + MAP_ANON_NORESERVE, -1, 0);
> > + if (mem == MAP_FAILED)
> > + return -ENOMEM;
> > +
> > + ret = kvm__register_dev_mem(kvm, AARCH64_PVTIME_IPA_START,
> > + AARCH64_PVTIME_IPA_MAX_SIZE, mem);
> > + if (ret) {
> > + munmap(mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> > + return ret;
> > + }
> > +
> > + pvtime_data.usr_mem = mem;
> > + return ret;
> > +}
> > +
> > +static int pvtime__teardown_region(struct kvm *kvm)
> > +{
> > + kvm__destroy_mem(kvm, AARCH64_PVTIME_IPA_START,
> > + AARCH64_PVTIME_IPA_MAX_SIZE, pvtime_data.usr_mem);
> > + munmap(pvtime_data.usr_mem, AARCH64_PVTIME_IPA_MAX_SIZE);
> > + pvtime_data.usr_mem = NULL;
> > + return 0;
> > +}
> > +
> > +void pvtime__setup_vcpu(struct kvm_cpu *vcpu)
> > +{
> > + int ret;
> > + u64 pvtime_guest_addr = AARCH64_PVTIME_IPA_START + vcpu->cpu_id *
> > + AARCH64_PVTIME_SIZE;
> > + struct kvm_device_attr pvtime_attr = (struct kvm_device_attr) {
> > + .group = KVM_ARM_VCPU_PVTIME_CTRL,
> > + .addr = KVM_ARM_VCPU_PVTIME_IPA
> > + };
> > +
> > + if (!pvtime_data.is_supported)
> > + return;
> > +
> > + if (!pvtime_data.usr_mem) {
> > + ret = pvtime__aloc_region(vcpu->kvm);
> > + if (ret)
> > + goto out_err_alloc;
> > + }
> > +
> > + ret = ioctl(vcpu->vcpu_fd, KVM_HAS_DEVICE_ATTR, &pvtime_attr);
> > + if (ret)
> > + goto out_err_attr;
>
> You should check for the stolen time capability before allocating and
> mapping
> the memory.
>
Good catch ! I will do this, thanks.
> > +
> > + pvtime_attr.addr = (u64)&pvtime_guest_addr;
> > + ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pvtime_attr);
> > + if (!ret)
> > + return;
> > +
> > +out_err_attr:
> > + pvtime__teardown_region(vcpu->kvm);
> > +out_err_alloc:
> > + 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..7b683d6 100644
> > --- a/arm/include/arm-common/kvm-arch.h
> > +++ b/arm/include/arm-common/kvm-arch.h
> > @@ -54,6 +54,12 @@
> > #define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
> > (ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> >
> > +#define AARCH64_PVTIME_IPA_MAX_SIZE (0x10000)
>
> SZ_64K?
>
I will update the `AARCH64_PVTIME_IPA_MAX_SIZE` defintion with `SZ_64K`.
> > +#define AARCH64_PROTECTED_VM_FW_MAX_SIZE (0x200000)
>
> This definitely looks like something that shouldn't be there. Yet.
>
I agree, I will remove it for the moment.
> > +#define AARCH64_PVTIME_IPA_START (ARM_MEMORY_AREA - \
> > + AARCH64_PROTECTED_VM_FW_MAX_SIZE - \
> > + AARCH64_PVTIME_IPA_MAX_SIZE)
> > +#define AARCH64_PVTIME_SIZE (64)
> >
> > #define ARM_LOMAP_MAX_MEMORY ((1ULL << 32) - ARM_MEMORY_AREA)
> > #define ARM_HIMAP_MAX_MEMORY ((1ULL << 40) - ARM_MEMORY_AREA)
> > 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;
> > -
>
> What is the reason for moving these assignments around?
>
The setup of the pvtime is done during the vcpu initialisation and I
need to access the kvm structure during that time.
> > if (kvm_cpu__configure_features(vcpu))
> > die("Unable to configure requested vcpu features");
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-17 15:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-16 16:16 [PATCH kvmtool] aarch64: Add stolen time support Sebastian Ene
2022-02-16 16:59 ` Marc Zyngier
2022-02-17 15:05 ` Sebastian Ene
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox