[PATCH v7 0/5] arm64: Support perf event modifiers :G and :H

[PATCH v7 0/5] arm64: Support perf event modifiers :G and :H

[Andrew Murray]

This patchset provides support for perf event modifiers :G and :H which
allows for filtering of PMU events between host and guests when used
with KVM.

As the underlying hardware cannot distinguish between guest and host
context, the performance counters must be stopped and started upon
entry/exit to the guest. This is performed at EL2 in a way that
minimizes overhead and improves accuracy of recording events that only
occur in the requested context.

This has been tested with VHE and non-VHE kernels with a KVM guest.

Changes from v6:

 - Move events_host/events_guest out of kvm_cpu_context

Changes from v5:

 - Tweak logic in use of kvm_set_pmu_events

Changes from v4:

 - Prevent unnecessary write_sysreg calls by improving
   __pmu_switch_to_xxx logic.

Changes from v3:

 - Remove confusing _only suffix from bitfields in kvm_cpu_context
 - Remove unnecessary condition when clearing event bits in disable
 - Simplify API of KVM accessors
 - Prevent unnecessary setting of pmcnten when guest/host events are
   the same.

Changes from v2:

 - Ensured that exclude_kernel works for guest
 - Removed unnecessary exclusion of EL2 with exclude_host on !VHE
 - Renamed kvm_clr_set_host_pmu_events to reflect args order
 - Added additional information to isb patch

Changes from v1:

 - Removed unnecessary exclusion of EL1 with exclude_guest on VHE
 - Removed unnecessary isb from existing perf_event.c driver
 - Folded perf_event.c patches together
 - Added additional information to last patch commit message

Andrew Murray (5):
  arm64: arm_pmu: remove unnecessary isb instruction
  arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data
  arm64: KVM: add accessors to track guest/host only counters
  arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes
  arm64: KVM: Enable support for :G/:H perf event modifiers

 arch/arm64/include/asm/kvm_host.h | 36 +++++++++++++++++++++++++--
 arch/arm64/kernel/asm-offsets.c   |  3 ++-
 arch/arm64/kernel/perf_event.c    | 52 +++++++++++++++++++++++++++++++++------
 arch/arm64/kvm/hyp/switch.c       | 52 +++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/arm.c                |  4 ++-
 5 files changed, 135 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH v7 0/5] arm64: Support perf event modifiers :G and :H

[Andrew Murray]

This patchset provides support for perf event modifiers :G and :H which
allows for filtering of PMU events between host and guests when used
with KVM.

As the underlying hardware cannot distinguish between guest and host
context, the performance counters must be stopped and started upon
entry/exit to the guest. This is performed at EL2 in a way that
minimizes overhead and improves accuracy of recording events that only
occur in the requested context.

This has been tested with VHE and non-VHE kernels with a KVM guest.

Changes from v6:

 - Move events_host/events_guest out of kvm_cpu_context

Changes from v5:

 - Tweak logic in use of kvm_set_pmu_events

Changes from v4:

 - Prevent unnecessary write_sysreg calls by improving
   __pmu_switch_to_xxx logic.

Changes from v3:

 - Remove confusing _only suffix from bitfields in kvm_cpu_context
 - Remove unnecessary condition when clearing event bits in disable
 - Simplify API of KVM accessors
 - Prevent unnecessary setting of pmcnten when guest/host events are
   the same.

Changes from v2:

 - Ensured that exclude_kernel works for guest
 - Removed unnecessary exclusion of EL2 with exclude_host on !VHE
 - Renamed kvm_clr_set_host_pmu_events to reflect args order
 - Added additional information to isb patch

Changes from v1:

 - Removed unnecessary exclusion of EL1 with exclude_guest on VHE
 - Removed unnecessary isb from existing perf_event.c driver
 - Folded perf_event.c patches together
 - Added additional information to last patch commit message

Andrew Murray (5):
  arm64: arm_pmu: remove unnecessary isb instruction
  arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data
  arm64: KVM: add accessors to track guest/host only counters
  arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes
  arm64: KVM: Enable support for :G/:H perf event modifiers

 arch/arm64/include/asm/kvm_host.h | 36 +++++++++++++++++++++++++--
 arch/arm64/kernel/asm-offsets.c   |  3 ++-
 arch/arm64/kernel/perf_event.c    | 52 +++++++++++++++++++++++++++++++++------
 arch/arm64/kvm/hyp/switch.c       | 52 +++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/arm.c                |  4 ++-
 5 files changed, 135 insertions(+), 12 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 1/5] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

The armv8pmu_enable_event_counter function issues an isb instruction
after enabling a pair of counters - this doesn't provide any value
and is inconsistent with the armv8pmu_disable_event_counter.

In any case armv8pmu_enable_event_counter is always called with the
PMU stopped. Starting the PMU with armv8pmu_start results in an isb
instruction being issued prior to writing to PMCR_EL0.

Let's remove the unnecessary isb instruction.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/perf_event.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 8e38d52..de564ae 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
 		armv8pmu_enable_counter(idx - 1);
-	isb();
 }
 
 static inline int armv8pmu_disable_counter(int idx)
-- 
2.7.4

[PATCH v7 1/5] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

The armv8pmu_enable_event_counter function issues an isb instruction
after enabling a pair of counters - this doesn't provide any value
and is inconsistent with the armv8pmu_disable_event_counter.

In any case armv8pmu_enable_event_counter is always called with the
PMU stopped. Starting the PMU with armv8pmu_start results in an isb
instruction being issued prior to writing to PMCR_EL0.

Let's remove the unnecessary isb instruction.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/perf_event.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 8e38d52..de564ae 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
 		armv8pmu_enable_counter(idx - 1);
-	isb();
 }
 
 static inline int armv8pmu_disable_counter(int idx)
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
type, at present this is typedef'd to kvm_cpu_context and used to store
host cpu context. The kvm_cpu_context structure is also used elsewhere to
hold vcpu context. In order to use the percpu to hold additional future
host information we encapsulate kvm_cpu_context in a new structure.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 8 ++++++--
 arch/arm64/kernel/asm-offsets.c   | 3 ++-
 virt/kvm/arm/arm.c                | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1550192..bcf9d60 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -205,7 +205,11 @@ struct kvm_cpu_context {
 	struct kvm_vcpu *__hyp_running_vcpu;
 };
 
-typedef struct kvm_cpu_context kvm_cpu_context_t;
+struct kvm_host_data {
+	struct kvm_cpu_context __kvm_cpu_state;
+};
+
+typedef struct kvm_host_data kvm_cpu_context_t;
 
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
@@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
 	struct kvm_guest_debug_arch external_debug_state;
 
 	/* Pointer to host CPU context */
