From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
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:35:23 +0000
Message-ID: <54B5575B.8060900@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>
	<1421169496.19103.173.camel@citrix.com>
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.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <julien.grall@linaro.org>) id 1YB5Ns-00033e-Je
	for xen-devel@lists.xenproject.org; Tue, 13 Jan 2015 17:36:04 +0000
Received: by mail-wi0-f181.google.com with SMTP id hi2so5564733wib.2
	for <xen-devel@lists.xenproject.org>;
	Tue, 13 Jan 2015 09:36:02 -0800 (PST)
In-Reply-To: <1421169496.19103.173.camel@citrix.com>
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: Ian Campbell <Ian.Campbell@citrix.com>
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 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,

-- 
Julien Grall