Re: [RFC PATCH 2/5] iommu/vt-d: Add generic IO page table support
On Thu, Nov 09, 2023 at 12:10:59AM +, Zhang, Tina wrote: > > If this is going to happen can we also convert vt-d to actually use the io > > page > > table stuff directly and shuffle the code around so it is structured like > > the rest of > > the io page table implementations? > Converting VT-d driver to use io page table involves much code > change. I made a local version of it, and it didn't prove much > benefit. Well, it structures the code in a similar way to all the other drivers, though I admit to having not looked closely at the io page table stuff. > VT-d only supports one 1st-stage IO pgtable format and one 2nd-stage > IO pgtable format. So, the current IO pgtable handling operations > seems more efficient comparing to adding another layer callbacks in > them. I would like to de-virtualize those callbacks, is is completely pointless when we have per-format iommu_domain ops now. Jason
RE: [RFC PATCH 2/5] iommu/vt-d: Add generic IO page table support
Hi Jason, > -Original Message- > From: Jason Gunthorpe > Sent: Tuesday, November 7, 2023 3:32 AM > To: Zhang, Tina > Cc: Jean-Philippe Brucker ; Tian, Kevin > ; Lu Baolu ; j...@8bytes.org; > w...@kernel.org; Liu, Yi L ; virtualization@lists.linux- > foundation.org; io...@lists.linux.dev; linux-ker...@vger.kernel.org > Subject: Re: [RFC PATCH 2/5] iommu/vt-d: Add generic IO page table support > > On Mon, Nov 06, 2023 at 02:12:23AM -0500, Tina Zhang wrote: > > Add basic hook up code to implement generic IO page table framework. > > > > Signed-off-by: Tina Zhang > > --- > > drivers/iommu/intel/Kconfig | 1 + > > drivers/iommu/intel/iommu.c | 94 > > + > > drivers/iommu/intel/iommu.h | 7 +++ > > drivers/iommu/io-pgtable.c | 3 ++ > > include/linux/io-pgtable.h | 2 + > > 5 files changed, 107 insertions(+) > > If this is going to happen can we also convert vt-d to actually use the io > page > table stuff directly and shuffle the code around so it is structured like the > rest of > the io page table implementations? Converting VT-d driver to use io page table involves much code change. I made a local version of it, and it didn't prove much benefit. VT-d only supports one 1st-stage IO pgtable format and one 2nd-stage IO pgtable format. So, the current IO pgtable handling operations seems more efficient comparing to adding another layer callbacks in them. We have a plan to add a new io_pgtable.c file under VT-d driver directory and use that file to collect IO pgtable related functions. But we want to hold on converting VT-d to use the IO page table directly unless we can see some benefits. Regards, -Tina > > Jason
Re: [PATCH RFC 00/17] Solve iommu probe races around iommu_fwspec
On Wed, Nov 08, 2023 at 06:34:58PM +, André Draszik wrote: > For me, it's working fine so far on master, and I've also done my own back > port > to 6.1 and am currently testing both. An official back port once finalised > could be useful, though :-) Great, I'll post a non-RFC version next week (LPC permitting) BTW, kbuild 0-day caught your note in the other email and a bunch of other wonky stuff I've fixed on the github version. Thanks, Jason
Re: [PATCH RFC 00/17] Solve iommu probe races around iommu_fwspec
Hi Jason, On Fri, 2023-11-03 at 13:44 -0300, Jason Gunthorpe wrote: > This is a more complete solution that the first attempt here: > https://lore.kernel.org/r/1698825902-10685-1-git-send-email-quic_zhenh...@quicinc.com > > I haven't been able to test this on any HW that touches these paths, so if > some people with HW can help get it in shape it can become non-RFC. Thank you for this series. Please note that we're also observing this issue on 6.1. I think this series is a good candidate for a back port (slightly complicated by the fact that various refactors have happened since). For me, it's working fine so far on master, and I've also done my own back port to 6.1 and am currently testing both. An official back port once finalised could be useful, though :-) Cheers, Andre'
Re: [PATCH RFC 12/17] iommu: Make iommu_ops_from_fwnode() static
Hi Jason,
On Fri, 2023-11-03 at 13:44 -0300, Jason Gunthorpe wrote:
> There are no external callers now.
>
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/iommu.c | 3 ++-
> include/linux/iommu.h | 6 --
> 2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 62c82a28cd5db3..becd1b881e62dc 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2945,7 +2945,8 @@ bool iommu_default_passthrough(void)
> }
> EXPORT_SYMBOL_GPL(iommu_default_passthrough);
>
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode)
> +static const struct iommu_ops *
> +iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> {
> const struct iommu_ops *ops = NULL;
> struct iommu_device *iommu;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 27e4605d498850..37948eee8d7394 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -701,7 +701,6 @@ static inline void iommu_fwspec_free(struct
> device *dev)
> dev->iommu->fwspec = NULL;
> }
> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode);
> int iommu_fwspec_append_ids(struct iommu_fwspec *fwspec, u32 *ids,
> int num_ids);
>
> static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct
> device *dev)
> @@ -1044,11 +1043,6 @@ static inline int iommu_fwspec_add_ids(struct
> device *dev, u32 *ids,
> }
>
> static inline
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode)
> -{
> - return NULL;
> -}
> -
> static inline int
> iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features
> feat)
This leaves the extra line with 'static inline', it should also be
removed.
Cheers,
Andre'
Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()
On Fri, Nov 03, 2023 at 01:44:46PM -0300, Jason Gunthorpe wrote: > This is not being used to pass ops, it is just a way to tell if an > iommu driver was probed. These days this can be detected directly via > device_iommu_mapped(). Call device_iommu_mapped() in the two places that > need to check it and remove the iommu parameter everywhere. > > Signed-off-by: Jason Gunthorpe > --- > arch/arc/mm/dma.c | 2 +- > arch/arm/mm/dma-mapping-nommu.c | 2 +- > arch/arm/mm/dma-mapping.c | 10 +- > arch/arm64/mm/dma-mapping.c | 4 ++-- > arch/mips/mm/dma-noncoherent.c | 2 +- > arch/riscv/mm/dma-noncoherent.c | 2 +- > drivers/acpi/scan.c | 3 +-- > drivers/hv/hv_common.c | 2 +- > drivers/of/device.c | 2 +- Acked-by: Rob Herring > include/linux/dma-map-ops.h | 4 ++-- > 10 files changed, 16 insertions(+), 17 deletions(-)
Re: [PATCH RFC 02/17] of: Do not return struct iommu_ops from of_iommu_configure()
On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote: > Nothing needs this pointer. Return a normal error code with the usual > IOMMU semantic that ENODEV means 'there is no IOMMU driver'. > > Signed-off-by: Jason Gunthorpe > --- > drivers/iommu/of_iommu.c | 29 ++--- > drivers/of/device.c | 22 +++--- Acked-by: Rob Herring > include/linux/of_iommu.h | 13 ++--- > 3 files changed, 39 insertions(+), 25 deletions(-)
Re: [PATCH RFC 03/17] of: Use -ENODEV consistently in of_iommu_configure()
On Fri, Nov 03, 2023 at 01:44:48PM -0300, Jason Gunthorpe wrote: > Instead of returning 1 and trying to handle positive error codes just > stick to the convention of returning -ENODEV. Remove references to ops > from of_iommu_configure(), a NULL ops will already generate an error code. nit: "iommu: of: ..." or "iommu/of: " for the subject. "of: ..." is generally drivers/of/. > > There is no reason to check dev->bus, if err=0 at this point then the > called configure functions thought there was an iommu and we should try to > probe it. Remove it. > > Signed-off-by: Jason Gunthorpe > --- > drivers/iommu/of_iommu.c | 42 +--- > 1 file changed, 13 insertions(+), 29 deletions(-)
Re: [PATCH RFC 17/17] iommu: Mark dev_iommu_priv_set() with a lockdep
On 2023/11/4 0:45, Jason Gunthorpe wrote: A perfect driver would only call dev_iommu_priv_set() from its probe callback. We've made it functionally correct to call it from the of_xlate by adding a lock around that call. lockdep assert that iommu_probe_device_lock is held to discourage misuse. Exclude PPC kernels with CONFIG_FSL_PAMU turned on because FSL_PAMU uses a global static for its priv and abuses priv for its domain. Remove the pointless stores of NULL, all these are on paths where the core code will free dev->iommu after the op returns. Signed-off-by: Jason Gunthorpe --- drivers/iommu/amd/iommu.c | 2 -- drivers/iommu/apple-dart.c | 1 - drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 - drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 - drivers/iommu/intel/iommu.c | 2 -- drivers/iommu/iommu.c | 9 + drivers/iommu/omap-iommu.c | 1 - include/linux/iommu.h | 5 + 8 files changed, 10 insertions(+), 12 deletions(-) Reviewed-by: Lu Baolu Best regards, baolu
Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()
On 2023/11/4 0:44, Jason Gunthorpe wrote: This is not being used to pass ops, it is just a way to tell if an iommu driver was probed. These days this can be detected directly via device_iommu_mapped(). Call device_iommu_mapped() in the two places that need to check it and remove the iommu parameter everywhere. Signed-off-by: Jason Gunthorpe --- arch/arc/mm/dma.c | 2 +- arch/arm/mm/dma-mapping-nommu.c | 2 +- arch/arm/mm/dma-mapping.c | 10 +- arch/arm64/mm/dma-mapping.c | 4 ++-- arch/mips/mm/dma-noncoherent.c | 2 +- arch/riscv/mm/dma-noncoherent.c | 2 +- drivers/acpi/scan.c | 3 +-- drivers/hv/hv_common.c | 2 +- drivers/of/device.c | 2 +- include/linux/dma-map-ops.h | 4 ++-- 10 files changed, 16 insertions(+), 17 deletions(-) Reviewed-by: Lu Baolu Best regards, baolu
