Hi Marc,
On Thu, Nov 3, 2022 at 1:44 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi Reiji,
>
> On Thu, 03 Nov 2022 04:55:52 +0000,
> Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > case SYS_ID_DFR0_EL1:
> > > - /* Limit guests to PMUv3 for ARMv8.4 */
> > > - val = cpuid_feature_cap_perfmon_field(val,
> > > - ID_DFR0_PERFMON_SHIFT,
> > > - kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> > > + val &= ~ARM64_FEATURE_MASK(ID_DFR0_PERFMON);
> > > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_PERFMON),
> > > + pmuver_to_perfmon(vcpu_pmuver(vcpu)));
> >
> > Shouldn't KVM expose the sanitized value as it is when AArch32 is
> > not supported at EL0 ? Since the register value is UNKNOWN when AArch32
> > is not supported at EL0, I would think this code might change the PERFMON
> > field value on such systems (could cause live migration to fail).
>
> I'm not sure this would cause anything to fail as we now treat all
> AArch32 idregs as RAZ/WI when AArch32 isn't supported (and the
> visibility callback still applies here).
Oops, sorry I totally forgot about that change...
> But it doesn't hurt to make pmuver_to_perfmon() return 0 when AArch32
> isn't supported, and it will at least make the ID register consistent
> from a guest perspective.
I believe the register will be consistent (0) even from a guest
perspective with the current patch when AArch32 isn't supported
because read_id_reg() checks that with sysreg_visible_as_raz()
in the beginning.
I withdraw my comment, and the patch looks good to me.
Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
Thank you,
Reiji
>
> I plan to squash the following (untested) hack in:
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8f4412cd4bf6..3b28ef48a525 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1094,6 +1094,10 @@ static u8 perfmon_to_pmuver(u8 perfmon)
>
> static u8 pmuver_to_perfmon(u8 pmuver)
> {
> + /* If no AArch32, make the field RAZ */
> + if (!kvm_supports_32bit_el0())
> + return 0;
> +
> switch (pmuver) {
> case ID_AA64DFR0_EL1_PMUVer_IMP:
> return ID_DFR0_PERFMON_8_0;
> @@ -1302,10 +1306,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> {
> - u8 perfmon, host_perfmon = 0;
> + u8 perfmon, host_perfmon;
>
> - if (system_supports_32bit_el0())
> - host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> + host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
>
> /*
> * Allow DFR0_EL1.PerfMon to be set from userspace as long as
>
> > I should have noticed this with the previous version...
>
> No worries, thanks a lot for having had a look!
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
