Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf <graf@amazon.com>]

Hi Marc,

On 10.03.20 19:00, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny 
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84 
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.

I haven't seen a v3 follow-up after this. Do you happen to have that 
somewhere in a local branch and just need to send it out or would you 
prefer if I pick up v2 and address the comments?


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879