Hi Marc, this version of the patch works for me. Tested-by: Vikram Sethi <[email protected]>
Thanks, Vikram On 06/17/15 04:27, Marc Zyngier wrote: > On VM entry, we disable access to the VFP registers in order to > perform a lazy save/restore of these registers. > > On VM exit, we restore access, test if we did enable them before, > and save/restore the guest/host registers if necessary. In this > sequence, the FPEXC register is always accessed, irrespective > of the trapping configuration. > > If the guest didn't touch the VFP registers, then the HCPTR access > has now enabled such access, but we're missing a barrier to ensure > architectural execution of the new HCPTR configuration. If the HCPTR > access has been delayed/reordered, the subsequent access to FPEXC > will cause a trap, which we aren't prepared to handle at all. > > The same condition exists when trapping to enable VFP for the guest. > > The fix is to introduce a barrier after enabling VFP access. In the > vmexit case, it can be relaxed to only takes place if the guest hasn't > accessed its view of the VFP registers, making the access to FPEXC safe. > > The set_hcptr macro is modified to deal with both vmenter/vmexit and > vmtrap operations, and now takes an optional label that is branched to > when the guest hasn't touched the VFP registers. > > Reported-by: Vikram Sethi <[email protected]> > Cc: [email protected] # v3.9+ > Signed-off-by: Marc Zyngier <[email protected]> > --- > * From v1: > - Changed from a discrete fix to be integrated in set_hcptr > - Also introduce an ISB on vmtrap (reported by Vikram) > - Dropped Christoffer Reviewed-by, due to significant changes > > arch/arm/kvm/interrupts.S | 10 ++++------ > arch/arm/kvm/interrupts_head.S | 20 ++++++++++++++++++-- > 2 files changed, 22 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > index 79caf79..f7db3a5 100644 > --- a/arch/arm/kvm/interrupts.S > +++ b/arch/arm/kvm/interrupts.S > @@ -170,13 +170,9 @@ __kvm_vcpu_return: > @ Don't trap coprocessor accesses for host kernel > set_hstr vmexit > set_hdcr vmexit > - set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)) > + set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)), > after_vfp_restore > > #ifdef CONFIG_VFPv3 > - @ Save floating point registers we if let guest use them. > - tst r2, #(HCPTR_TCP(10) | HCPTR_TCP(11)) > - bne after_vfp_restore > - > @ Switch VFP/NEON hardware state to the host's > add r7, vcpu, #VCPU_VFP_GUEST > store_vfp_state r7 > @@ -188,6 +184,8 @@ after_vfp_restore: > @ Restore FPEXC_EN which we clobbered on entry > pop {r2} > VFPFMXR FPEXC, r2 > +#else > +after_vfp_restore: > #endif > > @ Reset Hyp-role > @@ -483,7 +481,7 @@ switch_to_guest_vfp: > push {r3-r7} > > @ NEON/VFP used. Turn on VFP access. > - set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11)) > + set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11)) > > @ Switch VFP/NEON hardware state to the guest's > add r7, r0, #VCPU_VFP_HOST > diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S > index 35e4a3a..48efe2e 100644 > --- a/arch/arm/kvm/interrupts_head.S > +++ b/arch/arm/kvm/interrupts_head.S > @@ -591,8 +591,13 @@ ARM_BE8(rev r6, r6 ) > .endm > > /* Configures the HCPTR (Hyp Coprocessor Trap Register) on entry/return > - * (hardware reset value is 0). Keep previous value in r2. */ > -.macro set_hcptr operation, mask > + * (hardware reset value is 0). Keep previous value in r2. > + * An ISB is emited on vmexit/vmtrap, but executed on vmexit only if > + * VFP wasn't already enabled (always executed on vmtrap). > + * If a label is specified with vmexit, it is branched to if VFP wasn't > + * enabled. > + */ > +.macro set_hcptr operation, mask, label = none > mrc p15, 4, r2, c1, c1, 2 > ldr r3, =\mask > .if \operation == vmentry > @@ -601,6 +606,17 @@ ARM_BE8(rev r6, r6 ) > bic r3, r2, r3 @ Don't trap defined coproc-accesses > .endif > mcr p15, 4, r3, c1, c1, 2 > + .if \operation != vmentry > + .if \operation == vmexit > + tst r2, #(HCPTR_TCP(10) | HCPTR_TCP(11)) > + beq 1f > + .endif > + isb > + .if \label != none > + b \label > + .endif > +1: > + .endif > .endm > > /* Configures the HDCR (Hyp Debug Configuration Register) on entry/return -- Vikram Sethi Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
