From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
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:18:16 +0000
Message-ID: <1421169496.19103.173.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>
	<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: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Ian.Campbell@citrix.com>) id 1YB59Q-0001E0-J2
	for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 17:21:08 +0000
In-Reply-To: <54B54E6D.8060105@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: tim@xen.org, stefano.stabellini@citrix.com, Jan Beulich <JBeulich@suse.com>, xen-devel@lists.xenproject.org, parth.dixit@linaro.org, christoffer.dall@linaro.org
List-Id: xen-devel@lists.xenproject.org

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.

> IHMO, if (!do_something()) BUG() <=> BUG_ON.

No, BUG_ON() is a variant of ASSERT(), with the distinction being that
the former is not only included when debug=y. It is as wrong to have a
side-effect in the BUG_ON as it is to have one in an ASSERT.

> On the latter you know directly why it's failing, on the former you have
> to look at the code.

If it's important/possible to fail then a log message would be
appropriate.

Ian.