-	kvm_cpu_context_t *host_cpu_context;
+	struct kvm_cpu_context *host_cpu_context;
 
 	struct thread_info *host_thread_info;	/* hyp VA */
 	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 323aeb5..da34022 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -142,7 +142,8 @@ int main(void)
   DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
   DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
-  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
+  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
+				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
 #endif
 #ifdef CONFIG_CPU_PM
   DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 150c8a6..4f2e534 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	int *last_ran;
+	kvm_cpu_context_t *cpu_ctxt;
 
 	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
+	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
 
 	/*
 	 * We might get preempted before the vCPU actually runs, but
@@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	}
 
 	vcpu->cpu = cpu;
-	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
+	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
 
 	kvm_arm_set_running_vcpu(vcpu);
 	kvm_vgic_load(vcpu);
-- 
2.7.4

[PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
type, at present this is typedef'd to kvm_cpu_context and used to store
host cpu context. The kvm_cpu_context structure is also used elsewhere to
hold vcpu context. In order to use the percpu to hold additional future
host information we encapsulate kvm_cpu_context in a new structure.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 8 ++++++--
 arch/arm64/kernel/asm-offsets.c   | 3 ++-
 virt/kvm/arm/arm.c                | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1550192..bcf9d60 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -205,7 +205,11 @@ struct kvm_cpu_context {
 	struct kvm_vcpu *__hyp_running_vcpu;
 };
 
-typedef struct kvm_cpu_context kvm_cpu_context_t;
+struct kvm_host_data {
+	struct kvm_cpu_context __kvm_cpu_state;
+};
+
+typedef struct kvm_host_data kvm_cpu_context_t;
 
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
@@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
 	struct kvm_guest_debug_arch external_debug_state;
 
 	/* Pointer to host CPU context */
-	kvm_cpu_context_t *host_cpu_context;
+	struct kvm_cpu_context *host_cpu_context;
 
 	struct thread_info *host_thread_info;	/* hyp VA */
 	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 323aeb5..da34022 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -142,7 +142,8 @@ int main(void)
   DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
   DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
-  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
+  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
+				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
 #endif
 #ifdef CONFIG_CPU_PM
   DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 150c8a6..4f2e534 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	int *last_ran;
+	kvm_cpu_context_t *cpu_ctxt;
 
 	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
+	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
 
 	/*
 	 * We might get preempted before the vCPU actually runs, but
@@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	}
 
 	vcpu->cpu = cpu;
-	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
+	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
 
 	kvm_arm_set_running_vcpu(vcpu);
 	kvm_vgic_load(vcpu);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 3/5] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

In order to effeciently enable/disable guest/host only perf counters
at guest entry/exit we add bitfields to kvm_cpu_context for guest and
host events as well as accessors for updating them.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index bcf9d60..8c0c694 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -205,8 +205,14 @@ struct kvm_cpu_context {
 	struct kvm_vcpu *__hyp_running_vcpu;
 };
 
+struct kvm_pmu_events {
+	u32 events_host;
+	u32 events_guest;
+};
+
 struct kvm_host_data {
 	struct kvm_cpu_context __kvm_cpu_state;
+	struct kvm_pmu_events pmu_events;
 };
 
 typedef struct kvm_host_data kvm_cpu_context_t;
@@ -471,11 +477,33 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
 
+#define KVM_PMU_EVENTS_HOST	1
+#define KVM_PMU_EVENTS_GUEST	2
+
 #ifdef CONFIG_KVM /* Avoid conflicts with core headers if CONFIG_KVM=n */
 static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 {
 	return kvm_arch_vcpu_run_map_fp(vcpu);
 }
+static inline void kvm_set_pmu_events(u32 set, int flags)
+{
+	struct kvm_host_data *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	if (flags & KVM_PMU_EVENTS_HOST)
+		ctx->pmu_events.events_host |= set;
+	if (flags & KVM_PMU_EVENTS_GUEST)
+		ctx->pmu_events.events_guest |= set;
+}
+static inline void kvm_clr_pmu_events(u32 clr)
+{
+	struct kvm_host_data *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->pmu_events.events_host &= ~clr;
+	ctx->pmu_events.events_guest &= ~clr;
+}
+#else
+static inline void kvm_set_pmu_events(u32 set, int flags) {}
+static inline void kvm_clr_pmu_events(u32 clr) {}
 #endif
 
 static inline void kvm_arm_vhe_guest_enter(void)
-- 
2.7.4

[PATCH v7 3/5] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

In order to effeciently enable/disable guest/host only perf counters
at guest entry/exit we add bitfields to kvm_cpu_context for guest and
host events as well as accessors for updating them.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index bcf9d60..8c0c694 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -205,8 +205,14 @@ struct kvm_cpu_context {
 	struct kvm_vcpu *__hyp_running_vcpu;
 };
 
+struct kvm_pmu_events {
+	u32 events_host;
+	u32 events_guest;
+};
+
 struct kvm_host_data {
 	struct kvm_cpu_context __kvm_cpu_state;
+	struct kvm_pmu_events pmu_events;
 };
 
 typedef struct kvm_host_data kvm_cpu_context_t;
@@ -471,11 +477,33 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
 
+#define KVM_PMU_EVENTS_HOST	1
+#define KVM_PMU_EVENTS_GUEST	2
+
 #ifdef CONFIG_KVM /* Avoid conflicts with core headers if CONFIG_KVM=n */
 static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 {
 	return kvm_arch_vcpu_run_map_fp(vcpu);
 }
+static inline void kvm_set_pmu_events(u32 set, int flags)
+{
+	struct kvm_host_data *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	if (flags & KVM_PMU_EVENTS_HOST)
+		ctx->pmu_events.events_host |= set;
+	if (flags & KVM_PMU_EVENTS_GUEST)
+		ctx->pmu_events.events_guest |= set;
+}
+static inline void kvm_clr_pmu_events(u32 clr)
+{
+	struct kvm_host_data *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->pmu_events.events_host &= ~clr;
+	ctx->pmu_events.events_guest &= ~clr;
+}
+#else
+static inline void kvm_set_pmu_events(u32 set, int flags) {}
+static inline void kvm_clr_pmu_events(u32 clr) {}
 #endif
 
 static inline void kvm_arm_vhe_guest_enter(void)
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 4/5] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

Add support for the :G and :H attributes in perf by handling the
exclude_host/exclude_guest event attributes.

We notify KVM of counters that we wish to be enabled or disabled on
guest entry/exit and thus defer from starting or stopping :G events
as per the events exclude_host attribute.

