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>]

(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

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