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).
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 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.