With both VHE and non-VHE we switch the counters between host/guest
at EL2. We are able to eliminate counters counting host events on
the boundaries of guest entry/exit when using :G by filtering out
EL2 for exclude_host. However when using :H unless exclude_hv is set
on non-VHE then there is a small blackout window at the guest
entry/exit where host events are not captured.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 51 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index de564ae..4a3c73d 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -26,6 +26,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clocksource.h>
+#include <linux/kvm_host.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -647,11 +648,26 @@ static inline int armv8pmu_enable_counter(int idx)
 
 static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 {
+	struct perf_event_attr *attr = &event->attr;
 	int idx = event->hw.idx;
+	int flags = 0;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
-	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_enable_counter(idx - 1);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (!attr->exclude_host)
+		flags |= KVM_PMU_EVENTS_HOST;
+	if (!attr->exclude_guest)
+		flags |= KVM_PMU_EVENTS_GUEST;
+
+	kvm_set_pmu_events(counter_bits, flags);
+
+	if (!attr->exclude_host) {
+		armv8pmu_enable_counter(idx);
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_enable_counter(idx - 1);
+	}
 }
 
 static inline int armv8pmu_disable_counter(int idx)
@@ -664,11 +680,20 @@ static inline int armv8pmu_disable_counter(int idx)
 static inline void armv8pmu_disable_event_counter(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	struct perf_event_attr *attr = &event->attr;
 	int idx = hwc->idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_disable_counter(idx - 1);
-	armv8pmu_disable_counter(idx);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	kvm_clr_pmu_events(counter_bits);
+
+	if (!attr->exclude_host) {
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_disable_counter(idx - 1);
+		armv8pmu_disable_counter(idx);
+	}
 }
 
 static inline int armv8pmu_enable_intens(int idx)
@@ -943,16 +968,25 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 	 * Therefore we ignore exclude_hv in this configuration, since
 	 * there's no hypervisor to sample anyway. This is consistent
 	 * with other architectures (x86 and Power).
+	 *
+	 * To eliminate counting host events on the boundaries of
+	 * guest entry/exit we ensure EL2 is not included in hyp mode
+	 * with !exclude_host.
 	 */
 	if (is_kernel_in_hyp_mode()) {
-		if (!attr->exclude_kernel)
+		if (!attr->exclude_kernel && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	} else {
-		if (attr->exclude_kernel)
-			config_base |= ARMV8_PMU_EXCLUDE_EL1;
 		if (!attr->exclude_hv)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	}
+
+	/*
+	 * Filter out !VHE kernels and guest kernels
+	 */
+	if (attr->exclude_kernel)
+		config_base |= ARMV8_PMU_EXCLUDE_EL1;
+
 	if (attr->exclude_user)
 		config_base |= ARMV8_PMU_EXCLUDE_EL0;
 
@@ -976,6 +1010,9 @@ static void armv8pmu_reset(void *info)
 		armv8pmu_disable_intens(idx);
 	}
 
+	/* Clear the counters we flip at guest entry/exit */
+	kvm_clr_pmu_events(U32_MAX);
+
 	/*
 	 * Initialize & Reset PMNC. Request overflow interrupt for
 	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
-- 
2.7.4

[PATCH v7 4/5] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

Add support for the :G and :H attributes in perf by handling the
exclude_host/exclude_guest event attributes.

We notify KVM of counters that we wish to be enabled or disabled on
guest entry/exit and thus defer from starting or stopping :G events
as per the events exclude_host attribute.

With both VHE and non-VHE we switch the counters between host/guest
at EL2. We are able to eliminate counters counting host events on
the boundaries of guest entry/exit when using :G by filtering out
EL2 for exclude_host. However when using :H unless exclude_hv is set
on non-VHE then there is a small blackout window at the guest
entry/exit where host events are not captured.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 51 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index de564ae..4a3c73d 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -26,6 +26,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clocksource.h>
+#include <linux/kvm_host.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -647,11 +648,26 @@ static inline int armv8pmu_enable_counter(int idx)
 
 static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 {
+	struct perf_event_attr *attr = &event->attr;
 	int idx = event->hw.idx;
+	int flags = 0;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
-	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_enable_counter(idx - 1);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (!attr->exclude_host)
+		flags |= KVM_PMU_EVENTS_HOST;
+	if (!attr->exclude_guest)
+		flags |= KVM_PMU_EVENTS_GUEST;
+
+	kvm_set_pmu_events(counter_bits, flags);
+
+	if (!attr->exclude_host) {
+		armv8pmu_enable_counter(idx);
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_enable_counter(idx - 1);
+	}
 }
 
 static inline int armv8pmu_disable_counter(int idx)
@@ -664,11 +680,20 @@ static inline int armv8pmu_disable_counter(int idx)
 static inline void armv8pmu_disable_event_counter(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	struct perf_event_attr *attr = &event->attr;
 	int idx = hwc->idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_disable_counter(idx - 1);
-	armv8pmu_disable_counter(idx);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	kvm_clr_pmu_events(counter_bits);
+
+	if (!attr->exclude_host) {
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_disable_counter(idx - 1);
+		armv8pmu_disable_counter(idx);
+	}
 }
 
 static inline int armv8pmu_enable_intens(int idx)
@@ -943,16 +968,25 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 	 * Therefore we ignore exclude_hv in this configuration, since
 	 * there's no hypervisor to sample anyway. This is consistent
 	 * with other architectures (x86 and Power).
+	 *
+	 * To eliminate counting host events on the boundaries of
+	 * guest entry/exit we ensure EL2 is not included in hyp mode
+	 * with !exclude_host.
 	 */
 	if (is_kernel_in_hyp_mode()) {
-		if (!attr->exclude_kernel)
+		if (!attr->exclude_kernel && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	} else {
-		if (attr->exclude_kernel)
-			config_base |= ARMV8_PMU_EXCLUDE_EL1;
 		if (!attr->exclude_hv)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	}
+
+	/*
+	 * Filter out !VHE kernels and guest kernels
+	 */
+	if (attr->exclude_kernel)
+		config_base |= ARMV8_PMU_EXCLUDE_EL1;
+
 	if (attr->exclude_user)
 		config_base |= ARMV8_PMU_EXCLUDE_EL0;
 
@@ -976,6 +1010,9 @@ static void armv8pmu_reset(void *info)
 		armv8pmu_disable_intens(idx);
 	}
 
