From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain Date: Tue, 13 Jan 2015 17:22:52 +0000 Message-ID: <54B5546C.1080104@linaro.org> 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> <1421167596.19103.157.camel@citrix.com> <54B54E6D.8060105@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YB5Bg-00020a-GP for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 17:23:28 +0000 Received: by mail-we0-f175.google.com with SMTP id k11so4202765wes.6 for ; Tue, 13 Jan 2015 09:23:21 -0800 (PST) In-Reply-To: <54B54E6D.8060105@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: Ian Campbell Cc: tim@xen.org, stefano.stabellini@citrix.com, Jan Beulich , xen-devel@lists.xenproject.org, parth.dixit@linaro.org, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org 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, > -- Julien Grall