Re: [PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Jan,

> On 6 Nov 2020, at 9:21 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 03.11.2020 16:59, Rahul Singh wrote:
>> If mem-sharing, mem-paging and log-dirty functionality is not enabled
>> for architecture when HAS_PCI is enabled, compiler will throw an error.
> 
> Nit: Is it really "and", not "or”?

Ok yes I will fix in next version.
> 
>> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, 
>> u8 bus, u8 devfn, u32 flag)
>>     if ( !is_iommu_enabled(d) )
>>         return 0;
>> 
>> -    /* Prevent device assign if mem paging or mem sharing have been 
>> -     * enabled for this domain */
>> -    if ( d != dom_io &&
>> -         unlikely(mem_sharing_enabled(d) ||
>> -                  vm_event_check_ring(d->vm_event_paging) ||
>> -                  p2m_get_hostp2m(d)->global_logdirty) )
>> +    if( !arch_iommu_usable(d) )
>>         return -EXDEV;
> 
> While iirc I did suggest this name, seeing it used here leaves me
> somewhat unhappy with the name, albeit I also can't think of any
> better alternative right now. Maybe arch_iommu_use_permitted()?

Ok I will modify as per your suggestion.
> 
>> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
>>            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> 
>> +bool_t arch_iommu_usable(struct domain *d)
> 
> Just bool please and I very much hope the parameter can be const.

Ok I will fix in next version.
> 
>> +{
>> +
>> +    /* Prevent device assign if mem paging or mem sharing have been
>> +     * enabled for this domain */
> 
> Please correct comment style as you move it.

Ok. 
> 
>> +    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
>> +                        vm_event_check_ring(d->vm_event_paging) ||
>> +                        p2m_get_hostp2m(d)->global_logdirty) )
> 
> You've screwed up indentation, and I don't see why ...

I will fix in next version.

> 
>> +        return false;
>> +    else
>> +        return true;
>> +}
> 
> ... this can't be a simple single return statement anyway:
> 
>    return d == dom_io ||
>           likely(!mem_sharing_enabled(d) &&
>                  !vm_event_check_ring(d->vm_event_paging) &&
>                  !p2m_get_hostp2m(d)->global_logdirty);
> 
> In the course of moving I'd also suggest dropping the use of
> likely() here: The way it's used (on an && expression) isn't
> normally having much effect anyway. If anything it should imo
> be
> 
>    return d == dom_io ||
>           (likely(!mem_sharing_enabled(d)) &&
>            likely(!vm_event_check_ring(d->vm_event_paging)) &&
>            likely(!p2m_get_hostp2m(d)->global_logdirty));
> 
> Any transformation to this effect wants mentioning in the
> description, though.

Ok I will modify as per your suggestion.
> 
> Jan

Regards,
Rahul