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

[Julien Thierry <julien.thierry@arm.com>]

On 12/12/2018 10:42, Suzuki K Poulose wrote:
> 
> 
> On 12/12/2018 10:29, Andrew Murray wrote:
>> 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);
>> +    }
> 
> Shouldn't we disable the events, irrespective of whether it is for host
> or/and guest ? Otherwise looks good to me.
> 

I made the opposite remark on one of the early version. My reasoning is
that, if we rely on the hypervisor to enable event that exclude the
host, we should also rely on the hypervisor disabling these events.

At least in my mind it makes more sense this way.

Cheers,

-- 
Julien Thierry

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