Hello Jan, > On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeul...@suse.com> 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 <jbeul...@suse.com> > > 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