From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain Date: Tue, 13 Jan 2015 16:46:36 +0000 Message-ID: <1421167596.19103.157.camel@citrix.com> References: <1418395392-30460-1-git-send-email-julien.grall@linaro.org> <1418395392-30460-3-git-send-email-julien.grall@linaro.org> <1421164286.19103.129.camel@citrix.com> <54B5476C.6010207@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YB4c6-00058f-6t for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 16:46:42 +0000 In-Reply-To: <54B5476C.6010207@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: christoffer.dall@linaro.org, xen-devel@lists.xenproject.org, tim@xen.org, parth.dixit@linaro.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On Tue, 2015-01-13 at 16:27 +0000, Julien Grall wrote: > 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. 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. > > > >> + 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: 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. > 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. > >> @@ -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. Ian.