Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel