| Message ID | 1418395392-30460-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Stefano, On 15/12/14 15:32, Stefano Stabellini wrote: > On Fri, 12 Dec 2014, Julien Grall wrote: >> + spin_lock_init(&d->arch.vgic.lock); > > you should probably explain in the commit message the reason why you are > making changes to the vgic lock Actually the domain vgic lock was never used. Only the per-vcpu vgic lock was in used. So I don't make any change to the vgic lock. If I don't use it, I will add a patch to drop it. >> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr) >> return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr); >> } >> >> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq) >> +{ >> + bool_t reserved; >> + >> + if ( virq >= vgic_num_irqs(d) ) >> + return 0; >> + >> + spin_lock(&d->arch.vgic.lock); >> + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs); >> + spin_unlock(&d->arch.vgic.lock); > > test_and_set_bit is atomic, why do you need to take the lock? To avoid race condition with vgic_allocate_virq. Anyway, I will dropped it with your suggestion to implement vgic_allocate_virq without lock. [..] >> +int vgic_allocate_virq(struct domain *d, bool_t spi) >> +{ >> + int ret = -1; >> + unsigned int virq; >> + >> + spin_lock(&d->arch.vgic.lock); >> + if ( !spi ) >> + { >> + virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32); >> + if ( virq >= 32 ) >> + goto unlock; >> + } >> + else >> + { >> + virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, >> + 32, vgic_num_irqs(d)); >> + if ( virq >= vgic_num_irqs(d) ) >> + goto unlock; >> + } >> + >> + set_bit(virq, d->arch.vgic.allocated_irqs); >> + ret = virq; >> + >> +unlock: >> + spin_unlock(&d->arch.vgic.lock); > > you might be able to write this function without taking the lock too, by > using test_and_set_bit and retries: > > retry: > virq = find_first_zero_bit; > if (test_and_set_bit(virq)) > goto retry; I will give a look to it. I will also to limit the number of retry (maybe to the number of vIRQ) for safety. Regards,
On 15/12/14 16:07, Julien Grall wrote: > On 15/12/14 15:32, Stefano Stabellini wrote: >> On Fri, 12 Dec 2014, Julien Grall wrote: >>> + spin_lock_init(&d->arch.vgic.lock); >> >> you should probably explain in the commit message the reason why you are >> making changes to the vgic lock > > Actually the domain vgic lock was never used. Only the per-vcpu vgic > lock was in used. > > So I don't make any change to the vgic lock. If I don't use it, I will > add a patch to drop it. > >>> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr) >>> return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr); >>> } >>> >>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq) >>> +{ >>> + bool_t reserved; >>> + >>> + if ( virq >= vgic_num_irqs(d) ) >>> + return 0; >>> + >>> + spin_lock(&d->arch.vgic.lock); >>> + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs); >>> + spin_unlock(&d->arch.vgic.lock); >> >> test_and_set_bit is atomic, why do you need to take the lock? > > To avoid race condition with vgic_allocate_virq. > > Anyway, I will dropped it with your suggestion to implement > vgic_allocate_virq without lock. Hmmm ... I was wrong on this one. The domain vgic lock is used in the macro vgic_lock. But it has never been initialized. I will send a separate patch for correctly initialize it. Regards,
Hi Ian, On 13/01/15 15:51, Ian Campbell wrote: > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote: >> While it's easy to know which hardware IRQ is assigned to a domain, there >> is no way to know which IRQ is emulated by Xen for a specific domain. > > It seems you are tracking all valid interrupts, including hardware ones, > not just those for emulated devices? Perhaps rather than emulated you > meant "allocated to the guest" or "routed" or something? > >> Introduce a bitmap to keep track of every vIRQ used by a domain. This >> will be used later to find free vIRQ for interrupt device assignment and >> emulated interrupt. > > Actually, don't you implement the alloc/free of vIRQs here too? Yes. > Is there a usecase for tracking SPIs in this way, or would tracking PPIs > only be sufficient? We need to track everything for interrupt assignment to a guest/dom0. So if the guest ask for a free vIRQ we can give it directly. >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/domain_build.c | 6 +++ >> xen/arch/arm/platforms/xgene-storm.c | 4 ++ >> xen/arch/arm/vgic.c | 76 ++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vtimer.c | 15 +++++++ >> xen/include/asm-arm/domain.h | 1 + >> xen/include/asm-arm/vgic.h | 13 ++++++ >> 6 files changed, 115 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index de180d8..c238c8f 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev) >> irq = res; >> >> DPRINT("irq %u = %u\n", i, irq); >> + /* >> + * Checking the return of vgic_reserve_virq is not >> + * necessary. It should not fail except when we try to map >> + * twice the IRQ. This can happen if the IRQ is shared > > Return and handle EBUSY to distinguish other errors? vgic_reserve_virq can fail for 2 reasons: - The IRQ is too high to handle by the vGIC => Unlikely as DOM0 use the same IRQ number as the hardware. - The vIRQ is already reserved. The former will be catch just after with route_irq_to_guest. So I don't think it's worth to change the return from a bool to an int and return -EBUSY. > ("try to map the IRQ twice") > >> + */ >> + vgic_reserve_virq(d, irq); >> res = route_irq_to_guest(d, irq, dt_node_name(dev)); >> if ( res ) >> { >> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c >> index 0b3492d..416d42c 100644 >> --- a/xen/arch/arm/platforms/xgene-storm.c >> +++ b/xen/arch/arm/platforms/xgene-storm.c >> @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what, >> >> printk("Additional IRQ %u (%s)\n", irq, what); >> >> + if ( !vgic_reserve_virq(d, irq) ) >> + printk("Failed to reserve the vIRQ %u on dom%d\n", > > Drop "the". Ok. >> + irq, d->domain_id); >> + >> ret = route_irq_to_guest(d, 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 75cb7ff..dbfc259 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d) >> return -ENODEV; >> } >> >> + spin_lock_init(&d->arch.vgic.lock); >> + >> d->arch.vgic.shared_irqs = >> xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d)); >> if ( d->arch.vgic.shared_irqs == NULL ) >> @@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d) >> >> d->arch.vgic.handler->domain_init(d); >> >> + d->arch.vgic.allocated_irqs = >> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d))); > > (this was why I asked if tracking SPIs was needed...) To complete my answer above: - dom0: vgic_num_irqs() = number of hardware IRQS - guest: vgic_num_irqs() = 32. So we don't waste memory. > >> + if ( !d->arch.vgic.allocated_irqs ) >> + return -ENOMEM; >> + >> + /* vIRQ0-15 (SGIs) are reserved */ >> + for (i = 0; i <= 15; i++) >> + set_bit(i, d->arch.vgic.allocated_irqs); >> + >> return 0; >> } >> >> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d) >> { >> xfree(d->arch.vgic.shared_irqs); >> xfree(d->arch.vgic.pending_irqs); >> + xfree(d->arch.vgic.allocated_irqs); >> } >> >> int vcpu_vgic_init(struct vcpu *v) >> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr) >> return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr); >> } >> >> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq) >> +{ >> + bool_t reserved; >> + >> + if ( virq >= vgic_num_irqs(d) ) >> + return 0; > > EINVAL? vgic_reserve_irq returns a boolean: 0 => not reserved 1 => reserved I don't see why we should return an int in this case, as the caller should know how to use it. >> + spin_lock(&d->arch.vgic.lock); >> + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs); >> + spin_unlock(&d->arch.vgic.lock); >> + >> + return reserved; >> +} >> + >> +int vgic_allocate_virq(struct domain *d, bool_t spi) >> +{ >> + int ret = -1; >> + unsigned int virq; >> + >> + spin_lock(&d->arch.vgic.lock); >> + if ( !spi ) >> + { >> + virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32); > > I think you could use find_next_zero_bit here to start the search at bit > 16 and stop at bit 31. Having done so, it might be nicer to if (spi) to > select min and max IRQs and have the bit manipulation all be common. I will give a look for the next version. > >> +void vgic_free_virq(struct domain *d, unsigned int virq) > > It only frees spis, but the alloc version can do SPI or PPI. Is that on > purpose? I forgot to update vgic_free_virq when I made the support for PPIs. >> +{ >> + unsigned int spi; >> + >> + if ( is_hardware_domain(d) ) >> + return; >> + >> + if ( virq < 32 && virq >= vgic_num_irqs(d) ) >> + return; >> + >> + spi = virq - 32; >> + >> + /* Taking the vGIC domain lock is not necessary. We don't care if >> + * the bit is cleared a bit later. What only matters is bit to 1. > > I don't grok the last sentence here. > >> + * >> + * With this solution vgic_allocate may fail to find an vIRQ if the >> + * allocated_irqs is fully. But we don't care. > > are some words missing after fully? This will be dropped in the next version. So forget this part :). >> + */ >> + clear_bit(spi, d->arch.vgic.allocated_irqs); >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c >> index 2e95ceb..de660bb 100644 >> + */ >> +extern int vgic_allocate_virq(struct domain *d, bool_t spi); >> + >> +extern void vgic_free_virq(struct domain *d, unsigned int irq); >> + >> #endif /* __ASM_ARM_VGIC_H__ */ >> >> /* > > >> --- a/xen/arch/arm/vtimer.c >> +++ b/xen/arch/arm/vtimer.c >> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d) >> { >> d->arch.phys_timer_base.offset = NOW(); >> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0); >> + >> + /* At this stage vgic_reserve_virq can't fail */ >> + if ( is_hardware_domain(d) ) >> + { >> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI))); >> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI))); >> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI))); >> + } >> + else >> + { >> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI)); >> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI)); >> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI)); > > Although BUG_ON is not conditional on $debug I think we still should > avoid side effects in the condition. I know, but this should never fail as it called during on domain construction. If so we may have some other issue later if we decide to assign PPI to a guest. I would prefer to keep the BUG_ON here. > >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >> index 74d5a4e..9e167fa 100644 >> --- a/xen/include/asm-arm/vgic.h >> +++ b/xen/include/asm-arm/vgic.h >> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir, >> enum gic_sgi_mode irqmode, int virq, >> unsigned long vcpu_mask); >> extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); >> + >> +/* Reserve a specific guest vIRQ */ >> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq); >> + >> +/* >> + * Allocate a guest VIRQ >> + * - spi == 0 => allocate a PPI. It will be the same on every vCPU >> + * - spi == 0 => allocate an SGI > > s/== 0/== 1/ and s/SGI/SPI/ in the last line. I will fix it. Regards,
(CC Jan) Hi Ian, On 13/01/15 16:46, Ian Campbell wrote: >> vgic_reserve_irq returns a boolean: > > Please use true/false then. > > In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm > not sure what the rules are for use. Jan please correct me if I'm wrong, xen/stdbool.h has been introduced for the ELF code and should not be used anywhere else. true/false is defined in xen/stdbool.h together with Bool not bool_t. >> 0 => not reserved >> 1 => reserved >> >> I don't see why we should return an int in this case, as the caller >> should know how to use it. > > It's slightly more conventional to return error codes, but I guess I > don't mind much. Agree, but in this particular case we don't have to know the error code. So it's pointless to return it. >>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d) >>>> { >>>> d->arch.phys_timer_base.offset = NOW(); >>>> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0); >>>> + >>>> + /* At this stage vgic_reserve_virq can't fail */ >>>> + if ( is_hardware_domain(d) ) >>>> + { >>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI))); >>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI))); >>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI))); >>>> + } >>>> + else >>>> + { >>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI)); >>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI)); >>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI)); >>> >>> Although BUG_ON is not conditional on $debug I think we still should >>> avoid side effects in the condition. >> >> I know, but this should never fail as it called during on domain >> construction. If so we may have some other issue later if we decide to >> assign PPI to a guest. >> >> I would prefer to keep the BUG_ON here > > I'm not objecting the the BUG_ON itself but to the fact that the > condition has a side effect. Please use: > if (!do_something()) > BUG() > instead to avoid this. We have other place in the code where BUG_ON as a side-effect. IHMO, if (!do_something()) BUG() <=> BUG_ON. On the latter you know directly why it's failing, on the former you have to look at the code. Regards,
On 13/01/15 16:57, Julien Grall wrote: > (CC Jan) Forgot to really CC Jan for the bool stuff. > Hi Ian, > > On 13/01/15 16:46, Ian Campbell wrote: >>> vgic_reserve_irq returns a boolean: >> >> Please use true/false then. >> >> In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm >> not sure what the rules are for use. > > Jan please correct me if I'm wrong, xen/stdbool.h has been introduced > for the ELF code and should not be used anywhere else. > > true/false is defined in xen/stdbool.h together with Bool not bool_t. > >>> 0 => not reserved >>> 1 => reserved >>> >>> I don't see why we should return an int in this case, as the caller >>> should know how to use it. >> >> It's slightly more conventional to return error codes, but I guess I >> don't mind much. > > Agree, but in this particular case we don't have to know the error code. > So it's pointless to return it. > >>>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d) >>>>> { >>>>> d->arch.phys_timer_base.offset = NOW(); >>>>> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0); >>>>> + >>>>> + /* At this stage vgic_reserve_virq can't fail */ >>>>> + if ( is_hardware_domain(d) ) >>>>> + { >>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI))); >>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI))); >>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI))); >>>>> + } >>>>> + else >>>>> + { >>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI)); >>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI)); >>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI)); >>>> >>>> Although BUG_ON is not conditional on $debug I think we still should >>>> avoid side effects in the condition. >>> >>> I know, but this should never fail as it called during on domain >>> construction. If so we may have some other issue later if we decide to >>> assign PPI to a guest. >>> >>> I would prefer to keep the BUG_ON here >> >> I'm not objecting the the BUG_ON itself but to the fact that the >> condition has a side effect. Please use: >> if (!do_something()) >> BUG() >> instead to avoid this. > > We have other place in the code where BUG_ON as a side-effect. > > IHMO, if (!do_something()) BUG() <=> BUG_ON. > > On the latter you know directly why it's failing, on the former you have > to look at the code. > > Regards, >
On 13/01/15 16:46, Ian Campbell wrote: >> We need to track everything for interrupt assignment to a guest/dom0. So >> if the guest ask for a free vIRQ we can give it directly. > > Makes sense. > > In that case you 0/4 mail doesn't fully describe the use case for the > series, since it talks about the dom0 PPI only. Sorry I skipped this comment by inadvertence. My cover letter was explaining the current use case, I didn't think to explain the future use case. I will update the cover letter. Regards,
On 13/01/15 17:18, Ian Campbell wrote: > On Tue, 2015-01-13 at 16:57 +0000, Julien Grall wrote: >> (CC Jan) > > I think you forget, I added him. > >>>>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d) >>>>>> { >>>>>> d->arch.phys_timer_base.offset = NOW(); >>>>>> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0); >>>>>> + >>>>>> + /* At this stage vgic_reserve_virq can't fail */ >>>>>> + if ( is_hardware_domain(d) ) >>>>>> + { >>>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI))); >>>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI))); >>>>>> + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI))); >>>>>> + } >>>>>> + else >>>>>> + { >>>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI)); >>>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI)); >>>>>> + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI)); >>>>> >>>>> Although BUG_ON is not conditional on $debug I think we still should >>>>> avoid side effects in the condition. >>>> >>>> I know, but this should never fail as it called during on domain >>>> construction. If so we may have some other issue later if we decide to >>>> assign PPI to a guest. >>>> >>>> I would prefer to keep the BUG_ON here >>> >>> I'm not objecting the the BUG_ON itself but to the fact that the >>> condition has a side effect. Please use: >>> if (!do_something()) >>> BUG() >>> instead to avoid this. >> >> We have other place in the code where BUG_ON as a side-effect. > > If we do then it is a tiny minority of places, and they are IMHO wrong. > I spotted one in the 600+ results of grepping for BUG_ON. I spotted more. Anyway, I will move to a if (!do_smth()) BUG() form. Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index de180d8..c238c8f 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -968,6 +968,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev) irq = res; DPRINT("irq %u = %u\n", i, irq); + /* + * Checking the return of vgic_reserve_virq is not + * necessary. It should not fail except when we try to map + * 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)); if ( res ) { diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index 0b3492d..416d42c 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what, printk("Additional IRQ %u (%s)\n", irq, what); + if ( !vgic_reserve_virq(d, irq) ) + printk("Failed to reserve the vIRQ %u on dom%d\n", + irq, d->domain_id); + ret = route_irq_to_guest(d, 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 75cb7ff..dbfc259 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d) return -ENODEV; } + spin_lock_init(&d->arch.vgic.lock); + d->arch.vgic.shared_irqs = xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d)); if ( d->arch.vgic.shared_irqs == NULL ) @@ -107,6 +109,15 @@ int domain_vgic_init(struct domain *d) d->arch.vgic.handler->domain_init(d); + d->arch.vgic.allocated_irqs = + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d))); + if ( !d->arch.vgic.allocated_irqs ) + return -ENOMEM; + + /* vIRQ0-15 (SGIs) are reserved */ + for (i = 0; i <= 15; i++) + set_bit(i, d->arch.vgic.allocated_irqs); + return 0; } @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d) { xfree(d->arch.vgic.shared_irqs); xfree(d->arch.vgic.pending_irqs); + xfree(d->arch.vgic.allocated_irqs); } int vcpu_vgic_init(struct vcpu *v) @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr) return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr); } +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq) +{ + bool_t reserved; + + if ( virq >= vgic_num_irqs(d) ) + return 0; + + spin_lock(&d->arch.vgic.lock); + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs); + spin_unlock(&d->arch.vgic.lock); + + return reserved; +} + +int vgic_allocate_virq(struct domain *d, bool_t spi) +{ + int ret = -1; + unsigned int virq; + + spin_lock(&d->arch.vgic.lock); + if ( !spi ) + { + virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32); + if ( virq >= 32 ) + goto unlock; + } + else + { + virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, + 32, vgic_num_irqs(d)); + if ( virq >= vgic_num_irqs(d) ) + goto unlock; + } + + set_bit(virq, d->arch.vgic.allocated_irqs); + ret = virq; + +unlock: + spin_unlock(&d->arch.vgic.lock); + + return ret; +} + +void vgic_free_virq(struct domain *d, unsigned int virq) +{ + unsigned int spi; + + if ( is_hardware_domain(d) ) + return; + + if ( virq < 32 && virq >= vgic_num_irqs(d) ) + return; + + spi = virq - 32; + + /* Taking the vGIC domain lock is not necessary. We don't care if + * the bit is cleared a bit later. What only matters is bit to 1. + * + * With this solution vgic_allocate may fail to find an vIRQ if the + * allocated_irqs is fully. But we don't care. + */ + clear_bit(spi, d->arch.vgic.allocated_irqs); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index 2e95ceb..de660bb 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d) { d->arch.phys_timer_base.offset = NOW(); d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0); + + /* At this stage vgic_reserve_virq can't fail */ + if ( is_hardware_domain(d) ) + { + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI))); + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI))); + BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI))); + } + else + { + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI)); + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI)); + BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI)); + } + return 0; } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 8b7dd85..d302fc9 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -90,6 +90,7 @@ struct arch_domain spinlock_t lock; int ctlr; int nr_spis; /* Number of SPIs */ + unsigned long *allocated_irqs; /* bitmap of IRQs allocated */ struct vgic_irq_rank *shared_irqs; /* * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 74d5a4e..9e167fa 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, unsigned long vcpu_mask); extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); + +/* Reserve a specific guest vIRQ */ +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq); + +/* + * Allocate a guest VIRQ + * - spi == 0 => allocate a PPI. It will be the same on every vCPU + * - spi == 0 => allocate an SGI + */ +extern int vgic_allocate_virq(struct domain *d, bool_t spi); + +extern void vgic_free_virq(struct domain *d, unsigned int irq); + #endif /* __ASM_ARM_VGIC_H__ */ /*
While it's easy to know which hardware IRQ is assigned to a domain, there is no way to know which IRQ is emulated by Xen for a specific domain. Introduce a bitmap to keep track of every vIRQ used by a domain. This will be used later to find free vIRQ for interrupt device assignment and emulated interrupt. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/domain_build.c | 6 +++ xen/arch/arm/platforms/xgene-storm.c | 4 ++ xen/arch/arm/vgic.c | 76 ++++++++++++++++++++++++++++++++++++ xen/arch/arm/vtimer.c | 15 +++++++ xen/include/asm-arm/domain.h | 1 + xen/include/asm-arm/vgic.h | 13 ++++++ 6 files changed, 115 insertions(+)