| Message ID | 27814e614618c413ac61a9f7a48d795c557bfe5c.1604417224.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote: > > PCI ATS functionality is not enabled and tested for ARM architecture > but it is enabled for x86 and referenced in common passthrough/pci.c > code. > > Therefore introducing the new flag to enable the ATS functionality for > x86 only to avoid issues for ARM architecture. > > No functional change. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > > Changes in v2: > - Fixed return value of PCI ATS related functions when PCI_ATS is not enabled. > - Make PCI_ATS user selectable kconfig option. > > --- > xen/drivers/passthrough/ats.h | 26 ++++++++++++++++++++++++++ > xen/drivers/passthrough/x86/Makefile | 2 +- > xen/drivers/pci/Kconfig | 9 +++++++++ > 3 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h > index 22ae209b37..3a71fedcb4 100644 > --- a/xen/drivers/passthrough/ats.h > +++ b/xen/drivers/passthrough/ats.h > @@ -17,6 +17,8 @@ > > #include <xen/pci_regs.h> > > +#ifdef CONFIG_PCI_ATS > + > #define ATS_REG_CAP 4 > #define ATS_REG_CTL 6 > #define ATS_QUEUE_DEPTH_MASK 0x1f > @@ -48,5 +50,29 @@ static inline int pci_ats_device(int seg, int bus, int devfn) > return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS); > } > > +#else > + > +#define ats_enabled (false) > + > +static inline int enable_ats_device(struct pci_dev *pdev, > + struct list_head *ats_list) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline void disable_ats_device(struct pci_dev *pdev) { } > + > +static inline int pci_ats_enabled(int seg, int bus, int devfn) > +{ > + return 0; > +} > + > +static inline int pci_ats_device(int seg, int bus, int devfn) > +{ > + return 0; > +} > + > +#endif /* CONFIG_PCI_ATS */ > + > #endif /* _ATS_H_ */ > > diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile > index a70cf9460d..aa515c680d 100644 > --- a/xen/drivers/passthrough/x86/Makefile > +++ b/xen/drivers/passthrough/x86/Makefile > @@ -1,2 +1,2 @@ > -obj-y += ats.o > +obj-$(CONFIG_PCI_ATS) += ats.o > obj-y += iommu.o > diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig > index 7da03fa13b..3cb79ea954 100644 > --- 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. > -- > 2.17.1 >
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@suse.com> Jan
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@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. 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? Jan
On Wed, 4 Nov 2020, Jan Beulich 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@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. 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? If the issue is that ATS wouldn't work on ARM straight away, then I think it would be best to make this a silent option like we did in patch #1: if x86 && HAS_PCI -> automatically enable, otherwise disable. I wouldn't move the code just yet, that's better done when we can actually test it on ARM.
On 05.11.2020 22:04, Stefano Stabellini wrote: > On Wed, 4 Nov 2020, Jan Beulich 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@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. 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? > > If the issue is that ATS wouldn't work on ARM straight away, then I > think it would be best to make this a silent option like we did in patch > #1: if x86 && HAS_PCI -> automatically enable, otherwise disable. Taking the opportunity to make this a non-silent option was actually a request of mine. As long as the code builds and isn't obviously broken for Arm, I think it shouldn't have an X86 dependency (and it then should be moved up in the tree). Arguably it could then default to off for Arm, but when asking for this option to gain a prompt I also indicated that I wonder whether the default shouldn't be off on x86 as well, seeing that the controlling command line option also defaults to off. Jan
Hello Jan, > On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@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 <jbeulich@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
On 06.11.2020 12:43, Rahul Singh wrote: > Hello Jan, > >> On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@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 <jbeulich@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
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@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 <jbeulich@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 ... 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,
diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h index 22ae209b37..3a71fedcb4 100644 --- a/xen/drivers/passthrough/ats.h +++ b/xen/drivers/passthrough/ats.h @@ -17,6 +17,8 @@ #include <xen/pci_regs.h> +#ifdef CONFIG_PCI_ATS + #define ATS_REG_CAP 4 #define ATS_REG_CTL 6 #define ATS_QUEUE_DEPTH_MASK 0x1f @@ -48,5 +50,29 @@ static inline int pci_ats_device(int seg, int bus, int devfn) return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS); } +#else + +#define ats_enabled (false) + +static inline int enable_ats_device(struct pci_dev *pdev, + struct list_head *ats_list) +{ + return -EOPNOTSUPP; +} + +static inline void disable_ats_device(struct pci_dev *pdev) { } + +static inline int pci_ats_enabled(int seg, int bus, int devfn) +{ + return 0; +} + +static inline int pci_ats_device(int seg, int bus, int devfn) +{ + return 0; +} + +#endif /* CONFIG_PCI_ATS */ + #endif /* _ATS_H_ */ diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile index a70cf9460d..aa515c680d 100644 --- a/xen/drivers/passthrough/x86/Makefile +++ b/xen/drivers/passthrough/x86/Makefile @@ -1,2 +1,2 @@ -obj-y += ats.o +obj-$(CONFIG_PCI_ATS) += ats.o obj-y += iommu.o diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig index 7da03fa13b..3cb79ea954 100644 --- 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.
PCI ATS functionality is not enabled and tested for ARM architecture but it is enabled for x86 and referenced in common passthrough/pci.c code. Therefore introducing the new flag to enable the ATS functionality for x86 only to avoid issues for ARM architecture. No functional change. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v2: - Fixed return value of PCI ATS related functions when PCI_ATS is not enabled. - Make PCI_ATS user selectable kconfig option. --- xen/drivers/passthrough/ats.h | 26 ++++++++++++++++++++++++++ xen/drivers/passthrough/x86/Makefile | 2 +- xen/drivers/pci/Kconfig | 9 +++++++++ 3 files changed, 36 insertions(+), 1 deletion(-)