Re: [Xen-devel] [Draft D] Xen on ARM vITS Handling

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-06-05 at 13:15 +0100, Julien Grall wrote:
> >> I'm struggling to see how this would work. After enabling/disabling an
> >> IRQ you need to send an INV which require the devID and the eventID.
> > 
> > This is a detail for the pits driver, which is deliberately out of scope
> > here. The interface to core Xen system is enable_irq/disable_irq and so
> > that is what we must use here.
> > 
> > I imagine the pits driver will need to store some details about this
> > somewhere for its own internal purposes, perhaps in struct its_device.
> 
> It's rather strange to put this out of scope because it would have
> solved your issue (i.e getting the vDevID, ID).

Details of the pits are out of scope so the fact that enabling or
disabling an interrupt needs a specific set of operations and info is
out of scope.

The fact that a vLPU->pLPI mapping might be helpful for some other issue
within the vits is a separate consideration. You might be correct that
this would help the LPI CFG emulation case, but: 

> If you have a mapping vLPI -> pLPI, you can therefore get the its_device
> which store the vDevID and you can device the eventID.

Previously you mentioned multiple (device,event) mapping to the same
(v)LPI. I hoped we could rule this out but I wasn't sure, and frankly
I'm no longer hopeful that it is the case.

That possibility makes vPLI->pLPI mapping trickier (if not impossible)
to maintain.

In fact given that LPIs are a GICR thing while (device,event)=>LPI
mapping is an ITS thing, its possible that this is simply allowed and
expected, even if strange.

> Which that you can decide what to do.
> 
> >> Also, until now you haven't explained how the vLPI will be mapped to the
> >> pLPI.
> > 
> > I didn't think anything here so far needed that mapping, so of course I
> > didn't explain it.
> > 
> > If some alternative scheme which you are proposing (or solution to an
> > outstanding issue) requires this mapping then we would have to add it.
> > 
> >>> LPI priority (the remaining bits in the written byte) is currently
> >>> ignored.
> >>>
> >>> ## LPI Pending Table Virtualisation
> >>>
> >>> XXX Can we simply ignore this? 4.8.5 suggests it is not necessarily in
> >>> sync and the mechanism to force a sync is `IMPLEMENTATION DEFINED`.
> >>>
> >>> ## Device Table Virtualisation
> >>>
> >>> The IPA, size and cacheability attributes of the guest device table
> >>> will be recorded in `struct domain` upon write to `GITS_BASER0`.
> >>>
> >>> In order to lookup an entry for `device`:
> >>>
> >>>     define {get,set}_vdevice_entry(domain, device, struct device_table 
> >>> *entry):
> >>>         offset = device*sizeof(struct vdevice_table)
> >>>         if offset > <DT size>: error
> >>>
> >>>         dt_entry = <DT base IPA> + device*sizeof(struct vdevice_table)
> >>>         page = p2m_lookup(domain, dt_entry, p2m_ram)
> >>>         if !page: error
> >>>         /* nb: non-RAM pages, e.g. grant mappings,
> >>>          * are rejected by this lookup */
> >>
> >> It is allowed to pass device memory here. See Device-nGnRnE.
> > 
> > The guest is allowed to tell us that we should use RAM with strongly
> > ordered etc device like semantics, but telling us to use an actual MMIO
> > region is nuts, do you really think we need to suppor that case?
> 
> I agree that it may be strange but I wasn't able to find a place which
> rule out this possibility...

I think we should rule it out for our use case, it's a legitimate
simplification.

> >> This is not the only issue. With your solution, you let the possibility
> >> to the guest having one vLPI for multiple (devID, ID)/LPI.
> > 
> > I'm not sure, but isn't this UNPREDICTABLE or something? The "Where:"
> > under Figure 19 (and most of the other related tables) suggests that the
> > the identifier must be unique. I'd like to find a stronger statement if
> > we were to rely on this though, true.
> 
> I think so. But you are assuming that the guest is nice and will never
> touch the memory provisioned to the ITS (ITT, Device Table...).

