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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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”

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.

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 

Please suggest us how we can proceed further on this.

> 
> Jan

Regards,
Rahul