+	/* Clear the counters we flip at guest entry/exit */
+	kvm_clr_pmu_events(U32_MAX);
+
 	/*
 	 * Initialize & Reset PMNC. Request overflow interrupt for
 	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 5/5] arm64: KVM: Enable support for :G/:H perf event modifiers

[Andrew Murray]

Enable/disable event counters as appropriate when entering and exiting
the guest to enable support for guest or host only event counting.

For both VHE and non-VHE we switch the counters between host/guest at
EL2. EL2 is filtered out by the PMU when we are using the :G modifier.

The PMU may be on when we change which counters are enabled however
we avoid adding an isb as we instead rely on existing context
synchronisation events: the isb in kvm_arm_vhe_guest_exit for VHE and
the eret from the hvc in kvm_call_hyp.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef5..bad713f 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -373,6 +373,46 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 	return true;
 }
 
+static bool __hyp_text __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
+{
+	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
+	u32 clr, set;
+
+	host = container_of(host_ctxt, struct kvm_host_data, __kvm_cpu_state);
+	pmu = &host->pmu_events;
+
+	clr = pmu->events_host & ~pmu->events_guest;
+	set = pmu->events_guest & ~pmu->events_host;
+
+	if (clr)
+		write_sysreg(clr, pmcntenclr_el0);
+
+	if (set)
+		write_sysreg(set, pmcntenset_el0);
+
+	return (clr || set);
+}
+
+static void __hyp_text __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
+{
+	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
+	u32 clr, set;
+
+	host = container_of(host_ctxt, struct kvm_host_data, __kvm_cpu_state);
+	pmu = &host->pmu_events;
+
+	clr = pmu->events_guest & ~pmu->events_host;
+	set = pmu->events_host & ~pmu->events_guest;
+
+	if (clr)
+		write_sysreg(clr, pmcntenclr_el0);
+
+	if (set)
+		write_sysreg(set, pmcntenset_el0);
+}
+
 /*
  * Return true when we were able to fixup the guest exit and should return to
  * the guest, false when we should restore the host state and return to the
@@ -488,12 +528,15 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	host_ctxt = vcpu->arch.host_cpu_context;
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	sysreg_save_host_state_vhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -524,6 +567,9 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
@@ -532,6 +578,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	vcpu = kern_hyp_va(vcpu);
@@ -540,6 +587,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	__sysreg_save_state_nvhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -586,6 +635,9 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 */
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
-- 
2.7.4

[PATCH v7 5/5] arm64: KVM: Enable support for :G/:H perf event modifiers

[Andrew Murray]

Enable/disable event counters as appropriate when entering and exiting
the guest to enable support for guest or host only event counting.

For both VHE and non-VHE we switch the counters between host/guest at
EL2. EL2 is filtered out by the PMU when we are using the :G modifier.

The PMU may be on when we change which counters are enabled however
we avoid adding an isb as we instead rely on existing context
synchronisation events: the isb in kvm_arm_vhe_guest_exit for VHE and
the eret from the hvc in kvm_call_hyp.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef5..bad713f 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -373,6 +373,46 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 	return true;
 }
 
+static bool __hyp_text __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
+{
+	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
+	u32 clr, set;
+
+	host = container_of(host_ctxt, struct kvm_host_data, __kvm_cpu_state);
+	pmu = &host->pmu_events;
+
+	clr = pmu->events_host & ~pmu->events_guest;
+	set = pmu->events_guest & ~pmu->events_host;
+
+	if (clr)
+		write_sysreg(clr, pmcntenclr_el0);
+
+	if (set)
+		write_sysreg(set, pmcntenset_el0);
+
+	return (clr || set);
+}
+
+static void __hyp_text __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
+{
+	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
+	u32 clr, set;
+
+	host = container_of(host_ctxt, struct kvm_host_data, __kvm_cpu_state);
+	pmu = &host->pmu_events;
+
+	clr = pmu->events_guest & ~pmu->events_host;
+	set = pmu->events_host & ~pmu->events_guest;
+
+	if (clr)
+		write_sysreg(clr, pmcntenclr_el0);
+
+	if (set)
+		write_sysreg(set, pmcntenset_el0);
+}
+
 /*
  * Return true when we were able to fixup the guest exit and should return to
  * the guest, false when we should restore the host state and return to the
@@ -488,12 +528,15 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	host_ctxt = vcpu->arch.host_cpu_context;
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	sysreg_save_host_state_vhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -524,6 +567,9 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
@@ -532,6 +578,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	vcpu = kern_hyp_va(vcpu);
@@ -540,6 +587,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	__sysreg_save_state_nvhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -586,6 +635,9 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 */
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Christoffer Dall]

On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> type, at present this is typedef'd to kvm_cpu_context and used to store
> host cpu context. The kvm_cpu_context structure is also used elsewhere to
> hold vcpu context. In order to use the percpu to hold additional future
> host information we encapsulate kvm_cpu_context in a new structure.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
>  arch/arm64/kernel/asm-offsets.c   | 3 ++-
>  virt/kvm/arm/arm.c                | 4 +++-
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..bcf9d60 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -205,7 +205,11 @@ struct kvm_cpu_context {
>  	struct kvm_vcpu *__hyp_running_vcpu;
>  };
>  
> -typedef struct kvm_cpu_context kvm_cpu_context_t;
> +struct kvm_host_data {
> +	struct kvm_cpu_context __kvm_cpu_state;
> +};
> +
> +typedef struct kvm_host_data kvm_cpu_context_t;

Now I'm confused based on the conversation on the last version.

I think it's bizarre to use the typedef to rename things in this way.

Can you please make this:

   struct kvm_cpu_context;
   typedef struct kvm_cpu_context kvm_cpu_context_t;

   struct kvm_host_data;
   typedef struct kvm_host_data kvm_host_data_t;

And change the code with the fallout from that.


>  
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
> @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
>  	struct kvm_guest_debug_arch external_debug_state;
>  
>  	/* Pointer to host CPU context */
> -	kvm_cpu_context_t *host_cpu_context;
> +	struct kvm_cpu_context *host_cpu_context;
>  
>  	struct thread_info *host_thread_info;	/* hyp VA */
>  	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5..da34022 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -142,7 +142,8 @@ int main(void)
>    DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
>    DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));

This should be HOST_DATA_VCPU, then.


Thanks,

    Christoffer

>  #endif
>  #ifdef CONFIG_CPU_PM
>    DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 150c8a6..4f2e534 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	int *last_ran;
> +	kvm_cpu_context_t *cpu_ctxt;
>  
>  	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
> +	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
>  
>  	/*
>  	 * We might get preempted before the vCPU actually runs, but
> @@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	}
>  
>  	vcpu->cpu = cpu;
> -	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
> +	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
>  
>  	kvm_arm_set_running_vcpu(vcpu);
>  	kvm_vgic_load(vcpu);
> -- 
> 2.7.4
> 

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Christoffer Dall]

On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> type, at present this is typedef'd to kvm_cpu_context and used to store
> host cpu context. The kvm_cpu_context structure is also used elsewhere to
> hold vcpu context. In order to use the percpu to hold additional future
> host information we encapsulate kvm_cpu_context in a new structure.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
>  arch/arm64/kernel/asm-offsets.c   | 3 ++-
>  virt/kvm/arm/arm.c                | 4 +++-
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..bcf9d60 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -205,7 +205,11 @@ struct kvm_cpu_context {
>  	struct kvm_vcpu *__hyp_running_vcpu;
>  };
>  
> -typedef struct kvm_cpu_context kvm_cpu_context_t;
> +struct kvm_host_data {
> +	struct kvm_cpu_context __kvm_cpu_state;
> +};
> +
> +typedef struct kvm_host_data kvm_cpu_context_t;

Now I'm confused based on the conversation on the last version.

I think it's bizarre to use the typedef to rename things in this way.

Can you please make this:

   struct kvm_cpu_context;
   typedef struct kvm_cpu_context kvm_cpu_context_t;

   struct kvm_host_data;
   typedef struct kvm_host_data kvm_host_data_t;

And change the code with the fallout from that.


>  
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
> @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
>  	struct kvm_guest_debug_arch external_debug_state;
>  
>  	/* Pointer to host CPU context */
> -	kvm_cpu_context_t *host_cpu_context;
> +	struct kvm_cpu_context *host_cpu_context;
>  
>  	struct thread_info *host_thread_info;	/* hyp VA */
>  	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5..da34022 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -142,7 +142,8 @@ int main(void)
>    DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
>    DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));

This should be HOST_DATA_VCPU, then.


Thanks,

    Christoffer

>  #endif
>  #ifdef CONFIG_CPU_PM
>    DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 150c8a6..4f2e534 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	int *last_ran;
> +	kvm_cpu_context_t *cpu_ctxt;
>  
>  	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
> +	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
>  
>  	/*
>  	 * We might get preempted before the vCPU actually runs, but
> @@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	}
>  
>  	vcpu->cpu = cpu;
> -	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
> +	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
>  
>  	kvm_arm_set_running_vcpu(vcpu);
>  	kvm_vgic_load(vcpu);
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Suzuki K Poulose]

On 11/12/2018 12:13, Andrew Murray wrote:
> The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> type, at present this is typedef'd to kvm_cpu_context and used to store
> host cpu context. The kvm_cpu_context structure is also used elsewhere to
> hold vcpu context. In order to use the percpu to hold additional future
> host information we encapsulate kvm_cpu_context in a new structure.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/include/asm/kvm_host.h | 8 ++++++--
>   arch/arm64/kernel/asm-offsets.c   | 3 ++-
>   virt/kvm/arm/arm.c                | 4 +++-
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..bcf9d60 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -205,7 +205,11 @@ struct kvm_cpu_context {
>   	struct kvm_vcpu *__hyp_running_vcpu;
>   };
>   
> -typedef struct kvm_cpu_context kvm_cpu_context_t;
> +struct kvm_host_data {
> +	struct kvm_cpu_context __kvm_cpu_state;
> +};
> +
> +typedef struct kvm_host_data kvm_cpu_context_t;
>   
>   struct kvm_vcpu_arch {
>   	struct kvm_cpu_context ctxt;
> @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
>   	struct kvm_guest_debug_arch external_debug_state;
>   
>   	/* Pointer to host CPU context */
> -	kvm_cpu_context_t *host_cpu_context;
> +	struct kvm_cpu_context *host_cpu_context;
>   
>   	struct thread_info *host_thread_info;	/* hyp VA */
>   	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5..da34022 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -142,7 +142,8 @@ int main(void)
>     DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
>     DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
>     DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));

nit: You could fold the member field in the offsetof(). i.e,
      DEFINE(HOST_CONTEXT_VCPU, offsetof(struct kvm_host_data, 
__kvm_cpu_state.__hyp_running_vcpu)

like the VCPU_HOST_CONTEXT above ?

Cheers
Suzuki

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Suzuki K Poulose]

On 11/12/2018 12:13, Andrew Murray wrote:
> The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> type, at present this is typedef'd to kvm_cpu_context and used to store
> host cpu context. The kvm_cpu_context structure is also used elsewhere to
> hold vcpu context. In order to use the percpu to hold additional future
> host information we encapsulate kvm_cpu_context in a new structure.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/include/asm/kvm_host.h | 8 ++++++--
>   arch/arm64/kernel/asm-offsets.c   | 3 ++-
>   virt/kvm/arm/arm.c                | 4 +++-
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..bcf9d60 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -205,7 +205,11 @@ struct kvm_cpu_context {
>   	struct kvm_vcpu *__hyp_running_vcpu;
>   };
>   
> -typedef struct kvm_cpu_context kvm_cpu_context_t;
> +struct kvm_host_data {
> +	struct kvm_cpu_context __kvm_cpu_state;
> +};
> +
> +typedef struct kvm_host_data kvm_cpu_context_t;
>   
>   struct kvm_vcpu_arch {
>   	struct kvm_cpu_context ctxt;
> @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
>   	struct kvm_guest_debug_arch external_debug_state;
>   
>   	/* Pointer to host CPU context */
> -	kvm_cpu_context_t *host_cpu_context;
> +	struct kvm_cpu_context *host_cpu_context;
>   
>   	struct thread_info *host_thread_info;	/* hyp VA */
>   	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5..da34022 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -142,7 +142,8 @@ int main(void)
>     DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
>     DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
>     DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));

nit: You could fold the member field in the offsetof(). i.e,
      DEFINE(HOST_CONTEXT_VCPU, offsetof(struct kvm_host_data, 
__kvm_cpu_state.__hyp_running_vcpu)

like the VCPU_HOST_CONTEXT above ?

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > type, at present this is typedef'd to kvm_cpu_context and used to store
> > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > hold vcpu context. In order to use the percpu to hold additional future
> > host information we encapsulate kvm_cpu_context in a new structure.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> >  virt/kvm/arm/arm.c                | 4 +++-
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..bcf9d60 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> >  	struct kvm_vcpu *__hyp_running_vcpu;
> >  };
> >  
> > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > +struct kvm_host_data {
> > +	struct kvm_cpu_context __kvm_cpu_state;
> > +};
> > +
> > +typedef struct kvm_host_data kvm_cpu_context_t;
> 
> Now I'm confused based on the conversation on the last version.
> 
> I think it's bizarre to use the typedef to rename things in this way.
> 
> Can you please make this:
> 
>    struct kvm_cpu_context;
>    typedef struct kvm_cpu_context kvm_cpu_context_t;
> 
>    struct kvm_host_data;
>    typedef struct kvm_host_data kvm_host_data_t;
> 
> And change the code with the fallout from that.

