| Message ID | 20220214065746.1230608-11-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On Sun, Feb 13, 2022 at 10:57:29PM -0800, Reiji Watanabe wrote: > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > expose the value for the guest as it is. Since KVM doesn't support > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > expose 0x0 (PMU is not implemented) instead. > > Change cpuid_feature_cap_perfmon_field() to update the field value > to 0x0 when it is 0xf. Definitely agree with the change in this patch. Do we need to tolerate writes of 0xf for ABI compatibility (even if it is nonsensical)? Otherwise a guest with IMP_DEF PMU cannot be migrated to a newer kernel. -- Thanks, Oliver
Hi Oliver, Thank you for the review! On Tue, Feb 15, 2022 at 10:57 AM Oliver Upton <oupton@google.com> wrote: > > Hi Reiji, > > On Sun, Feb 13, 2022 at 10:57:29PM -0800, Reiji Watanabe wrote: > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > expose the value for the guest as it is. Since KVM doesn't support > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > expose 0x0 (PMU is not implemented) instead. > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > to 0x0 when it is 0xf. > > Definitely agree with the change in this patch. Do we need to tolerate > writes of 0xf for ABI compatibility (even if it is nonsensical)? > Otherwise a guest with IMP_DEF PMU cannot be migrated to a newer kernel. Hmm, yes, I think KVM should tolerate writes of 0xf so that we can avoid the migration failure. I will make this change in v6. Since ID registers are immutable with the current KVM, I think a live migration failure to a newer kernel happens when the newer kernel/KVM supports more CPU features (or when an ID register field is newly masked or capped by KVM, etc). So, I would assume such migration breakage on KVM/ARM has been introduced from time to time though. Thanks, Reiji
On Tue, Feb 15, 2022 at 06:52:27PM -0800, Reiji Watanabe wrote: > Hi Oliver, > > Thank you for the review! > > On Tue, Feb 15, 2022 at 10:57 AM Oliver Upton <oupton@google.com> wrote: > > > > Hi Reiji, > > > > On Sun, Feb 13, 2022 at 10:57:29PM -0800, Reiji Watanabe wrote: > > > When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which > > > means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally > > > expose the value for the guest as it is. Since KVM doesn't support > > > IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should > > > expose 0x0 (PMU is not implemented) instead. > > > > > > Change cpuid_feature_cap_perfmon_field() to update the field value > > > to 0x0 when it is 0xf. > > > > Definitely agree with the change in this patch. Do we need to tolerate > > writes of 0xf for ABI compatibility (even if it is nonsensical)? > > Otherwise a guest with IMP_DEF PMU cannot be migrated to a newer kernel. > > Hmm, yes, I think KVM should tolerate writes of 0xf so that we can > avoid the migration failure. I will make this change in v6. > > Since ID registers are immutable with the current KVM, I think a live > migration failure to a newer kernel happens when the newer kernel/KVM > supports more CPU features (or when an ID register field is newly > masked or capped by KVM, etc). So, I would assume such migration > breakage on KVM/ARM has been introduced from time to time though. > Indeed it has, but IMO migration breakage should really be avoided at all costs. End of the day, its ABI breakage. Unless folks feel otherwise, I would be in favor of just ignoring the IMP_DEF write and setting the field value to NI instead. Allows VMs to migrate onto newer kernels and fixes the KVM bug at the same time. -- Oliver
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index a9edf1ca7dcb..375c9cd0123c 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -553,7 +553,7 @@ cpuid_feature_cap_perfmon_field(u64 features, int field, u64 cap) /* Treat IMPLEMENTATION DEFINED functionality as unimplemented */ if (val == ID_AA64DFR0_PMUVER_IMP_DEF) - val = 0; + return (features & ~mask); if (val > cap) { features &= ~mask;
When ID_AA64DFR0_EL1.PMUVER or ID_DFR0_EL1.PERFMON is 0xf, which means IMPLEMENTATION DEFINED PMU supported, KVM unconditionally expose the value for the guest as it is. Since KVM doesn't support IMPLEMENTATION DEFINED PMU for the guest, in that case KVM should expose 0x0 (PMU is not implemented) instead. Change cpuid_feature_cap_perfmon_field() to update the field value to 0x0 when it is 0xf. Fixes: 8e35aa642ee4 ("arm64: cpufeature: Extract capped perfmon fields") Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/cpufeature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)