Hello Jan,
> On 6 Nov 2020, at 9:21 am, Jan Beulich <[email protected]> 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
