On Mon, Apr 15, 2019 at 02:43:36PM +0100, Suzuki K Poulose wrote:
>
>
> On 09/04/2019 20:22, Andrew Murray wrote:
> > With VHE different exception levels are used between the host (EL2) and
> > guest (EL1) with a shared exception level for userpace (EL0). We can take
> > advantage of this and use the PMU's exception level filtering to avoid
> > enabling/disabling counters in the world-switch code. Instead we just
>
> s/Instead// ?
>
> > modify the counter type to include or exclude EL0 at vcpu_{load,put} time.
>
>
> >
> > We also ensure that trapped PMU system register writes do not re-enable
> > EL0 when reconfiguring the backing perf events.
> >
> > This approach completely avoids blackout windows seen with !VHE.
> >
> > Suggested-by: Christoffer Dall <christoffer.dall@xxxxxxx>
> > Signed-off-by: Andrew Murray <andrew.murray@xxxxxxx>
> > Acked-by: Will Deacon <will.deacon@xxxxxxx>
>
> ....
>
> > +/*
> > + * On VHE ensure that only guest events have EL0 counting enabled
> > + */
> > +void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm_cpu_context *host_ctxt;
> > + struct kvm_host_data *host;
> > + u32 events_guest, events_host;
> > +
> > + if (!has_vhe())
> > + return;
> > +
> > + host_ctxt = vcpu->arch.host_cpu_context;
> > + host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> > + events_guest = host->pmu_events.events_guest;
> > + events_host = host->pmu_events.events_host;
> > +
> > + kvm_vcpu_pmu_enable_el0(events_guest);
> > + kvm_vcpu_pmu_disable_el0(events_host);
> > +}
> > +
> > +/*
> > + * On VHE ensure that only guest host have EL0 counting enabled
>
> nit: s/guest/host/host events/
>
> > + */
> > +void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm_cpu_context *host_ctxt;
> > + struct kvm_host_data *host;
> > + u32 events_guest, events_host;
> > +
> > + if (!has_vhe())
> > + return;
> > +
> > + host_ctxt = vcpu->arch.host_cpu_context;
> > + host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> > + events_guest = host->pmu_events.events_guest;
> > + events_host = host->pmu_events.events_host;
> > +
> > + kvm_vcpu_pmu_enable_el0(events_host);
> > + kvm_vcpu_pmu_disable_el0(events_guest);
> > +}
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 539feecda5b8..c7fa47ad2387 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -695,6 +695,7 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > val |= p->regval & ARMV8_PMU_PMCR_MASK;
> > __vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> > kvm_pmu_handle_pmcr(vcpu, val);
> > + kvm_vcpu_pmu_restore_guest(vcpu);
>
> nit: I am not sure if we need to do this for PMCR accesses ? Unless
> we have modified some changes to the events (e.g, like the two instances
> below). Or am I missing something here ?
Thanks for the review.
The reason is that PMCR, via the E bit, can lead to kvm_pmu_handle_pmcr
calling kvm_pmu_enable_counter and perf_event_enable. If I recall correctly,
perf_event_enable could lead to armv8pmu_enable_event which may rewrite
config_base. However in this context (trap) we stay in the guest and may
not call kvm_vcpu_pmu_restore_{guest,host} for a while - thus the EL0 enable
bits of the underlying event may be incorrect (for example when exclude_host
is set we exclude EL0 from config_base - however as we are in the guest we
need it to be included!).
Thanks,
Andrew Murray
>
> Otherwise:
>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
