On Fri, May 09 2014 at 3:07:23 pm BST, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote: > On Wed, Apr 16, 2014 at 02:39:49PM +0100, Marc Zyngier wrote: >> GICv3 requires the hcr_el2 switch to be tightly coupled with some >> of the interrupt controller's register switch. > > can you be more specific, this feels a bit odd, enabling Stage-2 > translation and configuring all traps from within the vgic code... The IMO and FMO bits must be set before restoring the various system registers in GICv3. But I agreee that this looks pretty horrible. The alternative is to split the bits we set in HCR_EL2 into two sets (VM and trap control on one side, interrupt control on the other). This would translate into two accesses to HCR_EL2, but it would look nicer. I'll have a look. >> In order to have similar code paths, start moving the hcr_el2 >> manipulation code to the GICv2 switch code. >> >> Acked-by: Catalin Marinas <catalin.marinas@xxxxxxx> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >> --- >> arch/arm64/kvm/hyp.S | 7 ------- >> arch/arm64/kvm/vgic-v2-switch.S | 8 ++++++++ >> 2 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index aed72d0..92b9120 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -335,11 +335,6 @@ >> .endm >> >> .macro activate_traps >> - ldr x2, [x0, #VCPU_IRQ_LINES] >> - ldr x1, [x0, #VCPU_HCR_EL2] >> - orr x2, x2, x1 >> - msr hcr_el2, x2 >> - >> ldr x2, =(CPTR_EL2_TTA) >> msr cptr_el2, x2 >> >> @@ -353,8 +348,6 @@ >> .endm >> >> .macro deactivate_traps >> - mov x2, #HCR_RW >> - msr hcr_el2, x2 >> msr cptr_el2, xzr >> msr hstr_el2, xzr >> >> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S >> index c5dc777..d36cd7a 100644 >> --- a/arch/arm64/kvm/vgic-v2-switch.S >> +++ b/arch/arm64/kvm/vgic-v2-switch.S >> @@ -85,6 +85,9 @@ CPU_BE( rev w5, w5 ) >> sub w4, w4, #1 >> cbnz w4, 1b >> 2: >> + mov x2, #HCR_RW >> + msr hcr_el2, x2 >> + isb >> .endm >> >> /* >> @@ -92,6 +95,11 @@ CPU_BE( rev w5, w5 ) >> * x0: Register pointing to VCPU struct >> */ >> .macro restore_vgic_v2_state >> + ldr x2, [x0, #VCPU_IRQ_LINES] > > will this ever have any values on aarch64? Don't we mandate vgic > support and bail out during hyp init if we cannot init a vgic? Yes. But that doesn't mean we don't support the feature either. The case is fairly slim, I agree, but it has been there since Day-1... M. -- Jazz is not dead. It just smells funny. _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
