| Message ID | 20180206170903.30637-6-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: VGIC/GIC separation cleanups | expand |
Hi Andre, On 02/06/2018 05:09 PM, Andre Przywara wrote: > At the moment we happily access VGIC internal data structures like > the rank and struct pending_irq in gic.c, which should be VGIC agnostic. > > Factor out a new function vgic_connect_hw_irq(), which allows a virtual > IRQ to be connected to a hardware IRQ (using the hw bit in the LR). > > This removes said accesses to VGIC data structures and improves abstraction. > > One thing to note is that this changes the locking scheme slightly: > we hold the rank lock for a shorter period of time, not covering some > of the later lines, which deal with the "irq_desc" structure only. This > should not have any adverse effect, but is a change in locking anyway. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> Thank you for the quick respin! Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers, > --- > xen/arch/arm/gic-vgic.c | 41 +++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic.c | 44 ++++++++++---------------------------------- > xen/include/asm-arm/vgic.h | 2 ++ > 3 files changed, 53 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c > index 8221ae557c..820e464fc0 100644 > --- a/xen/arch/arm/gic-vgic.c > +++ b/xen/arch/arm/gic-vgic.c > @@ -397,6 +397,47 @@ void gic_dump_vgic_info(struct vcpu *v) > printk("Pending irq=%d\n", p->irq); > } > > +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq, > + struct irq_desc *desc, bool connect) > +{ > + unsigned long flags; > + /* > + * Use vcpu0 to retrieve the pending_irq struct. Given that we only > + * route SPIs to guests, it doesn't make any difference. > + */ > + struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); > + struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); > + struct pending_irq *p = irq_to_pending(v_target, virq); > + int ret = 0; > + > + /* "desc" is optional when we disconnect an IRQ. */ > + ASSERT(connect && desc); > + > + /* We are taking to rank lock to prevent parallel connections. */ > + vgic_lock_rank(v_target, rank, flags); > + > + if ( connect ) > + { > + /* The VIRQ should not be already enabled by the guest */ > + if ( !p->desc && > + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) > + p->desc = desc; > + else > + ret = -EBUSY; > + } > + else > + { > + if ( desc && p->desc != desc ) > + ret = -EINVAL; > + else > + p->desc = NULL; > + } > + > + vgic_unlock_rank(v_target, rank, flags); > + > + return ret; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 4cb74d449e..968e46fabb 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -128,13 +128,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority) > int gic_route_irq_to_guest(struct domain *d, unsigned int virq, > struct irq_desc *desc, unsigned int priority) > { > - unsigned long flags; > - /* Use vcpu0 to retrieve the pending_irq struct. Given that we only > - * route SPIs to guests, it doesn't make any difference. */ > - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); > - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); > - struct pending_irq *p = irq_to_pending(v_target, virq); > - int res = -EBUSY; > + int ret; > > ASSERT(spin_is_locked(&desc->lock)); > /* Caller has already checked that the IRQ is an SPI */ > @@ -142,12 +136,9 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, > ASSERT(virq < vgic_num_irqs(d)); > ASSERT(!is_lpi(virq)); > > - vgic_lock_rank(v_target, rank, flags); > - > - if ( p->desc || > - /* The VIRQ should not be already enabled by the guest */ > - test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) > - goto out; > + ret = vgic_connect_hw_irq(d, NULL, virq, desc, true); > + if ( ret ) > + return ret; > > desc->handler = gic_hw_ops->gic_guest_irq_type; > set_bit(_IRQ_GUEST, &desc->status); > @@ -156,31 +147,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, > gic_set_irq_type(desc, desc->arch.type); > gic_set_irq_priority(desc, priority); > > - p->desc = desc; > - res = 0; > - > -out: > - vgic_unlock_rank(v_target, rank, flags); > - > - return res; > + return 0; > } > > /* This function only works with SPIs for now */ > int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, > struct irq_desc *desc) > { > - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); > - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); > - struct pending_irq *p = irq_to_pending(v_target, virq); > - unsigned long flags; > + int ret; > > ASSERT(spin_is_locked(&desc->lock)); > ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > - ASSERT(p->desc == desc); > ASSERT(!is_lpi(virq)); > > - vgic_lock_rank(v_target, rank, flags); > - > if ( d->is_dying ) > { > desc->handler->shutdown(desc); > @@ -198,19 +177,16 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, > */ > if ( test_bit(_IRQ_INPROGRESS, &desc->status) || > !test_bit(_IRQ_DISABLED, &desc->status) ) > - { > - vgic_unlock_rank(v_target, rank, flags); > return -EBUSY; > - } > } > > + ret = vgic_connect_hw_irq(d, NULL, virq, desc, false); > + if ( ret ) > + return ret; > + > clear_bit(_IRQ_GUEST, &desc->status); > desc->handler = &no_irq_type; > > - p->desc = NULL; > - > - vgic_unlock_rank(v_target, rank, flags); > - > return 0; > } > > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 22c8502c95..fda082395b 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count); > int vgic_v3_init(struct domain *d, int *mmio_count); > > bool vgic_evtchn_irq_pending(struct vcpu *v); > +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq, > + struct irq_desc *desc, bool connect); > > extern int domain_vgic_register(struct domain *d, int *mmio_count); > extern int vcpu_vgic_free(struct vcpu *v); >
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c index 8221ae557c..820e464fc0 100644 --- a/xen/arch/arm/gic-vgic.c +++ b/xen/arch/arm/gic-vgic.c @@ -397,6 +397,47 @@ void gic_dump_vgic_info(struct vcpu *v) printk("Pending irq=%d\n", p->irq); } +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq, + struct irq_desc *desc, bool connect) +{ + unsigned long flags; + /* + * Use vcpu0 to retrieve the pending_irq struct. Given that we only + * route SPIs to guests, it doesn't make any difference. + */ + struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); + struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); + struct pending_irq *p = irq_to_pending(v_target, virq); + int ret = 0; + + /* "desc" is optional when we disconnect an IRQ. */ + ASSERT(connect && desc); + + /* We are taking to rank lock to prevent parallel connections. */ + vgic_lock_rank(v_target, rank, flags); + + if ( connect ) + { + /* The VIRQ should not be already enabled by the guest */ + if ( !p->desc && + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) + p->desc = desc; + else + ret = -EBUSY; + } + else + { + if ( desc && p->desc != desc ) + ret = -EINVAL; + else + p->desc = NULL; + } + + vgic_unlock_rank(v_target, rank, flags); + + return ret; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 4cb74d449e..968e46fabb 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -128,13 +128,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority) int gic_route_irq_to_guest(struct domain *d, unsigned int virq, struct irq_desc *desc, unsigned int priority) { - unsigned long flags; - /* Use vcpu0 to retrieve the pending_irq struct. Given that we only - * route SPIs to guests, it doesn't make any difference. */ - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); - struct pending_irq *p = irq_to_pending(v_target, virq); - int res = -EBUSY; + int ret; ASSERT(spin_is_locked(&desc->lock)); /* Caller has already checked that the IRQ is an SPI */ @@ -142,12 +136,9 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, ASSERT(virq < vgic_num_irqs(d)); ASSERT(!is_lpi(virq)); - vgic_lock_rank(v_target, rank, flags); - - if ( p->desc || - /* The VIRQ should not be already enabled by the guest */ - test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) - goto out; + ret = vgic_connect_hw_irq(d, NULL, virq, desc, true); + if ( ret ) + return ret; desc->handler = gic_hw_ops->gic_guest_irq_type; set_bit(_IRQ_GUEST, &desc->status); @@ -156,31 +147,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, gic_set_irq_type(desc, desc->arch.type); gic_set_irq_priority(desc, priority); - p->desc = desc; - res = 0; - -out: - vgic_unlock_rank(v_target, rank, flags); - - return res; + return 0; } /* This function only works with SPIs for now */ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, struct irq_desc *desc) { - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); - struct pending_irq *p = irq_to_pending(v_target, virq); - unsigned long flags; + int ret; ASSERT(spin_is_locked(&desc->lock)); ASSERT(test_bit(_IRQ_GUEST, &desc->status)); - ASSERT(p->desc == desc); ASSERT(!is_lpi(virq)); - vgic_lock_rank(v_target, rank, flags); - if ( d->is_dying ) { desc->handler->shutdown(desc); @@ -198,19 +177,16 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, */ if ( test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED, &desc->status) ) - { - vgic_unlock_rank(v_target, rank, flags); return -EBUSY; - } } + ret = vgic_connect_hw_irq(d, NULL, virq, desc, false); + if ( ret ) + return ret; + clear_bit(_IRQ_GUEST, &desc->status); desc->handler = &no_irq_type; - p->desc = NULL; - - vgic_unlock_rank(v_target, rank, flags); - return 0; } diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 22c8502c95..fda082395b 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count); int vgic_v3_init(struct domain *d, int *mmio_count); bool vgic_evtchn_irq_pending(struct vcpu *v); +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq, + struct irq_desc *desc, bool connect); extern int domain_vgic_register(struct domain *d, int *mmio_count); extern int vcpu_vgic_free(struct vcpu *v);
At the moment we happily access VGIC internal data structures like the rank and struct pending_irq in gic.c, which should be VGIC agnostic. Factor out a new function vgic_connect_hw_irq(), which allows a virtual IRQ to be connected to a hardware IRQ (using the hw bit in the LR). This removes said accesses to VGIC data structures and improves abstraction. One thing to note is that this changes the locking scheme slightly: we hold the rank lock for a shorter period of time, not covering some of the later lines, which deal with the "irq_desc" structure only. This should not have any adverse effect, but is a change in locking anyway. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/gic-vgic.c | 41 +++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/gic.c | 44 ++++++++++---------------------------------- xen/include/asm-arm/vgic.h | 2 ++ 3 files changed, 53 insertions(+), 34 deletions(-)