Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On 1/5/21 4:25 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2021-01-05 15:49, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 1/4/21 5:27 PM, Marc Zyngier wrote:
>>> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
>>> and makes no sense now that we are 64bit only. Get rid of it.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/Kconfig  | 8 --------
>>>  arch/arm64/kvm/Makefile | 2 +-
>>>  include/kvm/arm_pmu.h   | 2 +-
>>>  3 files changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>> index 043756db8f6e..3964acf5451e 100644
>>> --- a/arch/arm64/kvm/Kconfig
>>> +++ b/arch/arm64/kvm/Kconfig
>>> @@ -49,14 +49,6 @@ if KVM
>>>
>>>  source "virt/kvm/Kconfig"
>>>
>>> -config KVM_ARM_PMU
>>> -    bool "Virtual Performance Monitoring Unit (PMU) support"
>>> -    depends on HW_PERF_EVENTS
>>> -    default y
>>> -    help
>>> -      Adds support for a virtual Performance Monitoring Unit (PMU) in
>>> -      virtual machines.
>>> -
>>>  endif # KVM
>>>
>>>  endif # VIRTUALIZATION
>>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>>> index 60fd181df624..13b017284bf9 100644
>>> --- a/arch/arm64/kvm/Makefile
>>> +++ b/arch/arm64/kvm/Makefile
>>> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
>>> $(KVM)/eventfd.o \
>>>       vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>>>       vgic/vgic-its.o vgic/vgic-debug.o
>>>
>>> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
>>> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
>>> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
>>> index fc85f50fa0e9..8dcb3e1477bc 100644
>>> --- a/include/kvm/arm_pmu.h
>>> +++ b/include/kvm/arm_pmu.h
>>> @@ -13,7 +13,7 @@
>>>  #define ARMV8_PMU_CYCLE_IDX        (ARMV8_PMU_MAX_COUNTERS - 1)
>>>  #define ARMV8_PMU_MAX_COUNTER_PAIRS    ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>>>
>>> -#ifdef CONFIG_KVM_ARM_PMU
>>> +#ifdef CONFIG_HW_PERF_EVENTS
>>>
>>>  struct kvm_pmc {
>>>      u8 idx;    /* index into the pmu->pmc array */
>>
>> I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care of all its
>> occurrences.
>>
>> A few things popped into my mind when I saw the patch.
>>
>> 1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not possible
>> anymore for the host to have perf support while KVM does not support emulating a
>> PMU. In this scenario, functions which would have been empty functions if
>> KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
>> kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and return early
>> after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to me, and I
>> don't think this configuration was common (especially since the default was y).
>
> I don't think this is either common nor useful. If the kernel supports
> the PMU, then finding a PMU enables all the uses of the PMU, including
> for guests. And userspace is still in control of what it exposes to
> the guest. Yes, it's a tiny more overhead (one extra load) on the
> exit/entry path. Should we care? I don't think so.

Yes, I agree:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

>
>> 2. I did a grep for the files that include arm_pmu.h, and all the files were in
>> arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
>> arch/arm64/include/asm because it was shared with KVM/arm when it was still
>> around, right? Or is there another reason for that?
>
> No, that's basically the only reason. That was the easy landing spot
> for anything shared (including things like GIC, timers and co).
> I'm not sure it is worth the move, TBH...

I feel the same way, I was asking more in the context of new code. I admit to also
having an ulterior motive, as one of the patches I picked up for the SPE series
added a header file in include/kvm and it looked a bit out of place.

Thanks,
Alex
>
>> [1] https://www.spinics.net/lists/kvm-arm/msg44184.html
>
> Yuo, and that's the reason I posted this patch. I have a couple more
> that I'll post by the end of the day.
>
> Thanks,
>
>         M.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm