Re: [PATCH v2 15/15] xen/arm: gic-v3: Add support of vGICv2 when available

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-06-30 at 18:29 +0100, Julien Grall wrote:
> > 
> >> +     */
> >> +    if ( v->domain->arch.vgic.version == GIC_V2 )
> >> +        val &= ~GICC_SRE_EL2_ENEL1;
> >> +    else
> >> +        val |= GICC_SRE_EL2_ENEL1;
> >> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
> >> +    isb();
> > 
> > Is the isb strictly needed? I suppose we are already using rather too
> > many, perhaps a more complete audit is in order.
> 
> AFAICT no, the ENEL1 doesn't gate any access to EL1 systems register in EL2.
> 
> There is an isb in the caller (gic_restore_state) but I find it
> confusing because we rely on the caller doing the right thing for us.
> I'm thinking to push the isb within the callee for more clarify.

isb's aren't free though.

> It can be part of a bigger audit.

Indeed, this isn't one for now/.

Really there should probably be a single isb at the end of ctxt
save/restore and very little otherwise except where there is an absolute
need for some sort of ordering between steps.

> >> +    /*
> >> +     * When the guest is GICv3, all guest IRQs are Group 1, as Group0
> >> +     * would result in a FIQ in the guest, which it wouldn't expect
> > 
> > Unless the guest actually asked for a FIQ (not sure how that works).
> > Anyway, I don't think you need to support that case here...
> 
> Well, I don't even think that FIQ has ever working with guest and Xen.

Yes, you don't need to anything, I was just idly wondering, sorry for
not being clear about that.

> So I would expect to see more work than this small change.

Absolutely,

> > I wonder how one specifies without GICC, given that GICH comes after
> > it... Oh well!
> 
> GICv3 doesn't use the GICH to control the GIC hypervisor interface but
> the system register.

Oh of course, I'd forgotten.

> > Despite all the comments I don't think there are any blockers:
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Thank you.
>