Re: [PATCH v2 1/4] KVM: arm64: Narrow PMU sysreg reset values to architectural requirements

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

Hi Marc,

On 7/19/21 4:56 PM, Marc Zyngier wrote:
> On 2021-07-19 16:55, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 7/19/21 1:38 PM, Marc Zyngier wrote:
>>> A number of the PMU sysregs expose reset values that are not
>>> compliant with the architecture (set bits in the RES0 ranges,
>>> for example).
>>>
>>> This in turn has the effect that we need to pointlessly mask
>>> some register fields when using them.
>>>
>>> Let's start by making sure we don't have illegal values in the
>>> shadow registers at reset time. This affects all the registers
>>> that dedicate one bit per counter, the counters themselves,
>>> PMEVTYPERn_EL0 and PMSELR_EL0.
>>>
>>> Reported-by: Alexandre Chartre <alexandre.chartre@oracle.com>
>>> Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
>>> Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/sys_regs.c | 43 ++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 40 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index f6f126eb6ac1..96bdfa0e68b2 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -603,6 +603,41 @@ static unsigned int pmu_visibility(const struct kvm_vcpu
>>> *vcpu,
>>>      return REG_HIDDEN;
>>>  }
>>>
>>> +static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>>> +{
>>> +    u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
>>> +
>>> +    /* No PMU available, any PMU reg may UNDEF... */
>>> +    if (!kvm_arm_support_pmu_v3())
>>> +        return;
>>> +
>>> +    n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
>>> +    n &= ARMV8_PMU_PMCR_N_MASK;
>>> +    if (n)
>>> +        mask |= GENMASK(n - 1, 0);
>>
>> Hm... seems to be missing the cycle counter.
>
> Check the declaration for 'mask'... :-)

Yeah, sorry for that, I still had in my mind the original function body.

Everything looks alright to me, no changes from the previous version (PMSWINC_EL1
is handled in the last patch) where I had checked that the reset values match the
architecture:

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

Thanks,

Alex