I guess I was trying to avoid similar naming issues on arm32. If we
make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:

typedef struct kvm_cpu_context kvm_cpu_context_t;

becomes:

typedef struct kvm_cpu_context kvm_host_data_t;

though I guess this may be acceptable?


On a similar note the hunk in my last email on the previous thread
would cause a breakage on arm32:

@@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
        }
 
        vcpu->cpu = cpu;
-       vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
+       vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
 
        kvm_arm_set_running_vcpu(vcpu);
        kvm_vgic_load(vcpu);

.. instead of this hunk I'll update the uses of host_cpu_context in the
arm64 code.

Thanks,

Andrew Murray


> 
> 
> >  
> >  struct kvm_vcpu_arch {
> >  	struct kvm_cpu_context ctxt;
> > @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
> >  	struct kvm_guest_debug_arch external_debug_state;
> >  
> >  	/* Pointer to host CPU context */
> > -	kvm_cpu_context_t *host_cpu_context;
> > +	struct kvm_cpu_context *host_cpu_context;
> >  
> >  	struct thread_info *host_thread_info;	/* hyp VA */
> >  	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> > index 323aeb5..da34022 100644
> > --- a/arch/arm64/kernel/asm-offsets.c
> > +++ b/arch/arm64/kernel/asm-offsets.c
> > @@ -142,7 +142,8 @@ int main(void)
> >    DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
> >    DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
> >    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> > -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> > +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> > +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
> 
> This should be HOST_DATA_VCPU, then.
> 
> 
> Thanks,
> 
>     Christoffer
> 
> >  #endif
> >  #ifdef CONFIG_CPU_PM
> >    DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 150c8a6..4f2e534 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  {
> >  	int *last_ran;
> > +	kvm_cpu_context_t *cpu_ctxt;
> >  
> >  	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
> > +	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
> >  
> >  	/*
> >  	 * We might get preempted before the vCPU actually runs, but
> > @@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  	}
> >  
> >  	vcpu->cpu = cpu;
> > -	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
> > +	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
> >  
> >  	kvm_arm_set_running_vcpu(vcpu);
> >  	kvm_vgic_load(vcpu);
> > -- 
> > 2.7.4
> > 

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > type, at present this is typedef'd to kvm_cpu_context and used to store
> > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > hold vcpu context. In order to use the percpu to hold additional future
> > host information we encapsulate kvm_cpu_context in a new structure.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> >  virt/kvm/arm/arm.c                | 4 +++-
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..bcf9d60 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> >  	struct kvm_vcpu *__hyp_running_vcpu;
> >  };
> >  
> > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > +struct kvm_host_data {
> > +	struct kvm_cpu_context __kvm_cpu_state;
> > +};
> > +
> > +typedef struct kvm_host_data kvm_cpu_context_t;
> 
> Now I'm confused based on the conversation on the last version.
> 
> I think it's bizarre to use the typedef to rename things in this way.
> 
> Can you please make this:
> 
>    struct kvm_cpu_context;
>    typedef struct kvm_cpu_context kvm_cpu_context_t;
> 
>    struct kvm_host_data;
>    typedef struct kvm_host_data kvm_host_data_t;
> 
> And change the code with the fallout from that.

I guess I was trying to avoid similar naming issues on arm32. If we
make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:

typedef struct kvm_cpu_context kvm_cpu_context_t;

becomes:

typedef struct kvm_cpu_context kvm_host_data_t;

though I guess this may be acceptable?


On a similar note the hunk in my last email on the previous thread
would cause a breakage on arm32:

@@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
        }
 
        vcpu->cpu = cpu;
-       vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
+       vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
 
        kvm_arm_set_running_vcpu(vcpu);
        kvm_vgic_load(vcpu);

.. instead of this hunk I'll update the uses of host_cpu_context in the
arm64 code.

Thanks,

Andrew Murray


> 
> 
> >  
> >  struct kvm_vcpu_arch {
> >  	struct kvm_cpu_context ctxt;
> > @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
> >  	struct kvm_guest_debug_arch external_debug_state;
> >  
> >  	/* Pointer to host CPU context */
> > -	kvm_cpu_context_t *host_cpu_context;
> > +	struct kvm_cpu_context *host_cpu_context;
> >  
> >  	struct thread_info *host_thread_info;	/* hyp VA */
> >  	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> > index 323aeb5..da34022 100644
> > --- a/arch/arm64/kernel/asm-offsets.c
> > +++ b/arch/arm64/kernel/asm-offsets.c
> > @@ -142,7 +142,8 @@ int main(void)
> >    DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
> >    DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
> >    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> > -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> > +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> > +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
> 
> This should be HOST_DATA_VCPU, then.
> 
> 
> Thanks,
> 
>     Christoffer
> 
> >  #endif
> >  #ifdef CONFIG_CPU_PM
> >    DEFINE(CPU_SUSPEND_SZ,	sizeof(struct cpu_suspend_ctx));
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 150c8a6..4f2e534 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -361,8 +361,10 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  {
> >  	int *last_ran;
> > +	kvm_cpu_context_t *cpu_ctxt;
> >  
> >  	last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
> > +	cpu_ctxt = this_cpu_ptr(&kvm_host_cpu_state);
> >  
> >  	/*
> >  	 * We might get preempted before the vCPU actually runs, but
> > @@ -374,7 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  	}
> >  
> >  	vcpu->cpu = cpu;
> > -	vcpu->arch.host_cpu_context = this_cpu_ptr(&kvm_host_cpu_state);
> > +	vcpu->arch.host_cpu_context = &cpu_ctxt->__kvm_cpu_state;
> >  
> >  	kvm_arm_set_running_vcpu(vcpu);
> >  	kvm_vgic_load(vcpu);
> > -- 
> > 2.7.4
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 12:40:51PM +0000, Suzuki K Poulose wrote:
> 
> 
> On 11/12/2018 12:13, Andrew Murray wrote:
> > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > type, at present this is typedef'd to kvm_cpu_context and used to store
> > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > hold vcpu context. In order to use the percpu to hold additional future
> > host information we encapsulate kvm_cpu_context in a new structure.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >   arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> >   arch/arm64/kernel/asm-offsets.c   | 3 ++-
> >   virt/kvm/arm/arm.c                | 4 +++-
> >   3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..bcf9d60 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> >   	struct kvm_vcpu *__hyp_running_vcpu;
> >   };
> > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > +struct kvm_host_data {
> > +	struct kvm_cpu_context __kvm_cpu_state;
> > +};
> > +
> > +typedef struct kvm_host_data kvm_cpu_context_t;
> >   struct kvm_vcpu_arch {
> >   	struct kvm_cpu_context ctxt;
> > @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
> >   	struct kvm_guest_debug_arch external_debug_state;
> >   	/* Pointer to host CPU context */
> > -	kvm_cpu_context_t *host_cpu_context;
> > +	struct kvm_cpu_context *host_cpu_context;
> >   	struct thread_info *host_thread_info;	/* hyp VA */
> >   	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> > index 323aeb5..da34022 100644
> > --- a/arch/arm64/kernel/asm-offsets.c
> > +++ b/arch/arm64/kernel/asm-offsets.c
> > @@ -142,7 +142,8 @@ int main(void)
> >     DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
> >     DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
> >     DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> > -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> > +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> > +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
> 
> nit: You could fold the member field in the offsetof(). i.e,
>      DEFINE(HOST_CONTEXT_VCPU, offsetof(struct kvm_host_data,
> __kvm_cpu_state.__hyp_running_vcpu)
> 
> like the VCPU_HOST_CONTEXT above ?

Ah yes thanks for this.

Andrew Murray

> 
> Cheers
> Suzuki

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 12:40:51PM +0000, Suzuki K Poulose wrote:
> 
> 
> On 11/12/2018 12:13, Andrew Murray wrote:
> > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > type, at present this is typedef'd to kvm_cpu_context and used to store
> > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > hold vcpu context. In order to use the percpu to hold additional future
> > host information we encapsulate kvm_cpu_context in a new structure.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >   arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> >   arch/arm64/kernel/asm-offsets.c   | 3 ++-
> >   virt/kvm/arm/arm.c                | 4 +++-
> >   3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..bcf9d60 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> >   	struct kvm_vcpu *__hyp_running_vcpu;
> >   };
> > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > +struct kvm_host_data {
> > +	struct kvm_cpu_context __kvm_cpu_state;
> > +};
> > +
> > +typedef struct kvm_host_data kvm_cpu_context_t;
> >   struct kvm_vcpu_arch {
> >   	struct kvm_cpu_context ctxt;
> > @@ -241,7 +245,7 @@ struct kvm_vcpu_arch {
> >   	struct kvm_guest_debug_arch external_debug_state;
> >   	/* Pointer to host CPU context */
> > -	kvm_cpu_context_t *host_cpu_context;
> > +	struct kvm_cpu_context *host_cpu_context;
> >   	struct thread_info *host_thread_info;	/* hyp VA */
> >   	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> > index 323aeb5..da34022 100644
> > --- a/arch/arm64/kernel/asm-offsets.c
> > +++ b/arch/arm64/kernel/asm-offsets.c
> > @@ -142,7 +142,8 @@ int main(void)
> >     DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
> >     DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
> >     DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> > -  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
> > +  DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu)
> > +				+ offsetof(struct kvm_host_data, __kvm_cpu_state));
> 
> nit: You could fold the member field in the offsetof(). i.e,
>      DEFINE(HOST_CONTEXT_VCPU, offsetof(struct kvm_host_data,
> __kvm_cpu_state.__hyp_running_vcpu)
> 
> like the VCPU_HOST_CONTEXT above ?

Ah yes thanks for this.

Andrew Murray

> 
> Cheers
> Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Christoffer Dall]

On Tue, Dec 11, 2018 at 01:11:33PM +0000, Andrew Murray wrote:
> On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> > On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > > type, at present this is typedef'd to kvm_cpu_context and used to store
> > > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > > hold vcpu context. In order to use the percpu to hold additional future
> > > host information we encapsulate kvm_cpu_context in a new structure.
> > > 
> > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> > >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> > >  virt/kvm/arm/arm.c                | 4 +++-
> > >  3 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 1550192..bcf9d60 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> > >  	struct kvm_vcpu *__hyp_running_vcpu;
> > >  };
> > >  
> > > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > +struct kvm_host_data {
> > > +	struct kvm_cpu_context __kvm_cpu_state;
> > > +};
> > > +
> > > +typedef struct kvm_host_data kvm_cpu_context_t;
> > 
> > Now I'm confused based on the conversation on the last version.
> > 
> > I think it's bizarre to use the typedef to rename things in this way.
> > 
> > Can you please make this:
> > 
> >    struct kvm_cpu_context;
> >    typedef struct kvm_cpu_context kvm_cpu_context_t;
> > 
> >    struct kvm_host_data;
> >    typedef struct kvm_host_data kvm_host_data_t;
> > 
> > And change the code with the fallout from that.
> 
> I guess I was trying to avoid similar naming issues on arm32. If we
> make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
> then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:
> 
> typedef struct kvm_cpu_context kvm_cpu_context_t;
> 
> becomes:
> 
> typedef struct kvm_cpu_context kvm_host_data_t;
> 
> though I guess this may be acceptable?

I'd prefer it if you just introduce a struct kvm_host_data on the 32-bit
side only containing a struct kvm_cpu_context (if you wanted to support
perf on the 32-bit side you would also add additional fields to it,
similar to arm64).  That avoids the confusing typedef and you get the
symmmetry on both architectures allowing you to use shared code, which
is what we want at the end of the day.

So, on the 32-bit side, change this to:

struct kvm_host_data {
	struct kvm_cpu_context *host_ctxt;
};
typedef struct kvm_host_data kvm_host_data_t;


And use the same naming on both 32-bit and 64-bit arm, consistently.


Thanks,

    Christoffer

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Christoffer Dall]

On Tue, Dec 11, 2018 at 01:11:33PM +0000, Andrew Murray wrote:
> On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> > On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > > type, at present this is typedef'd to kvm_cpu_context and used to store
> > > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > > hold vcpu context. In order to use the percpu to hold additional future
> > > host information we encapsulate kvm_cpu_context in a new structure.
> > > 
> > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> > >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> > >  virt/kvm/arm/arm.c                | 4 +++-
> > >  3 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 1550192..bcf9d60 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> > >  	struct kvm_vcpu *__hyp_running_vcpu;
> > >  };
> > >  
> > > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > +struct kvm_host_data {
> > > +	struct kvm_cpu_context __kvm_cpu_state;
> > > +};
> > > +
> > > +typedef struct kvm_host_data kvm_cpu_context_t;
> > 
> > Now I'm confused based on the conversation on the last version.
> > 
> > I think it's bizarre to use the typedef to rename things in this way.
> > 
> > Can you please make this:
> > 
> >    struct kvm_cpu_context;
> >    typedef struct kvm_cpu_context kvm_cpu_context_t;
> > 
> >    struct kvm_host_data;
> >    typedef struct kvm_host_data kvm_host_data_t;
> > 
> > And change the code with the fallout from that.
> 
> I guess I was trying to avoid similar naming issues on arm32. If we
> make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
> then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:
> 
> typedef struct kvm_cpu_context kvm_cpu_context_t;
> 
> becomes:
> 
> typedef struct kvm_cpu_context kvm_host_data_t;
> 
> though I guess this may be acceptable?

