| Message ID | 20220401010832.3425787-4-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Limit feature register reads from AArch32 | expand |
On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote: > > To date KVM has not trapped ID register accesses from AArch32, meaning > that guests get an unconstrained view of what hardware supports. This > can be a serious problem because we try to base the guest's feature > registers on values that are safe system-wide. Furthermore, KVM does not > implement the latest ISA in the PMU and Debug architecture, so we > constrain these fields to supported values. > > Since KVM now correctly handles CP15 and CP10 register traps, we no > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead > emulate reads with their safe values. > > Signed-off-by: Oliver Upton <oupton@google.com> Reviewed-by: Reiji Watanabe <reijiw@google.com> BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will become 0 for the aarch32 guest without PMUv3. This is the correct behavior, but it affects migration. I'm not sure how much we should care about migration of the aarch32 guest though (and it will be resolved once ID registers become configurable anyway). Thanks, Reiji
Hi Reiji, On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote: > On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote: > > > > To date KVM has not trapped ID register accesses from AArch32, meaning > > that guests get an unconstrained view of what hardware supports. This > > can be a serious problem because we try to base the guest's feature > > registers on values that are safe system-wide. Furthermore, KVM does not > > implement the latest ISA in the PMU and Debug architecture, so we > > constrain these fields to supported values. > > > > Since KVM now correctly handles CP15 and CP10 register traps, we no > > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead > > emulate reads with their safe values. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will > become 0 for the aarch32 guest without PMUv3. This is the correct behavior, > but it affects migration. I'm not sure how much we should care about > migration of the aarch32 guest though (and it will be resolved once ID > registers become configurable anyway). I believe userspace has been accessing the sanitised values of these feature registers the entire time, so we should be OK on the UAPI side. From the guest's perspective, I don't believe there is a meaningful change. Even if the guest were to believe the value it sees in ID_DFR0.PerfMon, it'll crash and burn on the first attempt to poke a PMU register as we synthesize an UNDEF, right? At least now we cover our tracks and ensure the vCPU correctly identifies itself to the guest. This is, of course, unless I missed something painfully obvious :) -- Thanks, Oliver
Hi Oliver, On Sun, Apr 3, 2022 at 10:46 PM Oliver Upton <oupton@google.com> wrote: > > Hi Reiji, > > On Sun, Apr 03, 2022 at 09:45:15PM -0700, Reiji Watanabe wrote: > > On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote: > > > > > > To date KVM has not trapped ID register accesses from AArch32, meaning > > > that guests get an unconstrained view of what hardware supports. This > > > can be a serious problem because we try to base the guest's feature > > > registers on values that are safe system-wide. Furthermore, KVM does not > > > implement the latest ISA in the PMU and Debug architecture, so we > > > constrain these fields to supported values. > > > > > > Since KVM now correctly handles CP15 and CP10 register traps, we no > > > longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead > > > emulate reads with their safe values. > > > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > > > BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will > > become 0 for the aarch32 guest without PMUv3. This is the correct behavior, > > but it affects migration. I'm not sure how much we should care about > > migration of the aarch32 guest though (and it will be resolved once ID > > registers become configurable anyway). > > I believe userspace has been accessing the sanitised values of these > feature registers the entire time, so we should be OK on the UAPI side. You are right:) I totally forgot this was just about trapping. Sorry for the noise. Thanks, Reiji
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 01d47c5886dc..2fc2d995c10a 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -80,11 +80,12 @@ * FMO: Override CPSR.F and enable signaling with VF * SWIO: Turn set/way invalidates into set/way clean+invalidate * PTW: Take a stage2 fault if a stage1 walk steps in device memory + * TID3: Trap EL1 reads of group 3 ID registers */ #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ HCR_BSU_IS | HCR_FB | HCR_TACR | \ HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \ - HCR_FMO | HCR_IMO | HCR_PTW ) + HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 ) #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF) #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA) #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC) diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index d62405ce3e6d..fe32b4c8b35b 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) vcpu->arch.hcr_el2 &= ~HCR_RW; - /* - * TID3: trap feature register accesses that we virtualise. - * For now this is conditional, since no AArch32 feature regs - * are currently virtualised. - */ - if (!vcpu_el1_is_32bit(vcpu)) - vcpu->arch.hcr_el2 |= HCR_TID3; - if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) || vcpu_el1_is_32bit(vcpu)) vcpu->arch.hcr_el2 |= HCR_TID2;
To date KVM has not trapped ID register accesses from AArch32, meaning that guests get an unconstrained view of what hardware supports. This can be a serious problem because we try to base the guest's feature registers on values that are safe system-wide. Furthermore, KVM does not implement the latest ISA in the PMU and Debug architecture, so we constrain these fields to supported values. Since KVM now correctly handles CP15 and CP10 register traps, we no longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead emulate reads with their safe values. Signed-off-by: Oliver Upton <oupton@google.com> --- arch/arm64/include/asm/kvm_arm.h | 3 ++- arch/arm64/include/asm/kvm_emulate.h | 8 -------- 2 files changed, 2 insertions(+), 9 deletions(-)