[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 13/30] ARM: GICv3: forward pending LPIs to guests
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |