On 06.11.2020 12:43, Rahul Singh wrote: > 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”
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