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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/11/2020 12:48, Jan Beulich wrote:
> 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 ...
The ATS code looks arch-agnostic. So I agree with this statement.

> > 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.
+1.

Cheers,

--
Julien Grall