[PATCH v4 17/19] arm64: KVM: add SGI generation register emulation

[christoffer.dall@linaro.org (Christoffer Dall)]

On Fri, Nov 28, 2014 at 03:40:12PM +0000, Andre Przywara wrote:
> Hej Christoffer,
> 
> On 25/11/14 11:03, Christoffer Dall wrote:
> > Hi Andre,
> > 
> > On Mon, Nov 24, 2014 at 04:37:58PM +0000, Andre Przywara wrote:
> >> Hi,
> >>
> >> On 23/11/14 15:08, Christoffer Dall wrote:
> >>> On Fri, Nov 14, 2014 at 10:08:01AM +0000, Andre Przywara wrote:
> >>>> While the generation of a (virtual) inter-processor interrupt (SGI)
> >>>> on a GICv2 works by writing to a MMIO register, GICv3 uses the system
> >>>> register ICC_SGI1R_EL1 to trigger them.
> >>>> Trap that register on ARM64 hosts and handle it in a new handler
> >>>> function in the GICv3 emulation code.
> >>>
> >>> Did you reorder something or does my previous comment still apply that
> >>> you're not enabling trapping yet, you're just adding the handler - those
> >>> are two different things.
> >>
> >> Yes, I can fix the wording.
> >>
> >>> You sort of left my question about access_gic_sgi() not checking if the
> >>> gicv3 is presetn hanging from the last thread, but I think I'm
> >>> understanding properly now, that as long as you're not setting the
> >>> ICC_SRE_EL2.Enable = 1, then we'll never get here, right?
> >>
> >> Right, that is the idea. Just to make sure that I got this right from
> >> the discussion the other day: We will not trap to EL2 as long as
> >> ICC_SRE_EL2.Enable is 0 - which it should still be at this point, right?
> > 
> > No, when ICC_SRE_EL2.Enable is 0, then Non-secure EL1 access to
> > ICC_SRE_EL1 trap to EL2 (See Section 5.7.39 in the spec), which means
> > that accesses to the ICC_SGIx registers will cause an undefined
> > exception in the guest because we set ICC_SRE_EL1.SRE to 0 for the
> > guest and the guest cannot change this.
> > 
> > Now, when we set ICC_SRE_EL2.Enable to 1, then the guest can set
> > ICC_SRE_EL1.SRE to 1 (and we also happen to reset it to 1), and we will
> > indeed trap on guest access to the ICC_SGIx registers, because all
> > virtual accesses to these registers trap.
> > 
> > (Going back and checking where 'virtual accesses' is defined in the spec
> > left me somewhere without any results, but I am guessing that because we
> > set the ICH_HCR_EL2.En to 1, all accesses will be deemed virtual
> > accesses, maybe the spec should be clarfied on this matter?).
> > 
> > Anyhow, to get back to my original question, getting here requires
> > a situation where the guest copy of the ICC_SRE_EL1.SRE is 1, which we
> > only allow when we have properly initialized the GICv3 data structures.
> 
> So to summarize (and check) this: There is no real issue at this point?
> And the code is totally fine after 19/19?

There is no issue at this point, no.

> 
> Would this kind of problem actually matter _inside_ a patch series? To
> trigger an issue, we would need a bogus guest and bogus userland
> (because at this point neither of them would see/inject a GICv3 FDT
> node). I'd assume that running a kernel at this point is just for
> debugging/bisecting? Where you wouldn't care about every corner case of
> execution?

The argument about bogus guests / fdts should *never* be considered in
the context of these discussions.  If we have code that looks like the
guest can kill the host, or do a NULL pointer dereference, then we need
to address it.

Your point about it being inside a patch series, sure, it's unlikely
that people will run this, but I'm reviewing this patch right now, and
honestly not considering how this changes in the subsequent patch.  For
this sort of thing, if we were leaving a gaping hole open, that would at
least require a clear note in the commit message on why we're doing it.

Hopefully you understood and agreed with my deduction about the various
SRE settings above though?

-Christoffer