[PATCH v3 17/19] arm64: KVM: move hcr_el2 setting into vgic-v2-switch.S

[Christoffer Dall]

On Wed, May 14, 2014 at 05:58:04PM +0100, Marc Zyngier wrote:
> On 14/05/14 17:34, Christoffer Dall wrote:
> > On 14 May 2014 15:33, Marc Zyngier <marc.zyngier at arm.com> wrote:
> >> On Fri, May 09 2014 at  3:07:23 pm BST, Christoffer Dall <christoffer.dall at linaro.org> 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 at arm.com>
> >>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> >>>> ---
> >>>>  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...
> >>
> > See kvm_vm_ioctl_irq_line() in arch/arm/kvm/arm.c:
> > 
> > case KVM_ARM_IRQ_TYPE_CPU:
> >     if (irqchip_in_kernel(kvm))
> >         return -ENXIO;
> 
> Unfortunately, this only checks if the VM has a vgic instantiated. It is
> always possible to create a VM without the in-kernel GIC, and use the
> pins to inject IRQs. As I said, unlikely to happen, but nonetheless...
> 
Yeah, you're right, I'm an idiot. Sorry for the noise.

Thanks,
-Christoffer