| Message ID | 1421159133-31526-10-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 28/01/15 17:55, Stefano Stabellini wrote: >> --- >> xen/arch/arm/irq.c | 58 +++++++++++++++++++++++++++++++++++++++++++---- >> xen/include/asm-arm/irq.h | 2 ++ >> 2 files changed, 56 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 830832c..af408ac 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -379,6 +379,15 @@ err: >> return rc; >> } >> >> +bool_t is_assignable_irq(unsigned int irq) > > static inline? It's exported (will be used later) and not possible to inline in irq.h because of interdependency between irq.h and gic.h [..] >> @@ -418,13 +460,21 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, >> struct domain *ad = irq_get_domain(desc); >> >> if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) >> + { >> + if ( irq_get_guest_info(desc)->virq != virq ) >> + { >> + dprintk(XENLOG_G_ERR, "d%u: IRQ %u is already assigned to vIRQ %u\n", >> + d->domain_id, irq, irq_get_guest_info(desc)->virq); >> + retval = -EPERM; > > I don't think that EPERM is the right error for this. Maybe EBUSY? Right. > >> + } > > Should we return error for this too? Maybe EEXIST? No, this is a valid use case especially for DOM0. The device tree may expose twice the same IRQ. Regards,
Hi Ian, On 20/02/15 16:00, Ian Campbell wrote: > On Tue, 2015-01-13 at 14:25 +0000, Julien Grall wrote: >> Currently Xen only supports SPIs routing for guest, add a function >> is_assignable_irq to check if we can assign a given IRQ to the guest. >> >> Secondly, make sure the vIRQ is not the greater that the number of IRQs handle >> to the vGIC and it's an SPIs. > > I think you mean the "number of IRQs handled by the vGIC" (or configured > in?) and it would just be "an SPI". I think "configured in" is better here. I will change to "number of IRQs configured in the vGIC". >> Thirdly, when the IRQ is already assigned to the domain, check the user >> is not asking to use a different vIRQ than the one already bound. >> >> Finally, desc->arch.type which contains the IRQ type (i.e level/edge) must >> be correctly configured before. The IRQ type won't be configure when: > ^routing? No, I wanted to mean when a IRQ type is not set. I will replace the last sentence by "This can happen when:" > >> - the device has been blacklist for the current platform > > "blacklisted". > >> - the IRQ has not been describe in the device tree > > "described". > >> I think we can safely assume that a user won't never ask to route >> as such IRQ to the guest. > > Can we now ;-) Does this mean the code doesn't check for and abort on > these cases? > <later>Having read further I think you do catch it, so I think you can > remove that sentence, or at least append "...but we check for this > anyway"</later>. Right, this sentence is not clear. What I wanted to mean is we won't support any IRQ not described in the DT or which belong to a specific domain. But with an upcoming patch from Parth, the IRQ configuration (edge/level) will be deferred until the guest has enabled this IRQ. >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 830832c..af408ac 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -379,6 +379,15 @@ err: >> return rc; >> } >> >> +bool_t is_assignable_irq(unsigned int irq) >> +{ >> + /* For now, we can only route SPIs to the guest */ >> + return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())); >> +} >> + >> +/* Route an IRQ to a specific guest. >> + * For now only SPIs are assignabled to the guest. > > "assignable" > >> + */ >> int route_irq_to_guest(struct domain *d, unsigned int virq, >> unsigned int irq, const char * devname) >> { >> @@ -388,6 +397,29 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, >> unsigned long flags; >> int retval = 0; >> >> + if ( !is_assignable_irq(irq) ) >> + { >> + dprintk(XENLOG_G_ERR, "the IRQ%u is not routable\n", irq); >> + return -EINVAL; >> + } >> + >> + desc = irq_to_desc(irq); > > I can't remember if this is expensive, but you could safely do it > further down after more of the sanity checks. For now we retrieve it from an array. But Vijay's plan to replace the array by a radix tree. I will move the whole block (if () ... desc = ) after the vGIC because I think they should be tight. > >> + >> + if ( virq >= vgic_num_irqs(d) ) >> + { >> + dprintk(XENLOG_G_ERR, >> + "the vIRQ number %u is too high for domain %u (max = %u)\n", >> + irq, d->domain_id, vgic_num_irqs(d)); >> + return -EINVAL; >> + } >> + >> + /* Only routing to virtual SPIs is supported */ >> + if ( virq < 32 ) > > NR_LOCAL_IRQS? Yes. I think I have multiple place where 32 is open-coded. I will replace them. > >> + { >> + dprintk(XENLOG_G_ERR, "IRQ can only be routed to a virtual SPIs"); > > Just "SPI". > >> - printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n", >> - irq, ad->domain_id); >> + dprintk(XENLOG_G_ERR, "IRQ %u is already used by domain %u\n", >> + irq, ad->domain_id); >> else >> - printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", irq); >> + dprintk(XENLOG_G_ERR, "IRQ %u is already used by Xen\n", irq); > > Is the file/line really needed here? The messages seem reasonably unique > already. I don't remember why I made this change. I will drop it. Regards,
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 830832c..af408ac 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -379,6 +379,15 @@ err: return rc; } +bool_t is_assignable_irq(unsigned int irq) +{ + /* For now, we can only route SPIs to the guest */ + return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())); +} + +/* Route an IRQ to a specific guest. + * For now only SPIs are assignabled to the guest. + */ int route_irq_to_guest(struct domain *d, unsigned int virq, unsigned int irq, const char * devname) { @@ -388,6 +397,29 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, unsigned long flags; int retval = 0; + if ( !is_assignable_irq(irq) ) + { + dprintk(XENLOG_G_ERR, "the IRQ%u is not routable\n", irq); + return -EINVAL; + } + + desc = irq_to_desc(irq); + + if ( virq >= vgic_num_irqs(d) ) + { + dprintk(XENLOG_G_ERR, + "the vIRQ number %u is too high for domain %u (max = %u)\n", + irq, d->domain_id, vgic_num_irqs(d)); + return -EINVAL; + } + + /* Only routing to virtual SPIs is supported */ + if ( virq < 32 ) + { + dprintk(XENLOG_G_ERR, "IRQ can only be routed to a virtual SPIs"); + return -EINVAL; + } + action = xmalloc(struct irqaction); if ( !action ) return -ENOMEM; @@ -408,8 +440,18 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, spin_lock_irqsave(&desc->lock, flags); + if ( desc->arch.type == DT_IRQ_TYPE_INVALID ) + { + dprintk(XENLOG_G_ERR, "IRQ %u has not been configured\n", + irq); + retval = -EIO; + goto out; + } + /* If the IRQ is already used by someone - * - If it's the same domain -> Xen doesn't need to update the IRQ desc + * - If it's the same domain -> Xen doesn't need to update the IRQ desc. + * For safety check if we are not trying to assign the IRQ to a + * different vIRQ. * - Otherwise -> For now, don't allow the IRQ to be shared between * Xen and domains. */ @@ -418,13 +460,21 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, struct domain *ad = irq_get_domain(desc); if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) + { + if ( irq_get_guest_info(desc)->virq != virq ) + { + dprintk(XENLOG_G_ERR, "d%u: IRQ %u is already assigned to vIRQ %u\n", + d->domain_id, irq, irq_get_guest_info(desc)->virq); + retval = -EPERM; + } goto out; + } if ( test_bit(_IRQ_GUEST, &desc->status) ) - printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n", - irq, ad->domain_id); + dprintk(XENLOG_G_ERR, "IRQ %u is already used by domain %u\n", + irq, ad->domain_id); else - printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", irq); + dprintk(XENLOG_G_ERR, "IRQ %u is already used by Xen\n", irq); retval = -EBUSY; goto out; } diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index f00eb11..71b39e7 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -37,6 +37,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); #define domain_pirq_to_irq(d, pirq) (pirq) +bool_t is_assignable_irq(unsigned int irq); + void init_IRQ(void); void init_secondary_IRQ(void);
Currently Xen only supports SPIs routing for guest, add a function is_assignable_irq to check if we can assign a given IRQ to the guest. Secondly, make sure the vIRQ is not the greater that the number of IRQs handle to the vGIC and it's an SPIs. Thirdly, when the IRQ is already assigned to the domain, check the user is not asking to use a different vIRQ than the one already bound. Finally, desc->arch.type which contains the IRQ type (i.e level/edge) must be correctly configured before. The IRQ type won't be configure when: - the device has been blacklist for the current platform - the IRQ has not been describe in the device tree I think we can safely assume that a user won't never ask to route as such IRQ to the guest. Also, use XENLOG_G_ERR in the error message within the function as it will be later called from a guest. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Fix typo in commit message and comment - Add a check that the vIRQ is an SPI - Check if the user is not asking for a different vIRQ when the IRQ is already assigned to the guest Changes in v2: - Rename is_routable_irq into is_assignable_irq - Check if the IRQ is not greater than the number handled by the number of IRQs handled by the gic - Move is_assignable_irq in irq.c rather than defining in the header irq.h - Retrieve the irq descriptor after checking the validity of the IRQ - vgic_num_irqs has been moved in a separate patch - Fix the irq check against vgic_num_irqs - Use virq instead of irq for vGIC sanity check --- xen/arch/arm/irq.c | 58 +++++++++++++++++++++++++++++++++++++++++++---- xen/include/asm-arm/irq.h | 2 ++ 2 files changed, 56 insertions(+), 4 deletions(-)