[v1] xen/arm: Make PCI passthrough code non-x86 specific

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

Posted by Rahul Singh 5 years, 3 months ago

d->vm_event_paging struct is defined under CONFIG_HAS_MEM_PAGING in
sched.h but referenced in passthrough/pci.c directly.

If CONFIG_HAS_MEM_PAGING is not enabled for architecture, compiler will
throws an error.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index c6fbb7172c..3125c23e87 100644
--- 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
 
     /* device_assigned() should already have cleared the device for assignment */
     ASSERT(pcidevs_locked());
-- 
2.17.1

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

Posted by Jan Beulich 5 years, 3 months ago

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?

Jan

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

Posted by Rahul Singh 5 years, 3 months ago

Hello Jan,

> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> 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. 

> Jan

Regards,
Rahul

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

Posted by Rahul Singh 5 years, 3 months ago

Hello Jan,

> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> Hello Jan,
> 
>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> 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 ?

> 
>> Jan

Regards,
Rahul

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

Posted by Jan Beulich 5 years, 3 months ago

On 29.10.2020 17:58, Rahul Singh wrote:
>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> 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)?

Jan

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

Posted by Rahul Singh 5 years, 3 months ago

Hello Jan,

> On 29 Oct 2020, at 5:16 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.10.2020 17:58, Rahul Singh wrote:
>>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> 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