| Message ID | 1421159133-31526-9-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 28/01/15 16:47, Stefano Stabellini wrote: >> 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; >> +}; > > I would prefer if you didn't use dev_id for this and just added a virq > field to irqaction. We already talked about it on v2. You were fine with the idea and acked the patch. Although, I haven't add your acked-by here because of the new changes in the code. Here my answer to the same question on v2: "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 28/01/15 16:56, Julien Grall wrote: > On 28/01/15 16:47, Stefano Stabellini wrote: >>> 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; >>> +}; >> >> I would prefer if you didn't use dev_id for this and just added a virq >> field to irqaction. > > We already talked about it on v2. You were fine with the idea and acked > the patch. Although, I haven't add your acked-by here because of the new > changes in the code. For reference: https://patches.linaro.org/34671/ > Here my answer to the same question on v2: > > "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."
On 20/02/15 15:52, Ian Campbell wrote: > On Tue, 2015-01-13 at 14:25 +0000, Julien Grall wrote: >> Actually Xen is assuming that the virtual IRQ will always be the same as IRQ. > > s/Actually/Currently/? Yes, I always mix both because the former is close to the french word. >> Modify route_guest_irq to take the virtual IRQ in parameter and let Xen >> assign a different IRQ number. > > I think I must be misunderstanding this, but if route_guest_irq is > taking the vIRQ as a parameter then why does it then need to assign a > different IRQ number? > > Oh, did you mean allowing the *caller* to setup a non-1:1 mapping > perhaps? I think s/and let Xen assign/which allows Xen to.../ might be > closer to that meaning if that's the intention? Yes, the IRQ is not mapped 1:1 to the guest. I will replace by "which allows Xen to...". >> Also store the vIRQ in the desc action to >> easily retrieve easily the IRQ target when we need to inject the interrupt. > > -ETooManyEasilys ;-) > > (I think you want to drop the second one) Will fix it. > >> As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. > > Am I correct that after this patch all callers still pass irq==virq to > the new function? > >> At the same time modify the behavior of irq_get_domain. The function now >> assumes that the irq_desc belongs to an IRQ assigned to a guest. > > s/assumes/requires/? Will fix it. >> >> action = xmalloc(struct irqaction); >> - if (!action) >> + if ( !action ) >> + return -ENOMEM; >> + >> + info = xmalloc(struct irq_guest); > > FWIW you might (subject to sizing/alignment needs) be able to do > action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); > info = (sturct irq_guest *)(action + 1); > > which would save some memory overhead for free pointers etc and allow > you to avoid manually managing the info. > > You probably won't like that though, so feel free to ignore. Actually it's a good idea :). I haven't though about it. Regards,
Hi Ian, On 20/02/15 15:52, Ian Campbell wrote: >> As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. > > Am I correct that after this patch all callers still pass irq==virq to > the new function? Sorry, I forgot to answer to this question. Yes, all the callers will pass irq == virq in case of DOM0. Regards,
Hi Ian, On 20/02/15 17:09, Julien Grall wrote: > On 20/02/15 15:52, Ian Campbell wrote: >>> >>> action = xmalloc(struct irqaction); >>> - if (!action) >>> + if ( !action ) >>> + return -ENOMEM; >>> + >>> + info = xmalloc(struct irq_guest); >> >> FWIW you might (subject to sizing/alignment needs) be able to do >> action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); >> info = (sturct irq_guest *)(action + 1); >> >> which would save some memory overhead for free pointers etc and allow >> you to avoid manually managing the info. >> >> You probably won't like that though, so feel free to ignore. > > Actually it's a good idea :). I haven't though about it. I though about it. The pointer to irq_guest may not be correctly aligned with this solution, right? So I prefer to keep separate the allocation. We can revisit it later. Regards,
On 27/02/15 14:44, Ian Campbell wrote: > On Fri, 2015-02-27 at 14:33 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 20/02/15 17:09, Julien Grall wrote: >>> On 20/02/15 15:52, Ian Campbell wrote: >>>>> >>>>> action = xmalloc(struct irqaction); >>>>> - if (!action) >>>>> + if ( !action ) >>>>> + return -ENOMEM; >>>>> + >>>>> + info = xmalloc(struct irq_guest); >>>> >>>> FWIW you might (subject to sizing/alignment needs) be able to do >>>> action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); >>>> info = (sturct irq_guest *)(action + 1); >>>> >>>> which would save some memory overhead for free pointers etc and allow >>>> you to avoid manually managing the info. >>>> >>>> You probably won't like that though, so feel free to ignore. >>> >>> Actually it's a good idea :). I haven't though about it. >> >> I though about it. The pointer to irq_guest may not be correctly aligned >> with this solution, right? > > It depends on sizeof(struct irqaction) (which is what I meant by > "subject to..."). t'd probably need a ROUNDUP(sizeof(foo), > pointer-alignement) in there somewhere. Oh right, I missed the parentheses part. The pointer alignment is not the same on ARM32 and ARM64, I need to think more about it. Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index b48b5d0..06c1dec 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1029,7 +1029,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev) * twice the IRQ. This can happen if the IRQ is shared */ vgic_reserve_virq(d, irq); - 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 eb0c5d6..15de283 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/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index b0808b8..e2f8b89 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -75,7 +75,7 @@ static int map_one_spi(struct domain *d, const char *what, printk("Failed to reserve the vIRQ %u on dom%d\n", irq, d->domain_id); - ret = route_irq_to_guest(d, irq, what); + ret = route_irq_to_guest(d, irq, irq, what); if ( ret ) printk("Failed to route %s to dom%d\n", what, d->domain_id); diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 38216f7..c915670 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -380,16 +380,16 @@ void vgic_clear_pending_irqs(struct vcpu *v) spin_unlock_irqrestore(&v->arch.vgic.lock, flags); } -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) +void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) { uint8_t priority; - struct vgic_irq_rank *rank = vgic_rank_irq(v, irq); - struct pending_irq *iter, *n = irq_to_pending(v, irq); + struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); + struct pending_irq *iter, *n = irq_to_pending(v, virq); unsigned long flags; bool_t running; vgic_lock_rank(v, rank, flags); - priority = v->domain->arch.vgic.handler->get_irq_priority(v, irq); + priority = v->domain->arch.vgic.handler->get_irq_priority(v, virq); vgic_unlock_rank(v, rank, flags); spin_lock_irqsave(&v->arch.vgic.lock, flags); @@ -405,7 +405,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) if ( !list_empty(&n->inflight) ) { - gic_raise_inflight_irq(v, irq); + gic_raise_inflight_irq(v, virq); goto out; } @@ -413,7 +413,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) /* the irq is enabled */ if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) ) - gic_raise_guest_irq(v, irq, priority); + gic_raise_guest_irq(v, virq, priority); list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight ) { @@ -433,15 +433,15 @@ out: smp_send_event_check_mask(cpumask_of(v->processor)); } -void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq) +void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq) { struct vcpu *v; /* the IRQ needs to be an SPI */ - ASSERT(irq >= 32 && irq <= gic_number_lines()); + ASSERT(virq >= 32 && virq <= vgic_num_irqs(d)); - v = vgic_get_target_vcpu(d->vcpu[0], irq); - vgic_vcpu_inject_irq(v, irq); + v = vgic_get_target_vcpu(d->vcpu[0], virq); + vgic_vcpu_inject_irq(v, virq); } void arch_evtchn_inject(struct vcpu *v) diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 6286c71..cf9f257 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -213,7 +213,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 435dfcd..f00eb11 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 */ diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 8582d9d..1cd7808 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -181,8 +181,8 @@ extern int domain_vgic_init(struct domain *d); extern void domain_vgic_free(struct domain *d); extern int vcpu_vgic_init(struct vcpu *v); extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); -extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq); -extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq); +extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq); +extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq); extern void vgic_clear_pending_irqs(struct vcpu *v); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq); extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
Actually Xen is assuming that the virtual IRQ will always be the same as IRQ. Modify route_guest_irq to take the virtual IRQ in parameter and let Xen assign a different IRQ number. Also store the vIRQ in the desc action to easily retrieve easily the IRQ target when we need to inject the interrupt. As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. At the same time modify the behavior of irq_get_domain. The function now assumes that the irq_desc belongs to an IRQ assigned to a guest. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3 - Spelling/grammar nits - Fix compilation on ARM64. Forgot to update route_irq_to_guest call for xgene platform. - Add a word about irq_get_domain behavior change - More s/irq/virq/ because of the rebasing on the latest staging 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/arch/arm/platforms/xgene-storm.c | 2 +- xen/arch/arm/vgic.c | 20 +++++++-------- xen/include/asm-arm/gic.h | 3 ++- xen/include/asm-arm/irq.h | 4 +-- xen/include/asm-arm/vgic.h | 4 +-- 8 files changed, 55 insertions(+), 32 deletions(-)