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 16:57:17 +0000 Message-ID: <54B54E6D.8060105@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> 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 1YB4mq-0006bX-Vr for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 16:57:49 +0000 Received: by mail-we0-f181.google.com with SMTP id q58so4064878wes.12 for ; Tue, 13 Jan 2015 08:57:47 -0800 (PST) In-Reply-To: <1421167596.19103.157.camel@citrix.com> 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: 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 (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, -- Julien Grall