I'd prefer it if you just introduce a struct kvm_host_data on the 32-bit
side only containing a struct kvm_cpu_context (if you wanted to support
perf on the 32-bit side you would also add additional fields to it,
similar to arm64).  That avoids the confusing typedef and you get the
symmmetry on both architectures allowing you to use shared code, which
is what we want at the end of the day.

So, on the 32-bit side, change this to:

struct kvm_host_data {
	struct kvm_cpu_context *host_ctxt;
};
typedef struct kvm_host_data kvm_host_data_t;


And use the same naming on both 32-bit and 64-bit arm, consistently.


Thanks,

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 02:40:07PM +0100, Christoffer Dall wrote:
> On Tue, Dec 11, 2018 at 01:11:33PM +0000, Andrew Murray wrote:
> > On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> > > On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > > > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > > > type, at present this is typedef'd to kvm_cpu_context and used to store
> > > > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > > > hold vcpu context. In order to use the percpu to hold additional future
> > > > host information we encapsulate kvm_cpu_context in a new structure.
> > > > 
> > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> > > >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> > > >  virt/kvm/arm/arm.c                | 4 +++-
> > > >  3 files changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index 1550192..bcf9d60 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> > > >  	struct kvm_vcpu *__hyp_running_vcpu;
> > > >  };
> > > >  
> > > > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > +struct kvm_host_data {
> > > > +	struct kvm_cpu_context __kvm_cpu_state;
> > > > +};
> > > > +
> > > > +typedef struct kvm_host_data kvm_cpu_context_t;
> > > 
> > > Now I'm confused based on the conversation on the last version.
> > > 
> > > I think it's bizarre to use the typedef to rename things in this way.
> > > 
> > > Can you please make this:
> > > 
> > >    struct kvm_cpu_context;
> > >    typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > 
> > >    struct kvm_host_data;
> > >    typedef struct kvm_host_data kvm_host_data_t;
> > > 
> > > And change the code with the fallout from that.
> > 
> > I guess I was trying to avoid similar naming issues on arm32. If we
> > make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
> > then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:
> > 
> > typedef struct kvm_cpu_context kvm_cpu_context_t;
> > 
> > becomes:
> > 
> > typedef struct kvm_cpu_context kvm_host_data_t;
> > 
> > though I guess this may be acceptable?
> 
> I'd prefer it if you just introduce a struct kvm_host_data on the 32-bit
> side only containing a struct kvm_cpu_context (if you wanted to support
> perf on the 32-bit side you would also add additional fields to it,
> similar to arm64).  That avoids the confusing typedef and you get the
> symmmetry on both architectures allowing you to use shared code, which
> is what we want at the end of the day.
> 
> So, on the 32-bit side, change this to:
> 
> struct kvm_host_data {
> 	struct kvm_cpu_context *host_ctxt;
> };
> typedef struct kvm_host_data kvm_host_data_t;
> 
> 
> And use the same naming on both 32-bit and 64-bit arm, consistently.

