| Message ID | 1406818852-31856-7-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 31 Jul 2014, Julien Grall wrote: > Actually Xen is assuming that the virtual IRQ will always be equal the IRQ. > > Modify, route_guest_irq to take the virtual IRQ in parameter and let Xen > assigned a different IRQ number. Also store the vIRQ in the desc action to > retrieve easily the IRQ target when we need to inject the interrupt. > > As DOM0 will get most the device, the vIRQ is equal to the IRQ. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Patch added > --- > xen/arch/arm/domain_build.c | 2 +- > xen/arch/arm/gic.c | 5 +++-- > xen/arch/arm/irq.c | 47 +++++++++++++++++++++++++++++++------------ > xen/include/asm-arm/gic.h | 3 ++- > xen/include/asm-arm/irq.h | 4 ++-- > 5 files changed, 42 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 47d114f..0dfe223 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1009,7 +1009,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev) > > if ( available ) > { > - res = route_irq_to_guest(d, irq, dt_node_name(dev)); > + res = route_irq_to_guest(d, irq, irq, dt_node_name(dev)); > if ( res ) > { > printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n", > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 6611ba0..8ef8764 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -126,7 +126,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, > /* Program the GIC to route an interrupt to a guest > * - desc.lock must be held > */ > -void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, > +void gic_route_irq_to_guest(struct domain *d, unsigned int virq, > + struct irq_desc *desc, > const cpumask_t *cpu_mask, unsigned int priority) > { > struct pending_irq *p; > @@ -139,7 +140,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, > > /* Use vcpu0 to retrieve the pending_irq struct. Given that we only > * route SPIs to guests, it doesn't make any difference. */ > - p = irq_to_pending(d->vcpu[0], desc->irq); > + p = irq_to_pending(d->vcpu[0], virq); > p->desc = desc; > } > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 25ecf1d..830832c 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -31,6 +31,13 @@ > static unsigned int local_irqs_type[NR_LOCAL_IRQS]; > static DEFINE_SPINLOCK(local_irqs_type_lock); > > +/* Describe an IRQ assigned to a guest */ > +struct irq_guest > +{ > + struct domain *d; > + unsigned int virq; > +}; > + > static void ack_none(struct irq_desc *irq) > { > printk("unexpected IRQ trap at irq %02x\n", irq->irq); > @@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void) > BUG_ON(init_local_irq_data() < 0); > } > > -static inline struct domain *irq_get_domain(struct irq_desc *desc) > +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc) > { > ASSERT(spin_is_locked(&desc->lock)); > - > - if ( !test_bit(_IRQ_GUEST, &desc->status) ) > - return dom_xen; > - > + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > ASSERT(desc->action != NULL); > > return desc->action->dev_id; > } > > +static inline struct domain *irq_get_domain(struct irq_desc *desc) > +{ > + return irq_get_guest_info(desc)->d; > +} > + > void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) > { > if ( desc != NULL ) > @@ -197,7 +206,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > > if ( test_bit(_IRQ_GUEST, &desc->status) ) > { > - struct domain *d = irq_get_domain(desc); > + struct irq_guest *info = irq_get_guest_info(desc); > > desc->handler->end(desc); > > @@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > > /* the irq cannot be a PPI, we only support delivery of SPIs to > * guests */ > - vgic_vcpu_inject_spi(d, irq); > + vgic_vcpu_inject_spi(info->d, info->virq); > goto out_no_end; > } > > @@ -370,19 +379,30 @@ err: > return rc; > } > > -int route_irq_to_guest(struct domain *d, unsigned int irq, > - const char * devname) > +int route_irq_to_guest(struct domain *d, unsigned int virq, > + unsigned int irq, const char * devname) > { > struct irqaction *action; > - struct irq_desc *desc = irq_to_desc(irq); > + struct irq_guest *info; > + struct irq_desc *desc; > unsigned long flags; > int retval = 0; > > action = xmalloc(struct irqaction); > - if (!action) > + if ( !action ) > + return -ENOMEM; > + > + info = xmalloc(struct irq_guest); > + if ( !info ) > + { > + xfree(action); > return -ENOMEM; > + } Rather than xmalloc'ing another struct and storing the pointer in dev_id, maybe we could simply expand struct arch_irq_desc? > + info->d = d; > + info->virq = virq; > > - action->dev_id = d; > + action->dev_id = info; > action->name = devname; > action->free_on_release = 1; > > @@ -413,7 +433,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq, > if ( retval ) > goto out; > > - gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()), > + gic_route_irq_to_guest(d, virq, desc, cpumask_of(smp_processor_id()), > GIC_PRI_IRQ); > spin_unlock_irqrestore(&desc->lock, flags); > return 0; > @@ -421,6 +441,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq, > out: > spin_unlock_irqrestore(&desc->lock, flags); > xfree(action); > + xfree(info); > > return retval; > } > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index a0c07bf..89816cd 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -196,7 +196,8 @@ extern enum gic_version gic_hw_version(void); > /* Program the GIC to route an interrupt */ > extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, > unsigned int priority); > -extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc, > +extern void gic_route_irq_to_guest(struct domain *, unsigned int virq, > + struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority); > > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index e877334..9bc3492 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -40,8 +40,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); > void init_IRQ(void); > void init_secondary_IRQ(void); > > -int route_irq_to_guest(struct domain *d, unsigned int irq, > - const char *devname); > +int route_irq_to_guest(struct domain *d, unsigned int virq, > + unsigned int irq, const char *devname); > void arch_move_irqs(struct vcpu *v); > > /* Set IRQ type for an SPI */ > -- > 1.7.10.4 >
Hi Stefano, On 08/06/2014 03:50 PM, Stefano Stabellini wrote: >> -int route_irq_to_guest(struct domain *d, unsigned int irq, >> - const char * devname) >> +int route_irq_to_guest(struct domain *d, unsigned int virq, >> + unsigned int irq, const char * devname) >> { >> struct irqaction *action; >> - struct irq_desc *desc = irq_to_desc(irq); >> + struct irq_guest *info; >> + struct irq_desc *desc; >> unsigned long flags; >> int retval = 0; >> >> action = xmalloc(struct irqaction); >> - if (!action) >> + if ( !action ) >> + return -ENOMEM; >> + >> + info = xmalloc(struct irq_guest); >> + if ( !info ) >> + { >> + xfree(action); >> return -ENOMEM; >> + } > > Rather than xmalloc'ing another struct and storing the pointer in > dev_id, maybe we could simply expand struct arch_irq_desc? I though about it. If we add another field in arch_irq_desc, we will likely use more memory than xmalloc. This is because most of the platform doesn't use 1024 interrupts but about 256 interrupts. As the new field will be a pointer (on ARM64, 8 bytes), that would make Xen use statically about 8K more. We could allocate irq_desc dynamically during Xen boot. Regards,
On Wed, 6 Aug 2014, Julien Grall wrote: > Hi Stefano, > > On 08/06/2014 03:50 PM, Stefano Stabellini wrote: > >> -int route_irq_to_guest(struct domain *d, unsigned int irq, > >> - const char * devname) > >> +int route_irq_to_guest(struct domain *d, unsigned int virq, > >> + unsigned int irq, const char * devname) > >> { > >> struct irqaction *action; > >> - struct irq_desc *desc = irq_to_desc(irq); > >> + struct irq_guest *info; > >> + struct irq_desc *desc; > >> unsigned long flags; > >> int retval = 0; > >> > >> action = xmalloc(struct irqaction); > >> - if (!action) > >> + if ( !action ) > >> + return -ENOMEM; > >> + > >> + info = xmalloc(struct irq_guest); > >> + if ( !info ) > >> + { > >> + xfree(action); > >> return -ENOMEM; > >> + } > > > > Rather than xmalloc'ing another struct and storing the pointer in > > dev_id, maybe we could simply expand struct arch_irq_desc? > > I though about it. If we add another field in arch_irq_desc, we will > likely use more memory than xmalloc. This is because most of the > platform doesn't use 1024 interrupts but about 256 interrupts. > > As the new field will be a pointer (on ARM64, 8 bytes), that would make > Xen use statically about 8K more. OK > We could allocate irq_desc dynamically during Xen boot. Let's leave it as is. Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Mostly spelling/grammar nits, a couple of questions but mostly looks good. On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote: > Actually Xen is assuming that the virtual IRQ will always be equal the IRQ. "be equal to" or better "always be the same as". > > Modify, route_guest_irq to take the virtual IRQ in parameter and let Xen Spurious comma. > assigned a different IRQ number. Also store the vIRQ in the desc action to "assign". > retrieve easily the IRQ target when we need to inject the interrupt. "easily retrieve" (or "be able to easily retrieve") > As DOM0 will get most the device, the vIRQ is equal to the IRQ. "most of the devices". I suppose the sentence should end with "in that case" or something? > @@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void) > BUG_ON(init_local_irq_data() < 0); > } > > -static inline struct domain *irq_get_domain(struct irq_desc *desc) > +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc) > { > ASSERT(spin_is_locked(&desc->lock)); > - > - if ( !test_bit(_IRQ_GUEST, &desc->status) ) > - return dom_xen; > - > + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > ASSERT(desc->action != NULL); > > return desc->action->dev_id; > } > > +static inline struct domain *irq_get_domain(struct irq_desc *desc) > +{ > + return irq_get_guest_info(desc)->d; Previously irq_get_domain could return dom_xen for !_IRQ_GUEST domains. What happened to this logic? If it is never possible to get here with a Xen owned IRQ then I think that is worth mentioning in the commit log and/or a comment. > @@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > > /* the irq cannot be a PPI, we only support delivery of SPIs to > * guests */ > - vgic_vcpu_inject_spi(d, irq); > + vgic_vcpu_inject_spi(info->d, info->virq); Could this function take an irq_guest* or are there other callers? Ian.
Hi Ian, On 09/09/14 06:29, Ian Campbell wrote: > Mostly spelling/grammar nits, a couple of questions but mostly looks > good. > > On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote: >> Actually Xen is assuming that the virtual IRQ will always be equal the IRQ. > > "be equal to" or better "always be the same as". > >> >> Modify, route_guest_irq to take the virtual IRQ in parameter and let Xen > > Spurious comma. > >> assigned a different IRQ number. Also store the vIRQ in the desc action to > > "assign". > >> retrieve easily the IRQ target when we need to inject the interrupt. > > "easily retrieve" (or "be able to easily retrieve") > >> As DOM0 will get most the device, the vIRQ is equal to the IRQ. > > "most of the devices". > > I suppose the sentence should end with "in that case" or something? Yes. I will fix all the grammar nits in the next version. >> @@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void) >> BUG_ON(init_local_irq_data() < 0); >> } >> >> -static inline struct domain *irq_get_domain(struct irq_desc *desc) >> +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc) >> { >> ASSERT(spin_is_locked(&desc->lock)); >> - >> - if ( !test_bit(_IRQ_GUEST, &desc->status) ) >> - return dom_xen; >> - >> + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); >> ASSERT(desc->action != NULL); >> >> return desc->action->dev_id; >> } >> >> +static inline struct domain *irq_get_domain(struct irq_desc *desc) >> +{ >> + return irq_get_guest_info(desc)->d; > > Previously irq_get_domain could return dom_xen for !_IRQ_GUEST domains. > What happened to this logic? If it is never possible to get here with a > Xen owned IRQ then I think that is worth mentioning in the commit log > and/or a comment. This was a safe guard in case of developer were misusing the function. Actually every callers of this function are checking that the IRQ is assigned to a guest before calling irq_get_domain. I will mention it in the commit log. > >> @@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) >> >> /* the irq cannot be a PPI, we only support delivery of SPIs to >> * guests */ >> - vgic_vcpu_inject_spi(d, irq); >> + vgic_vcpu_inject_spi(info->d, info->virq); > > Could this function take an irq_guest* or are there other callers? I haven't thought about this possibility. This is the only caller, so I will pass irq_guest. Regards,
Hi Ian, On 09/09/14 11:42, Julien Grall wrote: >>> - vgic_vcpu_inject_spi(d, irq); >>> + vgic_vcpu_inject_spi(info->d, info->virq); >> >> Could this function take an irq_guest* or are there other callers? > > I haven't thought about this possibility. This is the only caller, so I > will pass irq_guest. Looking again to this code, I've defined the structure irq_guest directly in arch/arm/irq.c. And I don't really want to move the definition in header as all the use are within this single file. I will stay with the current solution. Regards,
On Thu, 2014-09-11 at 15:50 -0700, Julien Grall wrote: > Hi Ian, > > On 09/09/14 11:42, Julien Grall wrote: > >>> - vgic_vcpu_inject_spi(d, irq); > >>> + vgic_vcpu_inject_spi(info->d, info->virq); > >> > >> Could this function take an irq_guest* or are there other callers? > > > > I haven't thought about this possibility. This is the only caller, so I > > will pass irq_guest. > > Looking again to this code, I've defined the structure irq_guest > directly in arch/arm/irq.c. And I don't really want to move the > definition in header as all the use are within this single file. > > I will stay with the current solution. OK.
On Thu, 2014-09-11 at 15:50 -0700, Julien Grall wrote: > Hi Ian, > > On 09/09/14 11:42, Julien Grall wrote: > >>> - vgic_vcpu_inject_spi(d, irq); > >>> + vgic_vcpu_inject_spi(info->d, info->virq); > >> > >> Could this function take an irq_guest* or are there other callers? > > > > I haven't thought about this possibility. This is the only caller, so I > > will pass irq_guest. > > Looking again to this code, I've defined the structure irq_guest > directly in arch/arm/irq.c. And I don't really want to move the > definition in header as all the use are within this single file. > > I will stay with the current solution. OK
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 47d114f..0dfe223 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1009,7 +1009,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev) if ( available ) { - res = route_irq_to_guest(d, irq, dt_node_name(dev)); + res = route_irq_to_guest(d, irq, irq, dt_node_name(dev)); if ( res ) { printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n", diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 6611ba0..8ef8764 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -126,7 +126,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, /* Program the GIC to route an interrupt to a guest * - desc.lock must be held */ -void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, +void gic_route_irq_to_guest(struct domain *d, unsigned int virq, + struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority) { struct pending_irq *p; @@ -139,7 +140,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, /* Use vcpu0 to retrieve the pending_irq struct. Given that we only * route SPIs to guests, it doesn't make any difference. */ - p = irq_to_pending(d->vcpu[0], desc->irq); + p = irq_to_pending(d->vcpu[0], virq); p->desc = desc; } diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 25ecf1d..830832c 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -31,6 +31,13 @@ static unsigned int local_irqs_type[NR_LOCAL_IRQS]; static DEFINE_SPINLOCK(local_irqs_type_lock); +/* Describe an IRQ assigned to a guest */ +struct irq_guest +{ + struct domain *d; + unsigned int virq; +}; + static void ack_none(struct irq_desc *irq) { printk("unexpected IRQ trap at irq %02x\n", irq->irq); @@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void) BUG_ON(init_local_irq_data() < 0); } -static inline struct domain *irq_get_domain(struct irq_desc *desc) +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc) { ASSERT(spin_is_locked(&desc->lock)); - - if ( !test_bit(_IRQ_GUEST, &desc->status) ) - return dom_xen; - + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); ASSERT(desc->action != NULL); return desc->action->dev_id; } +static inline struct domain *irq_get_domain(struct irq_desc *desc) +{ + return irq_get_guest_info(desc)->d; +} + void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) { if ( desc != NULL ) @@ -197,7 +206,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) if ( test_bit(_IRQ_GUEST, &desc->status) ) { - struct domain *d = irq_get_domain(desc); + struct irq_guest *info = irq_get_guest_info(desc); desc->handler->end(desc); @@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) /* the irq cannot be a PPI, we only support delivery of SPIs to * guests */ - vgic_vcpu_inject_spi(d, irq); + vgic_vcpu_inject_spi(info->d, info->virq); goto out_no_end; } @@ -370,19 +379,30 @@ err: return rc; } -int route_irq_to_guest(struct domain *d, unsigned int irq, - const char * devname) +int route_irq_to_guest(struct domain *d, unsigned int virq, + unsigned int irq, const char * devname) { struct irqaction *action; - struct irq_desc *desc = irq_to_desc(irq); + struct irq_guest *info; + struct irq_desc *desc; unsigned long flags; int retval = 0; action = xmalloc(struct irqaction); - if (!action) + if ( !action ) + return -ENOMEM; + + info = xmalloc(struct irq_guest); + if ( !info ) + { + xfree(action); return -ENOMEM; + } + + info->d = d; + info->virq = virq; - action->dev_id = d; + action->dev_id = info; action->name = devname; action->free_on_release = 1; @@ -413,7 +433,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq, if ( retval ) goto out; - gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()), + gic_route_irq_to_guest(d, virq, desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); spin_unlock_irqrestore(&desc->lock, flags); return 0; @@ -421,6 +441,7 @@ int route_irq_to_guest(struct domain *d, unsigned int irq, out: spin_unlock_irqrestore(&desc->lock, flags); xfree(action); + xfree(info); return retval; } diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index a0c07bf..89816cd 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -196,7 +196,8 @@ extern enum gic_version gic_hw_version(void); /* Program the GIC to route an interrupt */ extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority); -extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc, +extern void gic_route_irq_to_guest(struct domain *, unsigned int virq, + struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority); diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index e877334..9bc3492 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -40,8 +40,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); void init_IRQ(void); void init_secondary_IRQ(void); -int route_irq_to_guest(struct domain *d, unsigned int irq, - const char *devname); +int route_irq_to_guest(struct domain *d, unsigned int virq, + unsigned int irq, const char *devname); void arch_move_irqs(struct vcpu *v); /* Set IRQ type for an SPI */
Actually Xen is assuming that the virtual IRQ will always be equal the IRQ. Modify, route_guest_irq to take the virtual IRQ in parameter and let Xen assigned a different IRQ number. Also store the vIRQ in the desc action to retrieve easily the IRQ target when we need to inject the interrupt. As DOM0 will get most the device, the vIRQ is equal to the IRQ. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Patch added --- xen/arch/arm/domain_build.c | 2 +- xen/arch/arm/gic.c | 5 +++-- xen/arch/arm/irq.c | 47 +++++++++++++++++++++++++++++++------------ xen/include/asm-arm/gic.h | 3 ++- xen/include/asm-arm/irq.h | 4 ++-- 5 files changed, 42 insertions(+), 19 deletions(-)