| Message ID | 1372115067-17071-4-git-send-email-julien.grall@citrix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 25 Jun 2013, Julien Grall wrote: > From: Julien Grall <julien.grall@linaro.org> > > When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: > - Disable the IRQ > - Call the interrupt handler > - Conditionnally enable the IRQ > - EOI the IRQ > > When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's > still inflight. Therefore, LRs will contains duplicated IRQs and Xen will > EOI it twice if it's a physical IRQ. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ > xen/arch/arm/vgic.c | 3 ++- > xen/include/asm-arm/gic.h | 3 +++ > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 21575df..bf05716 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > return rc; > } > > +/* Check if an IRQ was already injected to the current VCPU */ > +bool_t gic_irq_injected(unsigned int irq) Can you rename it to something more specific, like gic_irq_inlr? > +{ > + bool_t found = 0; > + int i = 0; > + unsigned int virq; > + > + spin_lock_irq(&gic.lock); > + > + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), > + nr_lrs, i)) < nr_lrs ) > + { > + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; > + > + if ( virq == irq ) > + { > + found = 1; > + break; > + } > + i++; > + } Instead of reading back all the GICH_LR registers, can't just just read the ones that have a corresponding bit set in lr_mask? Also you should be able to avoid having to read the GICH_LR registers by simply checking if the irq is in the lr_queue list: if an irq is in inflight but not in lr_queue, it means that it is in one of the LRs. > + spin_unlock_irq(&gic.lock); > + > + return found; > +} > + > static inline void gic_set_lr(int lr, unsigned int virtual_irq, > unsigned int state, unsigned int priority) > { > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 2d91dce..cea9233 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -369,8 +369,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) { > irq = i + (32 * n); > p = irq_to_pending(v, irq); > - if ( !list_empty(&p->inflight) ) > + if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) > gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); > + > i++; > } > } > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 513c1fc..f9e9ef1 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -197,6 +197,9 @@ extern unsigned int gic_number_lines(void); > int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > unsigned int *out_hwirq, unsigned int *out_type); > > +/* Check if an IRQ was already injected to the current VCPU */ > +bool_t gic_irq_injected(unsigned int irq); > + > #endif /* __ASSEMBLY__ */ > #endif > > -- > 1.7.10.4 >
On 06/25/2013 02:24 PM, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Julien Grall wrote: >> From: Julien Grall <julien.grall@linaro.org> >> >> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: >> - Disable the IRQ >> - Call the interrupt handler >> - Conditionnally enable the IRQ >> - EOI the IRQ >> >> When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's >> still inflight. Therefore, LRs will contains duplicated IRQs and Xen will >> EOI it twice if it's a physical IRQ. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ >> xen/arch/arm/vgic.c | 3 ++- >> xen/include/asm-arm/gic.h | 3 +++ >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 21575df..bf05716 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> return rc; >> } >> >> +/* Check if an IRQ was already injected to the current VCPU */ >> +bool_t gic_irq_injected(unsigned int irq) > > Can you rename it to something more specific, like gic_irq_inlr? > >> +{ >> + bool_t found = 0; >> + int i = 0; >> + unsigned int virq; >> + >> + spin_lock_irq(&gic.lock); >> + >> + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), >> + nr_lrs, i)) < nr_lrs ) >> + { >> + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; >> + >> + if ( virq == irq ) >> + { >> + found = 1; >> + break; >> + } >> + i++; >> + } > > Instead of reading back all the GICH_LR registers, can't just just read > the ones that have a corresponding bit set in lr_mask? It's already the case, I use find_next_bit to find the next used LRs. > Also you should be able to avoid having to read the GICH_LR registers by > simply checking if the irq is in the lr_queue list: if an irq is in > inflight but not in lr_queue, it means that it is in one of the LRs. No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight but not in lr_queue. We don't have a way to know whether the IRQ is really in LRs or not.
On Tue, 2013-06-25 at 14:24 +0100, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Julien Grall wrote: > > From: Julien Grall <julien.grall@linaro.org> > > > > When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: > > - Disable the IRQ > > - Call the interrupt handler > > - Conditionnally enable the IRQ > > - EOI the IRQ > > > > When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's > > still inflight. Therefore, LRs will contains duplicated IRQs and Xen will > > EOI it twice if it's a physical IRQ. > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > > xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ > > xen/arch/arm/vgic.c | 3 ++- > > xen/include/asm-arm/gic.h | 3 +++ > > 3 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index 21575df..bf05716 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > return rc; > > } > > > > +/* Check if an IRQ was already injected to the current VCPU */ > > +bool_t gic_irq_injected(unsigned int irq) > > Can you rename it to something more specific, like gic_irq_inlr? > > > +{ > > + bool_t found = 0; > > + int i = 0; > > + unsigned int virq; > > + > > + spin_lock_irq(&gic.lock); > > + > > + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), > > + nr_lrs, i)) < nr_lrs ) > > + { > > + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; > > + > > + if ( virq == irq ) > > + { > > + found = 1; > > + break; > > + } > > + i++; > > + } > > Instead of reading back all the GICH_LR registers, can't just just read > the ones that have a corresponding bit set in lr_mask? > > Also you should be able to avoid having to read the GICH_LR registers by > simply checking if the irq is in the lr_queue list: if an irq is in > inflight but not in lr_queue, it means that it is in one of the LRs. This sounds roughly equivalent to what I was about to suggest which was a bool in the irq descriptor. In any case we should certainly try and avoid walking all the LRs via some means. Ian.
On Tue, 25 Jun 2013, Julien Grall wrote: > On 06/25/2013 02:24 PM, Stefano Stabellini wrote: > > > On Tue, 25 Jun 2013, Julien Grall wrote: > >> From: Julien Grall <julien.grall@linaro.org> > >> > >> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: > >> - Disable the IRQ > >> - Call the interrupt handler > >> - Conditionnally enable the IRQ > >> - EOI the IRQ > >> > >> When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's > >> still inflight. Therefore, LRs will contains duplicated IRQs and Xen will > >> EOI it twice if it's a physical IRQ. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ > >> xen/arch/arm/vgic.c | 3 ++- > >> xen/include/asm-arm/gic.h | 3 +++ > >> 3 files changed, 32 insertions(+), 1 deletion(-) > >> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index 21575df..bf05716 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > >> return rc; > >> } > >> > >> +/* Check if an IRQ was already injected to the current VCPU */ > >> +bool_t gic_irq_injected(unsigned int irq) > > > > Can you rename it to something more specific, like gic_irq_inlr? > > > >> +{ > >> + bool_t found = 0; > >> + int i = 0; > >> + unsigned int virq; > >> + > >> + spin_lock_irq(&gic.lock); > >> + > >> + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), > >> + nr_lrs, i)) < nr_lrs ) > >> + { > >> + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; > >> + > >> + if ( virq == irq ) > >> + { > >> + found = 1; > >> + break; > >> + } > >> + i++; > >> + } > > > > Instead of reading back all the GICH_LR registers, can't just just read > > the ones that have a corresponding bit set in lr_mask? > > It's already the case, I use find_next_bit to find the next used LRs. > > > Also you should be able to avoid having to read the GICH_LR registers by > > simply checking if the irq is in the lr_queue list: if an irq is in > > inflight but not in lr_queue, it means that it is in one of the LRs. > > > No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight > but not in lr_queue. We don't have a way to know whether the IRQ is > really in LRs or not. I think it's time we introduce a "status" member in struct irq_desc, so that we are not dependent on the information in the GICH_LR registers or the queue a pending_irq has been added to. I would implement it as a bitfield: int status; #define GIC_IRQ_ENABLED (1<<0) #define GIC_IRQ_INFLIGHT (1<<1) #define GIC_IRQ_INLR (1<<2) This way you should just go through the inflight queue and check whether status & GIC_IRQ_INLR. At the moment we just want to represent this basic state machine: irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt)
On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote: > I think it's time we introduce a "status" member in struct irq_desc, so > that we are not dependent on the information in the GICH_LR registers or > the queue a pending_irq has been added to. Yes please, I find this one of the hardest things to keep straight in my head (not helped by my inability to remember which of pending and inflight is which...) > I would implement it as a bitfield: > > int status; > #define GIC_IRQ_ENABLED (1<<0) > #define GIC_IRQ_INFLIGHT (1<<1) > #define GIC_IRQ_INLR (1<<2) > > This way you should just go through the inflight queue and check whether > status & GIC_IRQ_INLR. Since some of this stuff happens in interrupt context you probably want test_bit/set_bit et al rather than regular boolean logic, don't you? > At the moment we just want to represent this basic state machine: > > irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt) Can we model the states after the active/pending states which the gic has? It might make a bunch of stuff clearer? Ian.
On 06/25/2013 05:36 PM, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Julien Grall wrote: >> On 06/25/2013 02:24 PM, Stefano Stabellini wrote: >> >>> On Tue, 25 Jun 2013, Julien Grall wrote: >>>> From: Julien Grall <julien.grall@linaro.org> >>>> >>>> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: >>>> - Disable the IRQ >>>> - Call the interrupt handler >>>> - Conditionnally enable the IRQ >>>> - EOI the IRQ >>>> >>>> When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's >>>> still inflight. Therefore, LRs will contains duplicated IRQs and Xen will >>>> EOI it twice if it's a physical IRQ. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> --- >>>> xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ >>>> xen/arch/arm/vgic.c | 3 ++- >>>> xen/include/asm-arm/gic.h | 3 +++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>> index 21575df..bf05716 100644 >>>> --- a/xen/arch/arm/gic.c >>>> +++ b/xen/arch/arm/gic.c >>>> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >>>> return rc; >>>> } >>>> >>>> +/* Check if an IRQ was already injected to the current VCPU */ >>>> +bool_t gic_irq_injected(unsigned int irq) >>> >>> Can you rename it to something more specific, like gic_irq_inlr? >>> >>>> +{ >>>> + bool_t found = 0; >>>> + int i = 0; >>>> + unsigned int virq; >>>> + >>>> + spin_lock_irq(&gic.lock); >>>> + >>>> + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), >>>> + nr_lrs, i)) < nr_lrs ) >>>> + { >>>> + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; >>>> + >>>> + if ( virq == irq ) >>>> + { >>>> + found = 1; >>>> + break; >>>> + } >>>> + i++; >>>> + } >>> >>> Instead of reading back all the GICH_LR registers, can't just just read >>> the ones that have a corresponding bit set in lr_mask? >> >> It's already the case, I use find_next_bit to find the next used LRs. >> >>> Also you should be able to avoid having to read the GICH_LR registers by >>> simply checking if the irq is in the lr_queue list: if an irq is in >>> inflight but not in lr_queue, it means that it is in one of the LRs. >> >> >> No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight >> but not in lr_queue. We don't have a way to know whether the IRQ is >> really in LRs or not. > > I think it's time we introduce a "status" member in struct irq_desc, so Do you mean struct irq_pending? > that we are not dependent on the information in the GICH_LR registers or > the queue a pending_irq has been added to. > I would implement it as a bitfield: > > int status; > #define GIC_IRQ_ENABLED (1<<0) > #define GIC_IRQ_INFLIGHT (1<<1) > #define GIC_IRQ_INLR (1<<2) > > This way you should just go through the inflight queue and check whether > status & GIC_IRQ_INLR. > > At the moment we just want to represent this basic state machine: > > irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt) Sounds good to me. I will try to implement this approach. Moreover, it will fix another issue with this patch. I have just noticed that it's possible to reinject an IRQ on different vCPU even if it's injected on another vCPU.
On Tue, 25 Jun 2013, Julien Grall wrote: > On 06/25/2013 05:36 PM, Stefano Stabellini wrote: > > > On Tue, 25 Jun 2013, Julien Grall wrote: > >> On 06/25/2013 02:24 PM, Stefano Stabellini wrote: > >> > >>> On Tue, 25 Jun 2013, Julien Grall wrote: > >>>> From: Julien Grall <julien.grall@linaro.org> > >>>> > >>>> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will: > >>>> - Disable the IRQ > >>>> - Call the interrupt handler > >>>> - Conditionnally enable the IRQ > >>>> - EOI the IRQ > >>>> > >>>> When Linux will re-enable the IRQ, Xen will inject again the IRQ because it's > >>>> still inflight. Therefore, LRs will contains duplicated IRQs and Xen will > >>>> EOI it twice if it's a physical IRQ. > >>>> > >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >>>> --- > >>>> xen/arch/arm/gic.c | 27 +++++++++++++++++++++++++++ > >>>> xen/arch/arm/vgic.c | 3 ++- > >>>> xen/include/asm-arm/gic.h | 3 +++ > >>>> 3 files changed, 32 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >>>> index 21575df..bf05716 100644 > >>>> --- a/xen/arch/arm/gic.c > >>>> +++ b/xen/arch/arm/gic.c > >>>> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > >>>> return rc; > >>>> } > >>>> > >>>> +/* Check if an IRQ was already injected to the current VCPU */ > >>>> +bool_t gic_irq_injected(unsigned int irq) > >>> > >>> Can you rename it to something more specific, like gic_irq_inlr? > >>> > >>>> +{ > >>>> + bool_t found = 0; > >>>> + int i = 0; > >>>> + unsigned int virq; > >>>> + > >>>> + spin_lock_irq(&gic.lock); > >>>> + > >>>> + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), > >>>> + nr_lrs, i)) < nr_lrs ) > >>>> + { > >>>> + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; > >>>> + > >>>> + if ( virq == irq ) > >>>> + { > >>>> + found = 1; > >>>> + break; > >>>> + } > >>>> + i++; > >>>> + } > >>> > >>> Instead of reading back all the GICH_LR registers, can't just just read > >>> the ones that have a corresponding bit set in lr_mask? > >> > >> It's already the case, I use find_next_bit to find the next used LRs. > >> > >>> Also you should be able to avoid having to read the GICH_LR registers by > >>> simply checking if the irq is in the lr_queue list: if an irq is in > >>> inflight but not in lr_queue, it means that it is in one of the LRs. > >> > >> > >> No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight > >> but not in lr_queue. We don't have a way to know whether the IRQ is > >> really in LRs or not. > > > > I think it's time we introduce a "status" member in struct irq_desc, so > > Do you mean struct irq_pending? Yes, sorry I meant struct irq_pending
On Tue, 25 Jun 2013, Ian Campbell wrote: > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote: > > I think it's time we introduce a "status" member in struct irq_desc, so > > that we are not dependent on the information in the GICH_LR registers or > > the queue a pending_irq has been added to. > > Yes please, I find this one of the hardest things to keep straight in my > head (not helped by my inability to remember which of pending and > inflight is which...) > > > I would implement it as a bitfield: > > > > int status; > > #define GIC_IRQ_ENABLED (1<<0) > > #define GIC_IRQ_INFLIGHT (1<<1) > > #define GIC_IRQ_INLR (1<<2) > > > > This way you should just go through the inflight queue and check whether > > status & GIC_IRQ_INLR. > > Since some of this stuff happens in interrupt context you probably want > test_bit/set_bit et al rather than regular boolean logic, don't you? > > > At the moment we just want to represent this basic state machine: > > > > irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt) > > Can we model the states after the active/pending states which the gic > has? It might make a bunch of stuff clearer? It might be worth storing those too. So maybe: #define GIC_IRQ_ENABLED (1<<0) #define GIC_IRQ_PENDING (1<<1) #define GIC_IRQ_ACTIVE (1<<2) #define GIC_IRQ_GUEST_INFLIGHT (1<<3) #define GIC_IRQ_GUEST_INLR (1<<4) however if we do store the physical gic states (GIC_IRQ_PENDING and GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc rather than irq_pending that is used just for guest irqs.
On Tue, 2013-06-25 at 18:05 +0100, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Ian Campbell wrote: > > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote: > > > I think it's time we introduce a "status" member in struct irq_desc, so > > > that we are not dependent on the information in the GICH_LR registers or > > > the queue a pending_irq has been added to. > > > > Yes please, I find this one of the hardest things to keep straight in my > > head (not helped by my inability to remember which of pending and > > inflight is which...) > > > > > I would implement it as a bitfield: > > > > > > int status; > > > #define GIC_IRQ_ENABLED (1<<0) > > > #define GIC_IRQ_INFLIGHT (1<<1) > > > #define GIC_IRQ_INLR (1<<2) > > > > > > This way you should just go through the inflight queue and check whether > > > status & GIC_IRQ_INLR. > > > > Since some of this stuff happens in interrupt context you probably want > > test_bit/set_bit et al rather than regular boolean logic, don't you? > > > > > At the moment we just want to represent this basic state machine: > > > > > > irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt) > > > > Can we model the states after the active/pending states which the gic > > has? It might make a bunch of stuff clearer? > > It might be worth storing those too. > So maybe: > > #define GIC_IRQ_ENABLED (1<<0) > #define GIC_IRQ_PENDING (1<<1) > #define GIC_IRQ_ACTIVE (1<<2) > #define GIC_IRQ_GUEST_INFLIGHT (1<<3) > #define GIC_IRQ_GUEST_INLR (1<<4) > > however if we do store the physical gic states (GIC_IRQ_PENDING and > GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc > rather than irq_pending that is used just for guest irqs. I was thinking of these as states of the emulated interrupts, rather than any underlying physical interrupts, so I think irq_pending is correct? It occurs to me that at least some of these bits are also fields in the LR. I think it is good to save them separately (reducing the intertwining of our interrupt handling from GIC internals is a good thing) but it means you will need to take care of syncing the state between the LR and our internal state at various points, since the LR can change based on the guest EOI etc. Ian.
On Wed, 26 Jun 2013, Ian Campbell wrote: > On Tue, 2013-06-25 at 18:05 +0100, Stefano Stabellini wrote: > > On Tue, 25 Jun 2013, Ian Campbell wrote: > > > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote: > > > > I think it's time we introduce a "status" member in struct irq_desc, so > > > > that we are not dependent on the information in the GICH_LR registers or > > > > the queue a pending_irq has been added to. > > > > > > Yes please, I find this one of the hardest things to keep straight in my > > > head (not helped by my inability to remember which of pending and > > > inflight is which...) > > > > > > > I would implement it as a bitfield: > > > > > > > > int status; > > > > #define GIC_IRQ_ENABLED (1<<0) > > > > #define GIC_IRQ_INFLIGHT (1<<1) > > > > #define GIC_IRQ_INLR (1<<2) > > > > > > > > This way you should just go through the inflight queue and check whether > > > > status & GIC_IRQ_INLR. > > > > > > Since some of this stuff happens in interrupt context you probably want > > > test_bit/set_bit et al rather than regular boolean logic, don't you? > > > > > > > At the moment we just want to represent this basic state machine: > > > > > > > > irq guest asserted -> inflight -> injected (inlr) -> unasserted (maintenance interrupt) > > > > > > Can we model the states after the active/pending states which the gic > > > has? It might make a bunch of stuff clearer? > > > > It might be worth storing those too. > > So maybe: > > > > #define GIC_IRQ_ENABLED (1<<0) > > #define GIC_IRQ_PENDING (1<<1) > > #define GIC_IRQ_ACTIVE (1<<2) > > #define GIC_IRQ_GUEST_INFLIGHT (1<<3) > > #define GIC_IRQ_GUEST_INLR (1<<4) > > > > however if we do store the physical gic states (GIC_IRQ_PENDING and > > GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc > > rather than irq_pending that is used just for guest irqs. > > I was thinking of these as states of the emulated interrupts, rather > than any underlying physical interrupts, so I think irq_pending is > correct? In that case yes > It occurs to me that at least some of these bits are also fields in the > LR. I think it is good to save them separately (reducing the > intertwining of our interrupt handling from GIC internals is a good > thing) but it means you will need to take care of syncing the state > between the LR and our internal state at various points, since the LR > can change based on the guest EOI etc. I don't think we can accurately distinguish between pending and active states for guest irqs, because the transition between the two happens transparently from Xen's point of view. The only thing we can do is update the state in irq_pending when we save and restore the GICH_LR registers. That might still be useful because it would allow us to know which ones of the irqs currently in the LRs registers can be temporarily set aside (for example to implement priorities).
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 21575df..bf05716 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) return rc; } +/* Check if an IRQ was already injected to the current VCPU */ +bool_t gic_irq_injected(unsigned int irq) +{ + bool_t found = 0; + int i = 0; + unsigned int virq; + + spin_lock_irq(&gic.lock); + + while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask), + nr_lrs, i)) < nr_lrs ) + { + virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK; + + if ( virq == irq ) + { + found = 1; + break; + } + i++; + } + + spin_unlock_irq(&gic.lock); + + return found; +} + static inline void gic_set_lr(int lr, unsigned int virtual_irq, unsigned int state, unsigned int priority) { diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 2d91dce..cea9233 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -369,8 +369,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) { irq = i + (32 * n); p = irq_to_pending(v, irq); - if ( !list_empty(&p->inflight) ) + if ( !list_empty(&p->inflight) && !gic_irq_injected(irq) ) gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority); + i++; } } diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 513c1fc..f9e9ef1 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -197,6 +197,9 @@ extern unsigned int gic_number_lines(void); int gic_irq_xlate(const u32 *intspec, unsigned int intsize, unsigned int *out_hwirq, unsigned int *out_type); +/* Check if an IRQ was already injected to the current VCPU */ +bool_t gic_irq_injected(unsigned int irq); + #endif /* __ASSEMBLY__ */ #endif