| Message ID | 1bb971bed3f5a56b0691fbcfd0346ae721ba049f.1603731279.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
On Mon, 26 Oct 2020, Rahul Singh 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> > --- > xen/arch/x86/Kconfig | 1 + > xen/drivers/passthrough/ats.h | 24 ++++++++++++++++++++++++ > xen/drivers/passthrough/vtd/x86/Makefile | 2 +- > xen/drivers/passthrough/x86/Makefile | 2 +- > xen/drivers/pci/Kconfig | 3 +++ > 5 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 24868aa6ad..31906e9c97 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -20,6 +20,7 @@ config X86 > select HAS_NS16550 > select HAS_PASSTHROUGH > select HAS_PCI > + select HAS_PCI_ATS > select HAS_PDX > select HAS_SCHED_GRANULARITY > select HAS_UBSAN > diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h > index 22ae209b37..a0af07b287 100644 > --- a/xen/drivers/passthrough/ats.h > +++ b/xen/drivers/passthrough/ats.h > @@ -17,6 +17,8 @@ > > #include <xen/pci_regs.h> > > +#ifdef CONFIG_HAS_PCI_ATS > + > #define ATS_REG_CAP 4 > #define ATS_REG_CTL 6 > #define ATS_QUEUE_DEPTH_MASK 0x1f > @@ -48,5 +50,27 @@ 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 > + > +static inline int enable_ats_device(struct pci_dev *pdev, > + struct list_head *ats_list) > +{ > + return -ENODEV; > +} > + > +static inline void disable_ats_device(struct pci_dev *pdev) { } > + > +static inline int pci_ats_enabled(int seg, int bus, int devfn) > +{ > + return -ENODEV; > +} pci_ats_enabled returns 0 when ATS is not enabled > +static inline int pci_ats_device(int seg, int bus, int devfn) > +{ > + return -ENODEV; also returns 0 when ATS is not enabled > +} > + > +#endif /* CONFIG_HAS_PCI_ATS */ > + > #endif /* _ATS_H_ */ > > diff --git a/xen/drivers/passthrough/vtd/x86/Makefile b/xen/drivers/passthrough/vtd/x86/Makefile > index 4ef00a4c5b..60f79fe263 100644 > --- a/xen/drivers/passthrough/vtd/x86/Makefile > +++ b/xen/drivers/passthrough/vtd/x86/Makefile > @@ -1,3 +1,3 @@ > -obj-y += ats.o > +obj-$(CONFIG_HAS_PCI_ATS) += ats.o > obj-$(CONFIG_HVM) += hvm.o > obj-y += vtd.o > diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile > index a70cf9460d..05e6f51f25 100644 > --- a/xen/drivers/passthrough/x86/Makefile > +++ b/xen/drivers/passthrough/x86/Makefile > @@ -1,2 +1,2 @@ > -obj-y += ats.o > +obj-$(CONFIG_HAS_PCI_ATS) += ats.o > obj-y += iommu.o > diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig > index 7da03fa13b..1588d4a91e 100644 > --- a/xen/drivers/pci/Kconfig > +++ b/xen/drivers/pci/Kconfig > @@ -1,3 +1,6 @@ > > config HAS_PCI > bool > + > +config HAS_PCI_ATS > + bool > -- > 2.17.1 >
On 28.10.2020 00:55, Stefano Stabellini wrote: > On Mon, 26 Oct 2020, Rahul Singh wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -20,6 +20,7 @@ config X86 >> select HAS_NS16550 >> select HAS_PASSTHROUGH >> select HAS_PCI >> + select HAS_PCI_ATS From an abstract perspective - is there a way to have a PCI subsystem where it is impossible for an ATS-capable devices to be present? I think this shouldn't be a HAS_* option, but an ordinary user selectable one. We may then even want to consider having it off by default on x86 (the command line option defaults to off as well, after all). >> @@ -48,5 +50,27 @@ 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 >> + >> +static inline int enable_ats_device(struct pci_dev *pdev, >> + struct list_head *ats_list) >> +{ >> + return -ENODEV; >> +} >> + >> +static inline void disable_ats_device(struct pci_dev *pdev) { } >> + >> +static inline int pci_ats_enabled(int seg, int bus, int devfn) >> +{ >> + return -ENODEV; >> +} > > pci_ats_enabled returns 0 when ATS is not enabled > > >> +static inline int pci_ats_device(int seg, int bus, int devfn) >> +{ >> + return -ENODEV; > > also returns 0 when ATS is not enabled And really both ought to be returning bool. Jan
On 26.10.2020 18:17, Rahul Singh wrote: > @@ -48,5 +50,27 @@ 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 > + > +static inline int enable_ats_device(struct pci_dev *pdev, > + struct list_head *ats_list) > +{ > + return -ENODEV; In addition to what Stefano has said, I don't think this is an appropriate return value here either - -EOPNOTSUPP would seem quite a bit closer to reality. Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 24868aa6ad..31906e9c97 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -20,6 +20,7 @@ config X86 select HAS_NS16550 select HAS_PASSTHROUGH select HAS_PCI + select HAS_PCI_ATS select HAS_PDX select HAS_SCHED_GRANULARITY select HAS_UBSAN diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h index 22ae209b37..a0af07b287 100644 --- a/xen/drivers/passthrough/ats.h +++ b/xen/drivers/passthrough/ats.h @@ -17,6 +17,8 @@ #include <xen/pci_regs.h> +#ifdef CONFIG_HAS_PCI_ATS + #define ATS_REG_CAP 4 #define ATS_REG_CTL 6 #define ATS_QUEUE_DEPTH_MASK 0x1f @@ -48,5 +50,27 @@ 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 + +static inline int enable_ats_device(struct pci_dev *pdev, + struct list_head *ats_list) +{ + return -ENODEV; +} + +static inline void disable_ats_device(struct pci_dev *pdev) { } + +static inline int pci_ats_enabled(int seg, int bus, int devfn) +{ + return -ENODEV; +} + +static inline int pci_ats_device(int seg, int bus, int devfn) +{ + return -ENODEV; +} + +#endif /* CONFIG_HAS_PCI_ATS */ + #endif /* _ATS_H_ */ diff --git a/xen/drivers/passthrough/vtd/x86/Makefile b/xen/drivers/passthrough/vtd/x86/Makefile index 4ef00a4c5b..60f79fe263 100644 --- a/xen/drivers/passthrough/vtd/x86/Makefile +++ b/xen/drivers/passthrough/vtd/x86/Makefile @@ -1,3 +1,3 @@ -obj-y += ats.o +obj-$(CONFIG_HAS_PCI_ATS) += ats.o obj-$(CONFIG_HVM) += hvm.o obj-y += vtd.o diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile index a70cf9460d..05e6f51f25 100644 --- a/xen/drivers/passthrough/x86/Makefile +++ b/xen/drivers/passthrough/x86/Makefile @@ -1,2 +1,2 @@ -obj-y += ats.o +obj-$(CONFIG_HAS_PCI_ATS) += ats.o obj-y += iommu.o diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig index 7da03fa13b..1588d4a91e 100644 --- a/xen/drivers/pci/Kconfig +++ b/xen/drivers/pci/Kconfig @@ -1,3 +1,6 @@ config HAS_PCI bool + +config HAS_PCI_ATS + bool
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> --- xen/arch/x86/Kconfig | 1 + xen/drivers/passthrough/ats.h | 24 ++++++++++++++++++++++++ xen/drivers/passthrough/vtd/x86/Makefile | 2 +- xen/drivers/passthrough/x86/Makefile | 2 +- xen/drivers/pci/Kconfig | 3 +++ 5 files changed, 30 insertions(+), 2 deletions(-)