Re: [Xen-devel] [PATCH v5 13/30] ARM: GICv3: forward pending LPIs to guests

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 6 Apr 2017, Andre Przywara wrote:
> Hi Stefano,
> 
> thanks for spending your brain cells on this nasty code.
> 
> ...
> 
> On 06/04/17 00:45, Stefano Stabellini wrote:
> > On Thu, 6 Apr 2017, Andre Przywara wrote:
> >> Upon receiving an LPI on the host, we need to find the right VCPU and
> >> virtual IRQ number to get this IRQ injected.
> >> Iterate our two-level LPI table to find this information quickly when
> >> the host takes an LPI. Call the existing injection function to let the
> >> GIC emulation deal with this interrupt.
> >> Also we enhance struct pending_irq to cache the pending bit and the
> >> priority information for LPIs, as we can't afford to walk the tables in
> >> guest memory every time we handle an incoming LPI.
> >> This introduces a do_LPI() as a hardware gic_ops and a function to
> >> retrieve the (cached) priority value of an LPI and a vgic_ops.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >>
> >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> >> index 7c86f5b..08d6294 100644
> >> --- a/xen/include/asm-arm/vgic.h
> >> +++ b/xen/include/asm-arm/vgic.h
> >> @@ -66,12 +66,14 @@ struct pending_irq
> >>  #define GIC_IRQ_GUEST_VISIBLE  2
> >>  #define GIC_IRQ_GUEST_ENABLED  3
> >>  #define GIC_IRQ_GUEST_MIGRATING   4
> >> +#define GIC_IRQ_GUEST_LPI_PENDING 5     /* Caches the pending bit of an 
> >> LPI. */
> > 
> > I don't think we need GIC_IRQ_GUEST_LPI_PENDING, you can reuse
> > GIC_IRQ_GUEST_QUEUED and list_empty(&n->inflight): if you call
> > vgic_vcpu_inject_irq passing an irq as argument that is not enabled, it
> > will get GIC_IRQ_GUEST_QUEUED and added to inflight, but not injected.
> 
> Mmh, that's an interesting suggestion.
> The neat thing about piggy-backing on those status bits is that we can
> use the atomic set_bit/clear_bit to set and clear the pending state,
> independently from any code accessing the structure, even without taking
> any lock. Especially since the existing VGIC code does not care about
> this bit.
> However looking at all users of GUEST_LPI_PENDING, I see that we either
> already hold the VGIC lock and we are able to take it, so we might be
> able to use the combination you suggested.
> 
> I tried to make some sense of what "inflight" *really* means, can you
> shed some light on this?

See the comment in xen/include/asm-arm/vgic.h.

vgic_vcpu_inject_irq(irq) is called:
- GIC_IRQ_GUEST_QUEUED is set
- pending_irq is added to the inflight list

At this stage the irq is tracked as being inflight, but not really
injected into the guest yet.

Once/If the irq is enabled, we try to inject it into the guest.
gic_raise_guest_irq(irq) is called:
- if there is a free LR
  - clear GIC_IRQ_GUEST_QUEUED
  - set GIC_IRQ_GUEST_VISIBLE
- otherwise
  - add pending_irq to lr_pending
    - pending_irq is still on the inflight list too
    - pending_irq will be removed from lr_pending, and the irq added to an LR 
when
      available

When the guest finishes with the irq and EOIes it:
gic_update_one_lr is called:
- clear LR
- clear GIC_IRQ_GUEST_VISIBLE
- pending_irq is removed from inflight


Thus, you can use list_empty(&p->inflight) and QUEUED to check if the
LPI has been set to PENDING but it hasn't been injected yet, because the
corresponding pending_irq remains on the inflight list until the LPI has
been EOIed. but QUEUED is cleared as soon as the irq is added to an LR.

It is possible to set an interrupt as QUEUED, even if pending_irq is
already inflight and the irq is VISIBLE. That just means that the
physical irq has been reasserted after the virq becomes ACTIVE and
before the EOI.


> My understanding is that it's about IRQs that should be injected, but
> are not ready (because they are disabled). Are there other cases IRQs
> are in the "inflight" list? Is that also the spillover in case all LRs
> are used (in this case they would GIC_IRQ_GUEST_ENABLED)?

No. If all LRs are used, pending_irq is added to lr_pending. However,
pending_irq *also* remains in the inflight list until EOI.

If (ENABLED && QUEUED && inflight) is the condition to try to inject the
irq into the guest. If list_empty(&v->arch.vgic.lr_pending) is the
condition to actually be able to add the irq to an lr.


> >>      unsigned long status;
> >>      struct irq_desc *desc; /* only set it the irq corresponds to a 
> >> physical irq */
> >>      unsigned int irq;
> >>  #define GIC_INVALID_LR         (uint8_t)~0
> >>      uint8_t lr;
> >>      uint8_t priority;
> >> +    uint8_t lpi_priority;       /* Caches the priority if this is an LPI. 
> >> */
> > 
> > The commit message says: "we enhance struct pending_irq to cache the
> > pending bit and the priority information for LPIs, as we can't afford to
> > walk the tables in guest memory every time we handle an incoming LPI." I
> > thought it would be direct access, having the vlpi number in our hands?
> > Why is it a problem?
> > 
> > If there has been a conversation about this that I am missing, please
> > provide a link, I'll go back and read it.
> 
> Well, the property table is in guest memory as the other ITS tables and
> we now access this in a new way (vgic_access_guest_memory()), which is
> quite costly: we need to do the p2m lookup, map the page, access the
> data, unmap the page and put the page back. 

map, unmap and put are (almost) nop. The only operation is the
p2m_lookup that could be easily avoided by storing the struct page_info*
or mfn corresponding to the guest-provided data structures. We should be
able to do this in a very small number of steps, right?


> Everything on itself is not
> really too demanding (on arm64), but doing this in the interrupt
> handling path to learn the priority value sounds a bit over the top.
> For the *ITS* emulation (command handling) we can cope with this
> overhead (because this is only needing during the virtual command
> handling), but I wanted to avoid this in the generic GIC code, which is
> also a hot path, as I understand.
> Since the GICv3 architecture explicitly allows caching this information,
> I wanted to use this opportunity.
> 
> Does that make sense?

Yes, I see your reasoning and you are right that it is very important to
keep the hot path very fast. However, I think we should be able to do
that without duplicating information and adding another field to
pending_irq.

If you think that what I suggested is feasible but it's too complex to
do for now, then I would still drop the new lpi_priority field from
pending_irq, implement the slow version based on
vgic_access_guest_memory for now, and add another TODO comment in the
code.

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