Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.11.2020 12:43, Rahul Singh wrote:
> Hello Jan,
> 
>> On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 04.11.2020 16:43, Jan Beulich wrote:
>>> On 03.11.2020 16:59, Rahul Singh wrote:
>>>> --- a/xen/drivers/pci/Kconfig
>>>> +++ b/xen/drivers/pci/Kconfig
>>>> @@ -1,3 +1,12 @@
>>>>
>>>> config HAS_PCI
>>>>    bool
>>>> +
>>>> +config PCI_ATS
>>>> +  bool "PCI ATS support"
>>>> +  default y
>>>> +  depends on X86 && HAS_PCI
>>>> +  ---help---
>>>> +   Enable PCI Address Translation Services.
>>>> +
>>>> +   If unsure, say Y.
>>>
>>> Support for "---help---" having gone away in Linux, I think we'd
>>> better not add new instances. Also indentation of help content
>>> typically is by a tab and two spaces. With these two adjusted
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Initially I wanted to merely reply indicating I'd be fine making
>> these changes while committing, but there are two more things
>> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
>> field now unused when !PCI_ATS? If so, if should get an #ifdef
>> added.
> 
> Yes, I tried to #ifdef ats field in struct pci_dev but after doing that I 
> found that I have to modify the 
> code related to x86  iotlb flush, as I have limited knowledge about how iotlb 
> flush works for 
> x86 so I decided not to modify that part of the code. 
> 
> I already compiled the x86 with !PCI_ATS and is having same behaviour as 
> command line options "ats=false” with unused struct pci_dev ats field.
> 
>> And then, what exactly is it in ats.c that's x86-specific?
>> Shouldn't the whole file instead be moved one level up, and be
>> usable by Arm right away?
> 
> Yes, you are right ats.c file is not x86 specific, but not tested for ARM so 
> I thought we will move the ats.c file to common code once ATS code is 
> tested/implemented for ARM.
> 
> disable_ats_device() is referenced in common "passthrough/pci.c" file  and 
> defined in "x86/ats.c" therefore I decided to introduce the PCI_ATS option 
> for X86. 
> As I am not sure on ARM how the ATS functionality is going to be implemented. 
> 
> There are three ways to fix the compilation error for ARM related to 
> disable_ats_device() function.
> 
> 1. Introduce the PCI_ATS config option for x86 and enabled it by default for 
> X86 and having same behaviour as  command line options for !PCi_ATS  as 
> "ats=false”

I'll be happy to see the config option retained, but that's
orthogonal to the goals here.

> 2. disable_ats_device() is called by iommu_dev_iotlb_flush_timeout () that is 
> as per my understanding is x86 specific ( not sure please confirm).
> Move iommu_dev_iotlb_flush_timeout () to "passthrough/x86/iommu.c” now and 
> move the ats.c file to common code once ATS code is tested for ARM.

While the function is currently used by VT-d code only, I again
don't see what's x86-specific about it. Hence ...

> 3. Move x86/ats.c file one level up , fixed compilation error now and if on 
> ARM platforms going to implement the ATS functionality different from
> x86 we have to move ats.c file back to x86 directory 

... I view this as the only "option" among the ones you name.

Jan