| Message ID | 20220509095500.2408785-4-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Do not communicate host pmu event changes by accessing hyp data | expand |
On Mon, 09 May 2022 10:54:59 +0100, Fuad Tabba <tabba@google.com> wrote: > > Instead of the host accessing hyp data directly, pass the pmu > events of the current cpu to hyp via the vcpu. > > This adds 64 bits (in two fields) to the vcpu that need to be > synced before every vcpu run in nvhe and protected modes. > However, it isolates the hypervisor from the host, which allows > us to use pmu in protected mode in a subsequent patch. > > No functional change intended. > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 8 ++------ > arch/arm64/kvm/hyp/nvhe/switch.c | 20 ++++++-------------- > arch/arm64/kvm/pmu-emul.c | 3 +++ > arch/arm64/kvm/pmu.c | 12 ++++-------- > include/kvm/arm_pmu.h | 6 ++++++ > 5 files changed, 21 insertions(+), 28 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index dfd360404dd8..90476e713643 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -273,14 +273,8 @@ 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 host_ctxt; > - struct kvm_pmu_events pmu_events; > }; > > struct kvm_host_psci_config { > @@ -763,6 +757,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome); > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); > > DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data); > +DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events); > > static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt) > { > @@ -821,6 +816,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); > void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); > void kvm_clr_pmu_events(u32 clr); > > +struct kvm_pmu_events *kvm_get_pmu_events(void); > void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); > void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > #else > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > index 0716163313d6..c61120ec8d1a 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -153,13 +153,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu) > /* > * Disable host events, enable guest events > */ > -static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) > +static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu) > { > - struct kvm_host_data *host; > - struct kvm_pmu_events *pmu; > - > - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); > - pmu = &host->pmu_events; > + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; > > if (pmu->events_host) > write_sysreg(pmu->events_host, pmcntenclr_el0); > @@ -173,13 +169,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) > /* > * Disable guest events, enable host events > */ > -static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) > +static void __pmu_switch_to_host(struct kvm_vcpu *vcpu) > { > - struct kvm_host_data *host; > - struct kvm_pmu_events *pmu; > - > - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); > - pmu = &host->pmu_events; > + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; > > if (pmu->events_guest) > write_sysreg(pmu->events_guest, pmcntenclr_el0); > @@ -304,7 +296,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > host_ctxt->__hyp_running_vcpu = vcpu; > guest_ctxt = &vcpu->arch.ctxt; > > - pmu_switch_needed = __pmu_switch_to_guest(host_ctxt); > + pmu_switch_needed = __pmu_switch_to_guest(vcpu); > > __sysreg_save_state_nvhe(host_ctxt); > /* > @@ -366,7 +358,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > __debug_restore_host_buffers_nvhe(vcpu); > > if (pmu_switch_needed) > - __pmu_switch_to_host(host_ctxt); > + __pmu_switch_to_host(vcpu); > > /* Returning to host will clear PSR.I, remask PMR if needed */ > if (system_uses_irq_prio_masking()) > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 3dc990ac4f44..08d0551a4e43 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -406,6 +406,9 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu) > if (!kvm_vcpu_has_pmu(vcpu)) > return; > > + if (!has_vhe()) > + pmu->events = *kvm_get_pmu_events(); A bit of context: preempt_disable(); /* * The VMID allocator only tracks active VMIDs per * physical CPU, and therefore the VMID allocated may not be * preserved on VMID roll-over if the task was preempted, * making a thread's VMID inactive. So we need to call * kvm_arm_vmid_update() in non-premptible context. */ kvm_arm_vmid_update(&vcpu->arch.hw_mmu->vmid); kvm_pmu_flush_hwstate(vcpu); local_irq_disable(); You *still* are in a context where an interrupt can fire and mess things up behind your back. Not good. Also, this is now synchronised *twice* per run (once on flush, once on sync). Do we really need this? Thanks, M.
On Mon, 09 May 2022 10:54:59 +0100, Fuad Tabba <tabba@google.com> wrote: > > Instead of the host accessing hyp data directly, pass the pmu > events of the current cpu to hyp via the vcpu. > > This adds 64 bits (in two fields) to the vcpu that need to be > synced before every vcpu run in nvhe and protected modes. > However, it isolates the hypervisor from the host, which allows > us to use pmu in protected mode in a subsequent patch. > > No functional change intended. BTW, there *are* plenty of functional changes, even if there was no issue with the patch. The lifetime of the data structure changes, its sharing changes, and its memory footprint is different. I guess what you are trying to say is that there shouldn't be any visible side effect of this patch. But I don't think these are the same things. Thanks, M.
Hi Marc, On Mon, May 9, 2022 at 11:31 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 09 May 2022 10:54:59 +0100, > Fuad Tabba <tabba@google.com> wrote: > > > > Instead of the host accessing hyp data directly, pass the pmu > > events of the current cpu to hyp via the vcpu. > > > > This adds 64 bits (in two fields) to the vcpu that need to be > > synced before every vcpu run in nvhe and protected modes. > > However, it isolates the hypervisor from the host, which allows > > us to use pmu in protected mode in a subsequent patch. > > > > No functional change intended. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 8 ++------ > > arch/arm64/kvm/hyp/nvhe/switch.c | 20 ++++++-------------- > > arch/arm64/kvm/pmu-emul.c | 3 +++ > > arch/arm64/kvm/pmu.c | 12 ++++-------- > > include/kvm/arm_pmu.h | 6 ++++++ > > 5 files changed, 21 insertions(+), 28 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index dfd360404dd8..90476e713643 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -273,14 +273,8 @@ 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 host_ctxt; > > - struct kvm_pmu_events pmu_events; > > }; > > > > struct kvm_host_psci_config { > > @@ -763,6 +757,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome); > > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); > > > > DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data); > > +DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events); > > > > static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt) > > { > > @@ -821,6 +816,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); > > void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); > > void kvm_clr_pmu_events(u32 clr); > > > > +struct kvm_pmu_events *kvm_get_pmu_events(void); > > void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); > > void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > > #else > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index 0716163313d6..c61120ec8d1a 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -153,13 +153,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu) > > /* > > * Disable host events, enable guest events > > */ > > -static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) > > +static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu) > > { > > - struct kvm_host_data *host; > > - struct kvm_pmu_events *pmu; > > - > > - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); > > - pmu = &host->pmu_events; > > + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; > > > > if (pmu->events_host) > > write_sysreg(pmu->events_host, pmcntenclr_el0); > > @@ -173,13 +169,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) > > /* > > * Disable guest events, enable host events > > */ > > -static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) > > +static void __pmu_switch_to_host(struct kvm_vcpu *vcpu) > > { > > - struct kvm_host_data *host; > > - struct kvm_pmu_events *pmu; > > - > > - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); > > - pmu = &host->pmu_events; > > + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; > > > > if (pmu->events_guest) > > write_sysreg(pmu->events_guest, pmcntenclr_el0); > > @@ -304,7 +296,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > > host_ctxt->__hyp_running_vcpu = vcpu; > > guest_ctxt = &vcpu->arch.ctxt; > > > > - pmu_switch_needed = __pmu_switch_to_guest(host_ctxt); > > + pmu_switch_needed = __pmu_switch_to_guest(vcpu); > > > > __sysreg_save_state_nvhe(host_ctxt); > > /* > > @@ -366,7 +358,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > > __debug_restore_host_buffers_nvhe(vcpu); > > > > if (pmu_switch_needed) > > - __pmu_switch_to_host(host_ctxt); > > + __pmu_switch_to_host(vcpu); > > > > /* Returning to host will clear PSR.I, remask PMR if needed */ > > if (system_uses_irq_prio_masking()) > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > > index 3dc990ac4f44..08d0551a4e43 100644 > > --- a/arch/arm64/kvm/pmu-emul.c > > +++ b/arch/arm64/kvm/pmu-emul.c > > @@ -406,6 +406,9 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu) > > if (!kvm_vcpu_has_pmu(vcpu)) > > return; > > > > + if (!has_vhe()) > > + pmu->events = *kvm_get_pmu_events(); > > A bit of context: > > preempt_disable(); > > /* > * The VMID allocator only tracks active VMIDs per > * physical CPU, and therefore the VMID allocated may not be > * preserved on VMID roll-over if the task was preempted, > * making a thread's VMID inactive. So we need to call > * kvm_arm_vmid_update() in non-premptible context. > */ > kvm_arm_vmid_update(&vcpu->arch.hw_mmu->vmid); > > kvm_pmu_flush_hwstate(vcpu); > > local_irq_disable(); > > You *still* are in a context where an interrupt can fire and mess > things up behind your back. Not good. Also, this is now synchronised > *twice* per run (once on flush, once on sync). Do we really need this? Yes of course, it needs to happen after interrupts have been disabled. Will fix that. > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
Hi Marc, On Mon, May 9, 2022 at 11:41 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 09 May 2022 10:54:59 +0100, > Fuad Tabba <tabba@google.com> wrote: > > > > Instead of the host accessing hyp data directly, pass the pmu > > events of the current cpu to hyp via the vcpu. > > > > This adds 64 bits (in two fields) to the vcpu that need to be > > synced before every vcpu run in nvhe and protected modes. > > However, it isolates the hypervisor from the host, which allows > > us to use pmu in protected mode in a subsequent patch. > > > > No functional change intended. > > BTW, there *are* plenty of functional changes, even if there was no > issue with the patch. The lifetime of the data structure changes, its > sharing changes, and its memory footprint is different. > > I guess what you are trying to say is that there shouldn't be any > visible side effect of this patch. But I don't think these are the > same things. Always thought that in this context functional == functionality. Will make it clearer in the next commit message that there should be no visible side effect changes instead. Thanks, /fuad > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index dfd360404dd8..90476e713643 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -273,14 +273,8 @@ 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 host_ctxt; - struct kvm_pmu_events pmu_events; }; struct kvm_host_psci_config { @@ -763,6 +757,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome); struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data); +DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events); static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt) { @@ -821,6 +816,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); void kvm_clr_pmu_events(u32 clr); +struct kvm_pmu_events *kvm_get_pmu_events(void); void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); #else diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 0716163313d6..c61120ec8d1a 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -153,13 +153,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu) /* * Disable host events, enable guest events */ -static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) +static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu) { - struct kvm_host_data *host; - struct kvm_pmu_events *pmu; - - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); - pmu = &host->pmu_events; + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; if (pmu->events_host) write_sysreg(pmu->events_host, pmcntenclr_el0); @@ -173,13 +169,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt) /* * Disable guest events, enable host events */ -static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) +static void __pmu_switch_to_host(struct kvm_vcpu *vcpu) { - struct kvm_host_data *host; - struct kvm_pmu_events *pmu; - - host = container_of(host_ctxt, struct kvm_host_data, host_ctxt); - pmu = &host->pmu_events; + struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events; if (pmu->events_guest) write_sysreg(pmu->events_guest, pmcntenclr_el0); @@ -304,7 +296,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) host_ctxt->__hyp_running_vcpu = vcpu; guest_ctxt = &vcpu->arch.ctxt; - pmu_switch_needed = __pmu_switch_to_guest(host_ctxt); + pmu_switch_needed = __pmu_switch_to_guest(vcpu); __sysreg_save_state_nvhe(host_ctxt); /* @@ -366,7 +358,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) __debug_restore_host_buffers_nvhe(vcpu); if (pmu_switch_needed) - __pmu_switch_to_host(host_ctxt); + __pmu_switch_to_host(vcpu); /* Returning to host will clear PSR.I, remask PMR if needed */ if (system_uses_irq_prio_masking()) diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 3dc990ac4f44..08d0551a4e43 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -406,6 +406,9 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu) if (!kvm_vcpu_has_pmu(vcpu)) return; + if (!has_vhe()) + pmu->events = *kvm_get_pmu_events(); + overflow = !!kvm_pmu_overflow_status(vcpu); if (pmu->irq_level == overflow) return; diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c index 4bd38ff34221..c19bf6e4969e 100644 --- a/arch/arm64/kvm/pmu.c +++ b/arch/arm64/kvm/pmu.c @@ -5,7 +5,8 @@ */ #include <linux/kvm_host.h> #include <linux/perf_event.h> -#include <asm/kvm_hyp.h> + +DEFINE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events); /* * Given the perf event attributes and system type, determine @@ -25,14 +26,9 @@ static bool kvm_pmu_switch_needed(struct perf_event_attr *attr) return (attr->exclude_host != attr->exclude_guest); } -static struct kvm_pmu_events *kvm_get_pmu_events(void) +struct kvm_pmu_events *kvm_get_pmu_events(void) { - struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data); - - if (!ctx) - return NULL; - - return &ctx->pmu_events; + return this_cpu_ptr(&kvm_pmu_events); } /* diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h index eaa8290b116f..35a0903cae32 100644 --- a/include/kvm/arm_pmu.h +++ b/include/kvm/arm_pmu.h @@ -20,8 +20,14 @@ struct kvm_pmc { struct perf_event *perf_event; }; +struct kvm_pmu_events { + u32 events_host; + u32 events_guest; +}; + struct kvm_pmu { struct irq_work overflow_work; + struct kvm_pmu_events events; struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS]; DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS); int irq_num;
Instead of the host accessing hyp data directly, pass the pmu events of the current cpu to hyp via the vcpu. This adds 64 bits (in two fields) to the vcpu that need to be synced before every vcpu run in nvhe and protected modes. However, it isolates the hypervisor from the host, which allows us to use pmu in protected mode in a subsequent patch. No functional change intended. Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/include/asm/kvm_host.h | 8 ++------ arch/arm64/kvm/hyp/nvhe/switch.c | 20 ++++++-------------- arch/arm64/kvm/pmu-emul.c | 3 +++ arch/arm64/kvm/pmu.c | 12 ++++-------- include/kvm/arm_pmu.h | 6 ++++++ 5 files changed, 21 insertions(+), 28 deletions(-)