Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Jan,

> On 29 Oct 2020, at 5:16 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 29.10.2020 17:58, Rahul Singh wrote:
>>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@xxxxxxx> wrote:
>>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 26.10.2020 18:17, Rahul Singh wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -1419,13 +1419,15 @@ 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 
>>>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
>>>>> +    /* 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) )
>>>>>       return -EXDEV;
>>>>> +#endif
>>>> 
>>>> Besides this also disabling mem-sharing and log-dirty related
>>>> logic, I don't think the change is correct: Each item being
>>>> checked needs individually disabling depending on its associated
>>>> CONFIG_*. For this, perhaps you want to introduce something
>>>> like mem_paging_enabled(d), to avoid the need for #ifdef here?
>>>> 
>>> 
>>> Ok I will fix that in next version. 
>> 
>> I just check and found out that mem-sharing , men-paging and log-dirty is 
>> x86 specific and not implemented for ARM.
>> Is that will be ok if I move above code to x86 specific directory and 
>> introduce new function arch_pcidev_is_assignable() that will test if pcidev 
>> is assignable or not ?
> 
> As an immediate workaround - perhaps (long term the individual
> pieces should still be individually checked here, as they're
> not inherently x86-specific). Since there's no device property
> involved here, the suggested name looks misleading. Perhaps
> arch_iommu_usable(d)?

Thanks. I will modify the code as per suggestion and will share the version v2 
for review.

> 
> Jan

Regards,
Rahul