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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Jan,

> On 17 Nov 2020, at 11:12 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.11.2020 13:25, Rahul Singh wrote:
>> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
>> for non-x86 architecture when HAS_PCI is enabled, the compiler will
>> throw an error.
>> 
>> Move code to x86 specific directory to fix compilation error.
> 
> Perhaps rather "file" than "directory”?
Ok.
> 
>> Also, modify the code to use likely() in place of unlikley() for each
>> condition to make code more optimized.
>> 
>> No functional change.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> 
> In principle I'm okay with this now, but there continue to be a few
> nits:

Thanks for reviewing the code I will fix all comments and will share the next 
version.
> 
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -23,6 +23,7 @@
>> #include <asm/hvm/io.h>
>> #include <asm/io_apic.h>
>> #include <asm/setup.h>
>> +#include <xen/vm_event.h>
> 
> Please insert this alongside the other "#include <xen/...>" higher up.

Ok.
> 
>> @@ -315,6 +316,17 @@ int iommu_update_ire_from_msi(
>>            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> 
>> +bool arch_iommu_use_permitted(const struct domain *d)
>> +{
>> +    /*
>> +     * Prevent device assign if mem paging, mem sharing or log-dirty
>> +     * have been enabled for this domain.
>> +     */
>> +    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));
>> +}
>> /*
>>  * Local variables:
>>  * mode: C
> 
> Please don't alter stylistic aspects like this trailing comment
> being preceded by a blank line.

Ok.
> 
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -381,6 +381,8 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>> extern struct spinlock iommu_pt_cleanup_lock;
>> extern struct page_list_head iommu_pt_cleanup_list;
>> 
>> +bool arch_iommu_use_permitted(const struct domain *d);
> 
> Just FTR - this way you effectively preclude an arch from
> making this a trivial static inline in one of its headers.
> 
> Jan