No, I'm not. The document was very clear that Xen should never trust
anything written in that memory and should always carefully bounds check
it.

But if a guest wants to scribble over that memory and mess up its own
interrupt handling it is welcome to do so, just like on native. We don't
need to protect the guest against itself, just the host against the
guest.

> The vLPI is stored in the guest memory. A malicious guest could decide
> to modify different ITT entries to point to the same vLPI. It would be
> impossible to know which pLPI we have to EOI.

There may be an issue here. Thinking out loud:

At the point we come to inject a vPLI we do (or could) know the pLPI,
because we have the irq_desc/guest_irq (or something which would let us
find them) in our hands.

On native it's unclear if mapping multiple (Device,Event) pairs to a
single LPI is valid (or a single (Collection,LPI) pair, I'm not sure it
matters). I can't find where it is rules out, so lets assume it is
allowed.

In that case what would happen if a (DeviceB,EventB) triggers a given
LPI which has already been triggered and is pending due to
(DeviceA,EventA). LPIs are edge triggered so I think the second
interrupt would simply be ignored.

Can we do the same? Would this be harmful to anyone other than the guest
itself (who would no longer receive some interrupts, but its their own
fault).

It's not impossible to imagine (DeviceA,EventA) and (DeviceB,EventB)
mapping to the same LPI but the OS arranging that they are used at
mutually exclusive times and so things would work. Mad but not
impossible to imagine (especially if DeviceA==DeviceB with two events on
it).

> We can't validate this case when the interrupt is mapped via MAPVI as
> the guest can modify afterward the memory. And checking at injection
> time would require to browse all the ITTs.
> 
> So, shall we trust a guest using vITS? If not I'm afraid that we can't
> make usage of memory provisioned by the guest.

This is a false dichotomy, we don't need to trust the guest if we are
careful to validate things as we use them, and if we do this we can use
guest owned memory for things. This was discussed in the spec. Is the
section "Xen interaction with guest OS provisioned vITS memory" lacking
in some way?

> 
> FWIW, this is not the first place in this proposal where you assume the
> table won't be modified by the guest.

It is very much assumed throughout that the guest might maliciously
modify memory it has given to the vITS, it is a part of the design.

If there are cases where Xen is _unable_ to validate something at the
time of use, then that is a problem with the design. The fact that
everywhere else is required to check things before use is part of the
design, not a problem with it.

> >>  Even if we
> >> validate it when the command MAPI/MAPVI is sent, the guest could modify
> >> himself the ITT.
> >>
> >> Furthermore, each virtual IRQ of a domain is associated to a struct
> >> pending_irq. This structure contains an irq_desc which is a pointer to
> >> the physical IRQ. With a craft ITT the guest could mess up those
> >> datastructure. Although, I think we can get back on our feet after the
> >> domain is destroyed.
> >>
> >> After reading this draft, I think we can avoid to browse the Device
> >> Table and the ITT. As said on the previous paragraph, the pending_irq
> >> structure is linked to an irq_desc. In your proposal, you suggested to
> >> store the its_device in the irq_guest (part of irq_desc). If we make
> >> usage of pending_irq->desc to store the physical descriptor, we can have
> >> a mapping vLPI <=> pLPI. Therefore, this would resolve UI1 and AFAICT,
> >> the memory usage would be the same in Xen of the ITT/Device base solution.
> > 
> > If you have a scheme along those lines that would be good. From your
> > next message:
> > 
> >> BTW, I will suggest a pseudo-code based on your draft tomorrow. It may 
> >> be easier to understand.
> > 
> > Yes, please, but please update everything which is affected, add new
> > sections as necessary etc so the document remains a self consistent
> > plausible design.
> 
> I'm afraid to say that I won't be able to come up with a full proposal
> today and I will be away from tonight until the 15th of June.
> 
> I can try to suggest few lead but I still need to think about the INT
> problem.

Be aware that I _have_ thought about the INT problem, which resulted in
the current design and specifically in the decision to keep the device
table (and by extension the ITTs) in guest RAM. This wasn't a whim on my
part, I thought quite hard about it.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel