| Message ID | 20180315203050.19791-10-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi, On 15/03/18 20:30, Andre Przywara wrote: > When playing around with hardware mapped, level triggered virtual IRQs, > there is the need to explicitly set the active or pending state of an > interrupt at some point. > To prepare the GIC for that, we introduce a set_active_state() and a > set_pending_state() function to let the VGIC manipulate the state of > an associated hardware IRQ. > This takes care of properly setting the _IRQ_INPROGRESS bit. after having discussed this with Julien off-line, we can simplify the _IRQ_INPROGRESS setting. See below: > For this it adds gicv2/3_peek_irq() helper functions, to read a bit in a > bitmap spread over several MMIO registers. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog v1 ... v2: > - properly set _IRQ_INPROGRESS bit > - add gicv[23]_peek_irq() (pulled in from later patch) > - move wrappers functions into gic.h > > xen/arch/arm/gic-v2.c | 48 +++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic.h | 24 ++++++++++++++++++++++ > 3 files changed, 124 insertions(+) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 7dfe6fc68d..c6fcbf59d0 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -243,6 +243,52 @@ static void gicv2_poke_irq(struct irq_desc *irqd, uint32_t offset) > writel_gicd(1U << (irqd->irq % 32), offset + (irqd->irq / 32) * 4); > } > > +static bool gicv2_peek_irq(struct irq_desc *irqd, uint32_t offset) > +{ > + uint32_t reg; > + > + reg = readl_gicd(offset + (irqd->irq / 32) * 4) & (1U << (irqd->irq % 32)); > + > + return reg; > +} > + > +static void gicv2_set_active_state(struct irq_desc *irqd, bool active) > +{ > + ASSERT(spin_is_locked(&irqd->lock)); > + > + if ( active ) > + { > + if ( test_bit(_IRQ_GUEST, &irqd->status) ) > + set_bit(_IRQ_INPROGRESS, &irqd->status); > + gicv2_poke_irq(irqd, GICD_ISACTIVER); > + } > + else > + { > + gicv2_poke_irq(irqd, GICD_ICACTIVER); > + if ( !gicv2_peek_irq(irqd, GICD_ISPENDR) && > + test_bit(_IRQ_GUEST, &irqd->status) ) > + clear_bit(_IRQ_INPROGRESS, &irqd->status); The check for the pending bit shouldn't be necessary: - If the interrupt is (still) pending, clearing the active bit should trigger it again. The _IRQ_INPROGRESS bit will be set in turn. - If the interrupt is not pending, we need to clear the bit anyway. So reading the pending state is not necessary, we can always unconditionally clear the _IRQ_INPROGRESS bit, ideally before writing to ICACTIVER. > + } > +} > + > +static void gicv2_set_pending_state(struct irq_desc *irqd, bool pending) > +{ > + ASSERT(spin_is_locked(&irqd->lock)); > + > + if ( pending ) > + { > + /* The INPROGRESS bit will be set when the interrupt fires. */ > + gicv2_poke_irq(irqd, GICD_ISPENDR); > + } > + else > + { > + gicv2_poke_irq(irqd, GICD_ICPENDR); > + if ( !gicv2_peek_irq(irqd, GICD_ISACTIVER) && > + test_bit(_IRQ_GUEST, &irqd->status) ) > + clear_bit(_IRQ_INPROGRESS, &irqd->status); We should not need to touch the _IRQ_INPROGRESS bit here. That bit really shadows the *active* bit, so changing the pending state should not matter here: - If the h/w IRQ is active, the bit is set already and should remain so, as Xen and the guest are still dealing with it. Clearing the h/w pending state does not change that. - If the h/w IRQ is not active, the _IRQ_INPROGRESS bit is not set, so clearing it would be a NOP. So we can remove the _IRQ_INPROGRESS handling here completely. I will amend the code accordingly, including the respective GICv3 parts. Cheers, Andre. > + } > +} > + > static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type) > { > uint32_t cfg, actual, edgebit; > @@ -1277,6 +1323,8 @@ const static struct gic_hw_operations gicv2_ops = { > .eoi_irq = gicv2_eoi_irq, > .deactivate_irq = gicv2_dir_irq, > .read_irq = gicv2_read_irq, > + .set_active_state = gicv2_set_active_state, > + .set_pending_state = gicv2_set_pending_state, > .set_irq_type = gicv2_set_irq_type, > .set_irq_priority = gicv2_set_irq_priority, > .send_SGI = gicv2_send_SGI, > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 392cf91b58..316f2c4142 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -444,6 +444,19 @@ static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp) > gicv3_wait_for_rwp(irqd->irq); > } > > +static bool gicv3_peek_irq(struct irq_desc *irqd, u32 offset) > +{ > + void __iomem *base; > + unsigned int irq = irqd->irq; > + > + if ( irq >= NR_GIC_LOCAL_IRQS) > + base = GICD + (irq / 32) * 4; > + else > + base = GICD_RDIST_SGI_BASE; > + > + return !!(readl(base + offset) & (1U << (irq % 32))); > +} > + > static void gicv3_unmask_irq(struct irq_desc *irqd) > { > gicv3_poke_irq(irqd, GICD_ISENABLER, false); > @@ -477,6 +490,43 @@ static unsigned int gicv3_read_irq(void) > return irq; > } > > +static void gicv3_set_active_state(struct irq_desc *irqd, bool active) > +{ > + ASSERT(spin_is_locked(&irqd->lock)); > + > + if ( active ) > + { > + if ( test_bit(_IRQ_GUEST, &irqd->status) ) > + set_bit(_IRQ_INPROGRESS, &irqd->status); > + gicv3_poke_irq(irqd, GICD_ISACTIVER, false); > + } > + else > + { > + gicv3_poke_irq(irqd, GICD_ICACTIVER, false); > + if ( !gicv3_peek_irq(irqd, GICD_ISPENDR) && > + test_bit(_IRQ_GUEST, &irqd->status) ) > + clear_bit(_IRQ_INPROGRESS, &irqd->status); > + } > +} > + > +static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending) > +{ > + ASSERT(spin_is_locked(&irqd->lock)); > + > + if ( pending ) > + { > + /* The INPROGRESS bit will be set when the interrupt fires. */ > + gicv3_poke_irq(irqd, GICD_ISPENDR, false); > + } > + else > + { > + gicv3_poke_irq(irqd, GICD_ICPENDR, false); > + if ( !gicv3_peek_irq(irqd, GICD_ISACTIVER) && > + test_bit(_IRQ_GUEST, &irqd->status) ) > + clear_bit(_IRQ_INPROGRESS, &irqd->status); > + } > +} > + > static inline uint64_t gicv3_mpidr_to_affinity(int cpu) > { > uint64_t mpidr = cpu_logical_map(cpu); > @@ -1766,6 +1816,8 @@ static const struct gic_hw_operations gicv3_ops = { > .eoi_irq = gicv3_eoi_irq, > .deactivate_irq = gicv3_dir_irq, > .read_irq = gicv3_read_irq, > + .set_active_state = gicv3_set_active_state, > + .set_pending_state = gicv3_set_pending_state, > .set_irq_type = gicv3_set_irq_type, > .set_irq_priority = gicv3_set_irq_priority, > .send_SGI = gicv3_send_sgi, > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 565b0875ca..21cf35f106 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -344,6 +344,10 @@ struct gic_hw_operations { > void (*deactivate_irq)(struct irq_desc *irqd); > /* Read IRQ id and Ack */ > unsigned int (*read_irq)(void); > + /* Force the active state of an IRQ by accessing the distributor */ > + void (*set_active_state)(struct irq_desc *irqd, bool state); > + /* Force the pending state of an IRQ by accessing the distributor */ > + void (*set_pending_state)(struct irq_desc *irqd, bool state); > /* Set IRQ type */ > void (*set_irq_type)(struct irq_desc *desc, unsigned int type); > /* Set IRQ priority */ > @@ -392,6 +396,26 @@ static inline unsigned int gic_get_nr_lrs(void) > return gic_hw_ops->info->nr_lrs; > } > > +/* > + * Set the active state of an IRQ. This should be used with care, as this > + * directly forces the active bit, without considering the GIC state machine. > + * For private IRQs this only works for those of the current CPU. > + */ > +static inline void gic_set_active_state(struct irq_desc *irqd, bool state) > +{ > + gic_hw_ops->set_active_state(irqd, state); > +} > + > +/* > + * Set the pending state of an IRQ. This should be used with care, as this > + * directly forces the pending bit, without considering the GIC state machine. > + * For private IRQs this only works for those of the current CPU. > + */ > +static inline void gic_set_pending_state(struct irq_desc *irqd, bool state) > +{ > + gic_hw_ops->set_pending_state(irqd, state); > +} > + > void register_gic_ops(const struct gic_hw_operations *ops); > int gic_make_hwdom_dt_node(const struct domain *d, > const struct dt_device_node *gic, >
On 03/16/2018 04:05 PM, Andre Przywara wrote: > Hi, Hi Andre, > On 15/03/18 20:30, Andre Przywara wrote: >> + } >> +} >> + >> +static void gicv2_set_pending_state(struct irq_desc *irqd, bool pending) >> +{ >> + ASSERT(spin_is_locked(&irqd->lock)); >> + >> + if ( pending ) >> + { >> + /* The INPROGRESS bit will be set when the interrupt fires. */ >> + gicv2_poke_irq(irqd, GICD_ISPENDR); >> + } >> + else >> + { >> + gicv2_poke_irq(irqd, GICD_ICPENDR); >> + if ( !gicv2_peek_irq(irqd, GICD_ISACTIVER) && >> + test_bit(_IRQ_GUEST, &irqd->status) ) >> + clear_bit(_IRQ_INPROGRESS, &irqd->status); > > We should not need to touch the _IRQ_INPROGRESS bit here. That bit > really shadows the *active* bit, so changing the pending state should > not matter here: > - If the h/w IRQ is active, the bit is set already and should remain so, > as Xen and the guest are still dealing with it. Clearing the h/w pending > state does not change that. > - If the h/w IRQ is not active, the _IRQ_INPROGRESS bit is not set, so > clearing it would be a NOP. > So we can remove the _IRQ_INPROGRESS handling here completely. > > I will amend the code accordingly, including the respective GICv3 parts. Thank you for summarizing our discussion. Is it still making sense to document how those helpers should be called? Cheers,
Hi, On 19/03/18 09:30, Julien Grall wrote: > > > On 03/16/2018 04:05 PM, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 15/03/18 20:30, Andre Przywara wrote: >>> + } >>> +} >>> + >>> +static void gicv2_set_pending_state(struct irq_desc *irqd, bool >>> pending) >>> +{ >>> + ASSERT(spin_is_locked(&irqd->lock)); >>> + >>> + if ( pending ) >>> + { >>> + /* The INPROGRESS bit will be set when the interrupt fires. */ >>> + gicv2_poke_irq(irqd, GICD_ISPENDR); >>> + } >>> + else >>> + { >>> + gicv2_poke_irq(irqd, GICD_ICPENDR); >>> + if ( !gicv2_peek_irq(irqd, GICD_ISACTIVER) && >>> + test_bit(_IRQ_GUEST, &irqd->status) ) >>> + clear_bit(_IRQ_INPROGRESS, &irqd->status); >> >> We should not need to touch the _IRQ_INPROGRESS bit here. That bit >> really shadows the *active* bit, so changing the pending state should >> not matter here: >> - If the h/w IRQ is active, the bit is set already and should remain so, >> as Xen and the guest are still dealing with it. Clearing the h/w pending >> state does not change that. >> - If the h/w IRQ is not active, the _IRQ_INPROGRESS bit is not set, so >> clearing it would be a NOP. >> So we can remove the _IRQ_INPROGRESS handling here completely. >> >> I will amend the code accordingly, including the respective GICv3 parts. > > Thank you for summarizing our discussion. Is it still making sense to > document how those helpers should be called? Which helpers? The set_{pending,active}_state() functions? I already put some kind of warning before the (wrapper) prototypes: /* * Set the active state of an IRQ. This should be used with care, as * this directly forces the active bit, without considering the GIC * state machine. * For private IRQs this only works for those of the current CPU. */ Cheers, Andre.
Hi Andre, On 03/19/2018 05:54 PM, Andre Przywara wrote: > Which helpers? The set_{pending,active}_state() functions? I already put > some kind of warning before the (wrapper) prototypes: > /* > * Set the active state of an IRQ. This should be used with care, as > * this directly forces the active bit, without considering the GIC > * state machine. > * For private IRQs this only works for those of the current CPU. > */ Oh yes, I missed that. Sorry for the noise. Cheers,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 7dfe6fc68d..c6fcbf59d0 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -243,6 +243,52 @@ static void gicv2_poke_irq(struct irq_desc *irqd, uint32_t offset) writel_gicd(1U << (irqd->irq % 32), offset + (irqd->irq / 32) * 4); } +static bool gicv2_peek_irq(struct irq_desc *irqd, uint32_t offset) +{ + uint32_t reg; + + reg = readl_gicd(offset + (irqd->irq / 32) * 4) & (1U << (irqd->irq % 32)); + + return reg; +} + +static void gicv2_set_active_state(struct irq_desc *irqd, bool active) +{ + ASSERT(spin_is_locked(&irqd->lock)); + + if ( active ) + { + if ( test_bit(_IRQ_GUEST, &irqd->status) ) + set_bit(_IRQ_INPROGRESS, &irqd->status); + gicv2_poke_irq(irqd, GICD_ISACTIVER); + } + else + { + gicv2_poke_irq(irqd, GICD_ICACTIVER); + if ( !gicv2_peek_irq(irqd, GICD_ISPENDR) && + test_bit(_IRQ_GUEST, &irqd->status) ) + clear_bit(_IRQ_INPROGRESS, &irqd->status); + } +} + +static void gicv2_set_pending_state(struct irq_desc *irqd, bool pending) +{ + ASSERT(spin_is_locked(&irqd->lock)); + + if ( pending ) + { + /* The INPROGRESS bit will be set when the interrupt fires. */ + gicv2_poke_irq(irqd, GICD_ISPENDR); + } + else + { + gicv2_poke_irq(irqd, GICD_ICPENDR); + if ( !gicv2_peek_irq(irqd, GICD_ISACTIVER) && + test_bit(_IRQ_GUEST, &irqd->status) ) + clear_bit(_IRQ_INPROGRESS, &irqd->status); + } +} + static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type) { uint32_t cfg, actual, edgebit; @@ -1277,6 +1323,8 @@ const static struct gic_hw_operations gicv2_ops = { .eoi_irq = gicv2_eoi_irq, .deactivate_irq = gicv2_dir_irq, .read_irq = gicv2_read_irq, + .set_active_state = gicv2_set_active_state, + .set_pending_state = gicv2_set_pending_state, .set_irq_type = gicv2_set_irq_type, .set_irq_priority = gicv2_set_irq_priority, .send_SGI = gicv2_send_SGI, diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 392cf91b58..316f2c4142 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -444,6 +444,19 @@ static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp) gicv3_wait_for_rwp(irqd->irq); } +static bool gicv3_peek_irq(struct irq_desc *irqd, u32 offset) +{ + void __iomem *base; + unsigned int irq = irqd->irq; + + if ( irq >= NR_GIC_LOCAL_IRQS) + base = GICD + (irq / 32) * 4; + else + base = GICD_RDIST_SGI_BASE; + + return !!(readl(base + offset) & (1U << (irq % 32))); +} + static void gicv3_unmask_irq(struct irq_desc *irqd) { gicv3_poke_irq(irqd, GICD_ISENABLER, false); @@ -477,6 +490,43 @@ static unsigned int gicv3_read_irq(void) return irq; } +static void gicv3_set_active_state(struct irq_desc *irqd, bool active) +{ + ASSERT(spin_is_locked(&irqd->lock)); + + if ( active ) + { + if ( test_bit(_IRQ_GUEST, &irqd->status) ) + set_bit(_IRQ_INPROGRESS, &irqd->status); + gicv3_poke_irq(irqd, GICD_ISACTIVER, false); + } + else + { + gicv3_poke_irq(irqd, GICD_ICACTIVER, false); + if ( !gicv3_peek_irq(irqd, GICD_ISPENDR) && + test_bit(_IRQ_GUEST, &irqd->status) ) + clear_bit(_IRQ_INPROGRESS, &irqd->status); + } +} + +static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending) +{ + ASSERT(spin_is_locked(&irqd->lock)); + + if ( pending ) + { + /* The INPROGRESS bit will be set when the interrupt fires. */ + gicv3_poke_irq(irqd, GICD_ISPENDR, false); + } + else + { + gicv3_poke_irq(irqd, GICD_ICPENDR, false); + if ( !gicv3_peek_irq(irqd, GICD_ISACTIVER) && + test_bit(_IRQ_GUEST, &irqd->status) ) + clear_bit(_IRQ_INPROGRESS, &irqd->status); + } +} + static inline uint64_t gicv3_mpidr_to_affinity(int cpu) { uint64_t mpidr = cpu_logical_map(cpu); @@ -1766,6 +1816,8 @@ static const struct gic_hw_operations gicv3_ops = { .eoi_irq = gicv3_eoi_irq, .deactivate_irq = gicv3_dir_irq, .read_irq = gicv3_read_irq, + .set_active_state = gicv3_set_active_state, + .set_pending_state = gicv3_set_pending_state, .set_irq_type = gicv3_set_irq_type, .set_irq_priority = gicv3_set_irq_priority, .send_SGI = gicv3_send_sgi, diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 565b0875ca..21cf35f106 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -344,6 +344,10 @@ struct gic_hw_operations { void (*deactivate_irq)(struct irq_desc *irqd); /* Read IRQ id and Ack */ unsigned int (*read_irq)(void); + /* Force the active state of an IRQ by accessing the distributor */ + void (*set_active_state)(struct irq_desc *irqd, bool state); + /* Force the pending state of an IRQ by accessing the distributor */ + void (*set_pending_state)(struct irq_desc *irqd, bool state); /* Set IRQ type */ void (*set_irq_type)(struct irq_desc *desc, unsigned int type); /* Set IRQ priority */ @@ -392,6 +396,26 @@ static inline unsigned int gic_get_nr_lrs(void) return gic_hw_ops->info->nr_lrs; } +/* + * Set the active state of an IRQ. This should be used with care, as this + * directly forces the active bit, without considering the GIC state machine. + * For private IRQs this only works for those of the current CPU. + */ +static inline void gic_set_active_state(struct irq_desc *irqd, bool state) +{ + gic_hw_ops->set_active_state(irqd, state); +} + +/* + * Set the pending state of an IRQ. This should be used with care, as this + * directly forces the pending bit, without considering the GIC state machine. + * For private IRQs this only works for those of the current CPU. + */ +static inline void gic_set_pending_state(struct irq_desc *irqd, bool state) +{ + gic_hw_ops->set_pending_state(irqd, state); +} + void register_gic_ops(const struct gic_hw_operations *ops); int gic_make_hwdom_dt_node(const struct domain *d, const struct dt_device_node *gic,
When playing around with hardware mapped, level triggered virtual IRQs, there is the need to explicitly set the active or pending state of an interrupt at some point. To prepare the GIC for that, we introduce a set_active_state() and a set_pending_state() function to let the VGIC manipulate the state of an associated hardware IRQ. This takes care of properly setting the _IRQ_INPROGRESS bit. For this it adds gicv2/3_peek_irq() helper functions, to read a bit in a bitmap spread over several MMIO registers. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog v1 ... v2: - properly set _IRQ_INPROGRESS bit - add gicv[23]_peek_irq() (pulled in from later patch) - move wrappers functions into gic.h xen/arch/arm/gic-v2.c | 48 +++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/gic-v3.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/gic.h | 24 ++++++++++++++++++++++ 3 files changed, 124 insertions(+)