Sounds good to me.

Thanks,

Andrew Murray

> 
> 
> Thanks,
> 
>     Christoffer

Re: [PATCH v7 2/5] arm64: KVM: encapsulate kvm_cpu_context in kvm_host_data

[Andrew Murray]

On Tue, Dec 11, 2018 at 02:40:07PM +0100, Christoffer Dall wrote:
> On Tue, Dec 11, 2018 at 01:11:33PM +0000, Andrew Murray wrote:
> > On Tue, Dec 11, 2018 at 01:29:51PM +0100, Christoffer Dall wrote:
> > > On Tue, Dec 11, 2018 at 12:13:37PM +0000, Andrew Murray wrote:
> > > > The virt/arm core allocates a percpu structure as per the kvm_cpu_context_t
> > > > type, at present this is typedef'd to kvm_cpu_context and used to store
> > > > host cpu context. The kvm_cpu_context structure is also used elsewhere to
> > > > hold vcpu context. In order to use the percpu to hold additional future
> > > > host information we encapsulate kvm_cpu_context in a new structure.
> > > > 
> > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h | 8 ++++++--
> > > >  arch/arm64/kernel/asm-offsets.c   | 3 ++-
> > > >  virt/kvm/arm/arm.c                | 4 +++-
> > > >  3 files changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index 1550192..bcf9d60 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -205,7 +205,11 @@ struct kvm_cpu_context {
> > > >  	struct kvm_vcpu *__hyp_running_vcpu;
> > > >  };
> > > >  
> > > > -typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > > +struct kvm_host_data {
> > > > +	struct kvm_cpu_context __kvm_cpu_state;
> > > > +};
> > > > +
> > > > +typedef struct kvm_host_data kvm_cpu_context_t;
> > > 
> > > Now I'm confused based on the conversation on the last version.
> > > 
> > > I think it's bizarre to use the typedef to rename things in this way.
> > > 
> > > Can you please make this:
> > > 
> > >    struct kvm_cpu_context;
> > >    typedef struct kvm_cpu_context kvm_cpu_context_t;
> > > 
> > >    struct kvm_host_data;
> > >    typedef struct kvm_host_data kvm_host_data_t;
> > > 
> > > And change the code with the fallout from that.
> > 
> > I guess I was trying to avoid similar naming issues on arm32. If we
> > make the above changes (and thus the DEFINE_PER_CPU in virt/kvm/arm/arm.c)
> > then we need to change arm (arch/arm/include/asm/kvm_host.h) such that:
> > 
> > typedef struct kvm_cpu_context kvm_cpu_context_t;
> > 
> > becomes:
> > 
> > typedef struct kvm_cpu_context kvm_host_data_t;
> > 
> > though I guess this may be acceptable?
> 
> I'd prefer it if you just introduce a struct kvm_host_data on the 32-bit
> side only containing a struct kvm_cpu_context (if you wanted to support
> perf on the 32-bit side you would also add additional fields to it,
> similar to arm64).  That avoids the confusing typedef and you get the
> symmmetry on both architectures allowing you to use shared code, which
> is what we want at the end of the day.
> 
> So, on the 32-bit side, change this to:
> 
> struct kvm_host_data {
> 	struct kvm_cpu_context *host_ctxt;
> };
> typedef struct kvm_host_data kvm_host_data_t;
> 
> 
> And use the same naming on both 32-bit and 64-bit arm, consistently.

Sounds good to me.

Thanks,

Andrew Murray

> 
> 
> Thanks,
> 
>     Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel