[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 13/39] ARM: new VGIC: Add IRQ sync/flush framework
On 27/03/18 20:20, Stefano Stabellini wrote: > On Tue, 27 Mar 2018, Stefano Stabellini wrote: >> On Tue, 27 Mar 2018, Andre Przywara wrote: >>> Hi, >>> >>> On 26/03/18 22:30, Stefano Stabellini wrote: >>>> On Wed, 21 Mar 2018, Andre Przywara wrote: >>>>> Implement the framework for syncing IRQs between our emulation and the >>>>> list registers, which represent the guest's view of IRQs. >>>>> This is done in vgic_sync_from_lrs() and vgic_sync_to_lrs(), which >>>>> get called on guest entry and exit, respectively. >>>>> The code talking to the actual GICv2/v3 hardware is added in the >>>>> following patches. >>>>> >>>>> This is based on Linux commit 0919e84c0fc1, written by Marc Zyngier. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx> >>>> >>>> Just one question below, but the code looks nice >>>> >>>> >>>>> --- >>>>> Changelog v2 ... v3: >>>>> - replace "true" instead of "1" for the boolean parameter >>>>> >>>>> Changelog v1 ... v2: >>>>> - make functions void >>>>> - do underflow setting directly (no v2/v3 indirection) >>>>> - fix multiple SGIs injections (as the late Linux bugfix) >>>>> >>>>> xen/arch/arm/vgic/vgic.c | 232 >>>>> +++++++++++++++++++++++++++++++++++++++++++++++ >>>>> xen/arch/arm/vgic/vgic.h | 2 + >>>>> 2 files changed, 234 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>> index ee0de8d2e0..52e1669888 100644 >>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>> @@ -409,6 +409,238 @@ void vgic_inject_irq(struct domain *d, struct vcpu >>>>> *vcpu, unsigned int intid, >>> >>> .... >>> >>>>> +/* Requires the ap_list_lock to be held. */ >>>>> +static int compute_ap_list_depth(struct vcpu *vcpu) >>>>> +{ >>>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic; >>>>> + struct vgic_irq *irq; >>>>> + int count = 0; >>>>> + >>>>> + ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock)); >>>>> + >>>>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>>>> + { >>>>> + spin_lock(&irq->irq_lock); >>>>> + /* GICv2 SGIs can count for more than one... */ >>>>> + if ( vgic_irq_is_sgi(irq->intid) && irq->source ) >>>>> + count += hweight8(irq->source); >>>> >>>> Why is this done? >>> >>> GICv2 SGIs always have a source CPU ID connected to them. So if two CPUs >>> signal another CPU at the same time, there are *two* distinct SGIs, with >>> two different source IDs. This is an architectural feature of GICv2, so >>> we have to properly emulate this. >>> Despite them being edge triggered IRQs, we cannot coalesce them in this >>> case. >> >> I went through the whole lifecycle of SGIs with the new vgic and it is >> quite different from before, but it makes sense to me now. >> >> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > > Actually, I take it back, one more question :-) > > I understand that every bit set in irq->source corresponds to a > different interrupt that needs to be injected into the guest. They are > distinct interrupts. > > However, compute_ap_list_depth is called to figure out whether the > entries in ap_list overflow the LR registers, and it is never the case > that we write to more than one LR register for a given SGI, even if > irq->source has multiple bit sets, right? Yes, that was actually a recent change in KVM: https://lists.cs.columbia.edu/pipermail/kvmarm/2018-March/030226.html So I basically took this patch right into the series. Now there are more subtleties about priorities (see the follow-ups on this thread), which actually led to a different patch being merged: https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=16ca6a607 (what I only learned today). So this is a bit more sophisticated, and needs some porting because of the new "empty LR" interrupt. I would rather do this on top. > In a concrete example, if we have 3 LR registers and 3 interrupts in > ap_list, one of them is an SGI with multiple irq->source bits, there is > still no need to sort the ap_list, correct? > > I think we should remove the special if statement for sgis in > compute_ap_list_depth. Yes, I believe this is a good intermediate measure, until we get the proper solution. Let me test this tomorrow, then I can push a reworked version of this patch. Cheers, Andre. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |