Re: [PATCH] iommu/arm-smmu-v3: Align size in __arm_smmu_tlb_inv_range

2022-04-14 Thread Nicolin Chen via iommu
On Thu, Apr 14, 2022 at 11:32:38AM +0100, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022-04-13 21:19, Nicolin Chen wrote:
> > Hi Robin,
> > 
> > On Wed, Apr 13, 2022 at 02:40:31PM +0100, Robin Murphy wrote:
> > > On 2022-04-13 05:17, Nicolin Chen wrote:
> > > > To calculate num_pages, the size should be aligned with
> > > > "page size", determined by the tg value. Otherwise, its
> > > > following "while (iova < end)" might become an infinite
> > > > loop if unaligned size is slightly greater than 1 << tg.
> > > 
> > > Hmm, how does a non-page-aligned invalidation request get generated in
> > > the first place?
> > 
> > I don't have the testing environment because it was a bug,
> > reported by a client who uses SVA feature on top of SMMU.
> > 
> > But judging from the log, the non-page-aligned inv request
> > was coming from an likely incorrect end address, e.g.
> >   { start = 0xff1, end = 0xff2 }
> > So the size turned out to be 0x10001, unaligned.
> > 
> > I don't have a full call trace on hand right now to see if
> > upper callers are doing something wrong when calculate the
> > end address, though I've asked the owner to check.
> > 
> > By looking at the call trace within arm_smmu_* functions:
> >__arm_smmu_tlb_inv_range
> >arm_smmu_tlb_inv_range_asid
> >arm_smmu_mm_invalidate_range
> >{from mm_notifier_* functions}
> > 
> > There's no address alignment check. Although I do think we
> > should fix the source who passes down the non-page-aligned
> > parameter, the SMMU driver shouldn't silently dead loop if
> > a set of unaligned inputs are given, IMHO.
> 
> Oh, sure, I'm not saying we definitely don't need to fix anything, I'd
> just like to get a better understanding of *what* we're fixing. I'd have
> (naively) expected the mm layer to give us page-aligned quantities even
> in the SVA notifier case, so if we've got a clear off-by-one somewhere
> in that path we should fix that before just blindly over-invalidating to
> paper over it;

I see. Yea, definitely should fix the source too. I've asked
the owner to track it down. I sent the change, thinking that
we could do it in parallel.

> if we still also want to be robust at the SMMU driver end
> just in case, something like "if (WARN_ON(num_pages == 0)) num_pages =
> 1;" might be more appropriate. However if it turns out that we *can*
> actually end up with unsanitised input from some userspace unmap
> interface getting this far, then a silent fixup is the best option, but
> if so I'd still like to confirm that we're rounding in the same
> direction as whoever touched the pagetables (since it can't have been us).

I see. I'll give an update once I have the debugging result.

Thanks!
Nic
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v5 1/2] PCI: ACPI: Support Microsoft's "DmaProperty"

2022-04-14 Thread Rajat Jain via iommu
Hello Bjorn,


On Thu, Apr 7, 2022 at 12:17 PM Bjorn Helgaas  wrote:
>
> In subject,
>
>   PCI/ACPI: ...
>
> would be consistent with previous history (at least things coming
> through the PCI tree :)).

Will do.

>
> On Fri, Mar 25, 2022 at 11:46:08AM -0700, Rajat Jain wrote:
> > The "DmaProperty" is supported and documented by Microsoft here:
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
>
> Here's a more specific link (could probably be referenced below to
> avoid cluttering the text here):
>
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection

Will do.

>
> > They use this property for DMA protection:
> > https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> >
> > Support the "DmaProperty" with the same semantics. This is useful for
> > internal PCI devices that do not hang off a PCIe rootport, but offer
> > an attack surface for DMA attacks (e.g. internal network devices).
>
> Same semantics as what?

Er, I meant the same semantics as the "DmaProperty". Please also see below.

>
> The MS description of "ExternalFacingPort" says:
>
>   This ACPI object enables the operating system to identify externally
>   exposed PCIe hierarchies, such as Thunderbolt.
>

No, my patch doesn't have to do with this one.

> and "DmaProperty" says:
>
>   This ACPI object enables the operating system to identify internal
>   PCIe hierarchies that are easily accessible by users (such as,
>   Laptop M.2 PCIe slots accessible by way of a latch) and require
>   protection by the OS Kernel DMA Protection mechanism.

Yes, this is the property that my patch uses. Microsoft has agreed to
update this documentation (in a sideband thread that I also copied you
on), with the updated semantics that this property can be used to
identify any PCI devices that require Kernel DMA protection. i.e. the
property is not restricted to identify "internal PCIe hierarchies"
(starting at root port), but to "any PCI device".

>
> I don't really understand why they called out "laptop M.2 PCIe slots"
> here.  Is the idea that those are more accessible than a standard
> internal PCIe slot?  Seems like a pretty small distinction to me.
>
> I can understand your example of internal network devices adding an
> attack surface.  But I don't see how "DmaProperty" helps identify
> those.  Wouldn't a NIC in a standard internal PCIe slot add the same
> attack surface?

Yes it would. The attack surface is the same. They probably only
thought of devices external to the SoC (starting from a root port)
when designing this property and thus called out internal M.2 PCI
slots. But nowhave realized that this could be opened to any PCI
device.

>
> > Signed-off-by: Rajat Jain 
> > Reviewed-by: Mika Westerberg 
> > ---
> > v5: * Reorder the patches in the series
> > v4: * Add the GUID.
> > * Update the comment and commitlog.
> > v3: * Use Microsoft's documented property "DmaProperty"
> > * Resctrict to ACPI only
> >
> >  drivers/acpi/property.c |  3 +++
> >  drivers/pci/pci-acpi.c  | 16 
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index d0986bda2964..20603cacc28d 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
> >   /* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 
> > */
> >   GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> > 0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> > + /* DmaProperty for PCI devices GUID: 
> > 70d24161-6dd5-4c9e-8070-705531292865 */
> > + GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
> > +   0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
> >  };
> >
> >  /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 1f15ab7eabf8..378e05096c52 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1350,12 +1350,28 @@ static void pci_acpi_set_external_facing(struct 
> > pci_dev *dev)
> >   dev->external_facing = 1;
> >  }
> >
> > +static void pci_acpi_check_for_dma_protection(struct pci_dev *dev)
>
> I try to avoid function names like *_check_*() because they don't give
> any hint about whether there's a side effect or what direction things
> are going.  I prefer things that return a value or make sense when
> used as a predicate.  Maybe something like this?
>
>   int pci_dev_has_dma_property(struct pci_dev *dev)
>
>   dev->untrusted |= pci_dev_has_dma_property(pci_dev);
>

OK, will do.


> > +{
> > + u8 val;
> > +
> > + /*
> > +  * Property also used by Microsoft Windows for same purpose,
> > +  * (to implement DMA protection from a device, using the IOMMU).
> > +   

Re: [PATCH 00/13] iommu: Retire bus_set_iommu()

2022-04-14 Thread Marek Szyprowski
Hi Robin,

On 14.04.2022 14:42, Robin Murphy wrote:
> Hi all,
>
> Here's another chapter in my saga of moving to per-instance IOMMU ops -
> iommu_present() and iommu_capable() cleanups will be ongoing for another
> cycle or two, while this one is at least self-contained within the
> subsystem. The next steps after this are making iommu_domain_alloc()
> instance-aware - which should finish the public API - and pulling the
> fwnode/of_xlate bits into __iommu_probe_device(). And then making sense
> of whatever's left :)
>
> For ease of review here I split out individual driver patches based on
> whether there was any non-trivial change or affect on control flow; the
> straightforward deletions are all lumped together since the whole series
> needs applying together either way, but I'm happy to split the final
> patch up further if anyone would like.
>
> Patch #3 for AMD is based on Mario's SWIOTLB patch here:
>
> https://lore.kernel.org/linux-iommu/20220404204723.9767-1-mario.limoncie...@amd.com/
>
> since that wants merging first as fix material. The series is also based
> contextually (but not functionally) on my device_iommu_capable() patches
> here:
>
> https://lore.kernel.org/linux-iommu/cover.1649089693.git.robin.mur...@arm.com/
>
> since those are pretty much good to go now (I'll send a slightly-tweaked
> final version once the iommu/core branch is open).

Works fine on Samsung Exynos based boards (both, ARM 32bit and 64bit).

Tested-by: Marek Szyprowski  # for Exynos 
relevant changes


>
> Thanks,
> Robin.
>
>
> Robin Murphy (13):
>iommu: Always register bus notifiers
>iommu: Move bus setup to IOMMU device registration
>iommu/amd: Clean up bus_set_iommu()
>iommu/arm-smmu: Clean up bus_set_iommu()
>iommu/arm-smmu-v3: Clean up bus_set_iommu()
>iommu/dart: Clean up bus_set_iommu()
>iommu/exynos: Clean up bus_set_iommu()
>iommu/ipmmu-vmsa: Clean up bus_set_iommu()
>iommu/mtk: Clean up bus_set_iommu()
>iommu/omap: Clean up bus_set_iommu()
>iommu/tegra-smmu: Clean up bus_set_iommu()
>iommu/virtio: Clean up bus_set_iommu()
>iommu: Clean up bus_set_iommu()
>
>   drivers/iommu/amd/amd_iommu.h   |   1 -
>   drivers/iommu/amd/init.c|   9 +-
>   drivers/iommu/amd/iommu.c   |  21 
>   drivers/iommu/apple-dart.c  |  30 +-
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  53 +-
>   drivers/iommu/arm/arm-smmu/arm-smmu.c   |  84 +--
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c |   4 -
>   drivers/iommu/exynos-iommu.c|   9 --
>   drivers/iommu/fsl_pamu_domain.c |   4 -
>   drivers/iommu/intel/iommu.c |   1 -
>   drivers/iommu/iommu.c   | 109 +---
>   drivers/iommu/ipmmu-vmsa.c  |  35 +--
>   drivers/iommu/msm_iommu.c   |   2 -
>   drivers/iommu/mtk_iommu.c   |  13 +--
>   drivers/iommu/mtk_iommu_v1.c|  13 +--
>   drivers/iommu/omap-iommu.c  |   6 --
>   drivers/iommu/rockchip-iommu.c  |   2 -
>   drivers/iommu/s390-iommu.c  |   6 --
>   drivers/iommu/sprd-iommu.c  |   5 -
>   drivers/iommu/sun50i-iommu.c|   2 -
>   drivers/iommu/tegra-smmu.c  |  29 ++
>   drivers/iommu/virtio-iommu.c|  24 -
>   include/linux/iommu.h   |   1 -
>   23 files changed, 62 insertions(+), 401 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R Institute Poland

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 5.4 238/475] dma-debug: fix return value of __setup handlers

2022-04-14 Thread Greg Kroah-Hartman
From: Randy Dunlap 

[ Upstream commit 80e4390981618e290616dbd06ea190d4576f219d ]

When valid kernel command line parameters
  dma_debug=off dma_debug_entries=100
are used, they are reported as Unknown parameters and added to init's
environment strings, polluting it.

  Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc5
dma_debug=off dma_debug_entries=100", will be passed to user space.

and

 Run /sbin/init as init process
   with arguments:
 /sbin/init
   with environment:
 HOME=/
 TERM=linux
 BOOT_IMAGE=/boot/bzImage-517rc5
 dma_debug=off
 dma_debug_entries=100

Return 1 from these __setup handlers to indicate that the command line
option has been handled.

Fixes: 59d3daafa1726 ("dma-debug: add kernel command line parameters")
Signed-off-by: Randy Dunlap 
Reported-by: Igor Zhbanov 
Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0de...@omprussia.ru
Cc: Joerg Roedel 
Cc: Christoph Hellwig 
Cc: Marek Szyprowski 
Cc: iommu@lists.linux-foundation.org
Cc: Robin Murphy 
Signed-off-by: Christoph Hellwig 
Signed-off-by: Sasha Levin 
---
 kernel/dma/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index b28665f4d8c7..4dc3bbfd3e3f 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -980,7 +980,7 @@ static __init int dma_debug_cmdline(char *str)
global_disable = true;
}
 
-   return 0;
+   return 1;
 }
 
 static __init int dma_debug_entries_cmdline(char *str)
@@ -989,7 +989,7 @@ static __init int dma_debug_entries_cmdline(char *str)
return -EINVAL;
if (!get_option(, _prealloc_entries))
nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
-   return 0;
+   return 1;
 }
 
 __setup("dma_debug=", dma_debug_cmdline);
-- 
2.34.1



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 4.19 165/338] dma-debug: fix return value of __setup handlers

2022-04-14 Thread Greg Kroah-Hartman
From: Randy Dunlap 

[ Upstream commit 80e4390981618e290616dbd06ea190d4576f219d ]

When valid kernel command line parameters
  dma_debug=off dma_debug_entries=100
are used, they are reported as Unknown parameters and added to init's
environment strings, polluting it.

  Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc5
dma_debug=off dma_debug_entries=100", will be passed to user space.

and

 Run /sbin/init as init process
   with arguments:
 /sbin/init
   with environment:
 HOME=/
 TERM=linux
 BOOT_IMAGE=/boot/bzImage-517rc5
 dma_debug=off
 dma_debug_entries=100

Return 1 from these __setup handlers to indicate that the command line
option has been handled.

Fixes: 59d3daafa1726 ("dma-debug: add kernel command line parameters")
Signed-off-by: Randy Dunlap 
Reported-by: Igor Zhbanov 
Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0de...@omprussia.ru
Cc: Joerg Roedel 
Cc: Christoph Hellwig 
Cc: Marek Szyprowski 
Cc: iommu@lists.linux-foundation.org
Cc: Robin Murphy 
Signed-off-by: Christoph Hellwig 
Signed-off-by: Sasha Levin 
---
 kernel/dma/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 1c82b0d25498..9c9a5b12f92f 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -1056,7 +1056,7 @@ static __init int dma_debug_cmdline(char *str)
global_disable = true;
}
 
-   return 0;
+   return 1;
 }
 
 static __init int dma_debug_entries_cmdline(char *str)
@@ -1065,7 +1065,7 @@ static __init int dma_debug_entries_cmdline(char *str)
return -EINVAL;
if (!get_option(, _prealloc_entries))
nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
-   return 0;
+   return 1;
 }
 
 __setup("dma_debug=", dma_debug_cmdline);
-- 
2.34.1



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 0/7] Add support for HiSilicon PCIe Tune and Trace device

2022-04-14 Thread Yicong Yang via iommu
Hi Bjorn,

Since it's a device about tuning and analyzing PCIe link in your realm and 
you've given
helpful comments in RFC and v1 version, looking forward to see your opnion on 
this
driver as the user interface has changed to perf. Also to confirm that the 
hotplug problem
mentioned in RFC[1] has been resolved in a proper way.

[1] 
https://lore.kernel.org/linux-pci/20200710230913.GA90375@bjorn-Precision-5520/
Thanks.

On 2022/4/7 20:58, Yicong Yang wrote:
> HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex
> integrated Endpoint (RCiEP) device, providing the capability
> to dynamically monitor and tune the PCIe traffic (tune),
> and trace the TLP headers (trace).
> 
> PTT tune is designed for monitoring and adjusting PCIe link parameters.
> We provide several parameters of the PCIe link. Through the driver,
> user can adjust the value of certain parameter to affect the PCIe link
> for the purpose of enhancing the performance in certian situation.
> 
> PTT trace is designed for dumping the TLP headers to the memory, which
> can be used to analyze the transactions and usage condition of the PCIe
> Link. Users can choose filters to trace headers, by either requester
> ID, or those downstream of a set of Root Ports on the same core of the
> PTT device. It's also supported to trace the headers of certain type and
> of certain direction.
> 
> The driver registers a PMU device for each PTT device. The trace can
> be used through `perf record` and the traced headers can be decoded
> by `perf report`. The perf command support for the device is also
> added in this patchset. The tune can be used through the sysfs
> attributes of related PMU device. See the documentation for the
> detailed usage.
> 
> Change since v6:
> - Fix W=1 errors reported by lkp test, thanks
> 
> Change since v5:
> - Squash the PMU patch into PATCH 2 suggested by John
> - refine the commit message of PATCH 1 and some comments
> Link: 
> https://lore.kernel.org/lkml/20220308084930.5142-1-yangyic...@hisilicon.com/
> 
> Change since v4:
> Address the comments from Jonathan, John and Ma Ca, thanks.
> - Use devm* also for allocating the DMA buffers
> - Remove the IRQ handler stub in Patch 2
> - Make functions waiting for hardware state return boolean
> - Manual remove the PMU device as it should be removed first
> - Modifier the orders in probe and removal to make them matched well
> - Make available {directions,type,format} array const and non-global
> - Using the right filter list in filters show and well protect the
>   list with mutex
> - Record the trace status with a boolean @started rather than enum
> - Optimize the process of finding the PTT devices of the perf-tool
> Link: 
> https://lore.kernel.org/linux-pci/20220221084307.33712-1-yangyic...@hisilicon.com/
> 
> Change since v3:
> Address the comments from Jonathan and John, thanks.
> - drop members in the common struct which can be get on the fly
> - reduce buffer struct and organize the buffers with array instead of list
> - reduce the DMA reset wait time to avoid long time busy loop
> - split the available_filters sysfs attribute into two files, for root port
>   and requester respectively. Update the documentation accordingly
> - make IOMMU mapping check earlier in probe to avoid race condition. Also
>   make IOMMU quirk patch prior to driver in the series
> - Cleanups and typos fixes from John and Jonathan
> Link: 
> https://lore.kernel.org/linux-pci/20220124131118.17887-1-yangyic...@hisilicon.com/
> 
> Change since v2:
> - address the comments from Mathieu, thanks.
>   - rename the directory to ptt to match the function of the device
>   - spinoff the declarations to a separate header
>   - split the trace function to several patches
>   - some other comments.
> - make default smmu domain type of PTT device to identity
>   Drop the RMR as it's not recommended and use an iommu_def_domain_type
>   quirk to passthrough the device DMA as suggested by Robin. 
> Link: 
> https://lore.kernel.org/linux-pci/2026090625.53702-1-yangyic...@hisilicon.com/
> 
> Change since v1:
> - switch the user interface of trace to perf from debugfs
> - switch the user interface of tune to sysfs from debugfs
> - add perf tool support to start trace and decode the trace data
> - address the comments of documentation from Bjorn
> - add RMR[1] support of the device as trace works in RMR mode or
>   direct DMA mode. RMR support is achieved by common APIs rather
>   than the APIs implemented in [1].
> Link: 
> https://lore.kernel.org/lkml/1618654631-42454-1-git-send-email-yangyic...@hisilicon.com/
> [1] 
> https://lore.kernel.org/linux-acpi/20210805080724.480-1-shameerali.kolothum.th...@huawei.com/
> 
> Qi Liu (1):
>   perf tool: Add support for HiSilicon PCIe Tune and Trace device driver
> 
> Yicong Yang (6):
>   iommu/arm-smmu-v3: Make default domain type of HiSilicon PTT device to
> identity
>   hwtracing: Add trace function support for HiSilicon PCIe Tune and
> Trace device
>   

Re: [PATCH v7 5/7] perf tool: Add support for HiSilicon PCIe Tune and Trace device driver

2022-04-14 Thread John Garry via iommu

On 12/04/2022 08:41, Yicong Yang wrote:

+    hisi_ptt_pmus = zalloc(sizeof(struct perf_pmu *) * (*nr_ptts));
+    if (!hisi_ptt_pmus) {
+    pr_err("hisi_ptt alloc failed\n");
+    *err = -ENOMEM;

using PTR_ERR seems better, if possible


ok will change to that. *err = -ENOMEM is used here to keep consistence with
what spe does.



Ah, I see that we are contrained by the interface of 
auxtrace_record_init() to pass err as a pointer, so I suppose the code 
in this patch is ok to fit into that.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] habanalabs: Stop using iommu_present()

2022-04-14 Thread Oded Gabbay
On Mon, Apr 11, 2022 at 3:36 PM Robin Murphy  wrote:
>
> Even if an IOMMU might be present for some PCI segment in the system,
> that doesn't necessarily mean it provides translation for the device
> we care about. Replace iommu_present() with a more appropriate check.
>
> Signed-off-by: Robin Murphy 
> ---
>
> v2: Rebase on habanalabs-next
>
>  drivers/misc/habanalabs/common/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/habanalabs/common/debugfs.c 
> b/drivers/misc/habanalabs/common/debugfs.c
> index 7c4a4d504e4c..a94f01713efd 100644
> --- a/drivers/misc/habanalabs/common/debugfs.c
> +++ b/drivers/misc/habanalabs/common/debugfs.c
> @@ -722,7 +722,7 @@ static int hl_access_mem(struct hl_device *hdev, u64 
> addr, u64 *val,
> if (found)
> return 0;
>
> -   if (!user_address || iommu_present(_bus_type)) {
> +   if (!user_address || device_iommu_mapped(>pdev->dev)) {
> rc = -EINVAL;
> goto err;
> }
> --
> 2.28.0.dirty
>
Reviewed-by: Oded Gabbay 
Applied to -next.
Thanks,
Oded
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 13/13] iommu: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Clean up the remaining trivial bus_set_iommu() callsites along
with the implementation. Now drivers only have to know and care
about iommu_device instances, phew!

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c |  4 
 drivers/iommu/fsl_pamu_domain.c |  4 
 drivers/iommu/intel/iommu.c |  1 -
 drivers/iommu/iommu.c   | 24 
 drivers/iommu/msm_iommu.c   |  2 --
 drivers/iommu/rockchip-iommu.c  |  2 --
 drivers/iommu/s390-iommu.c  |  6 --
 drivers/iommu/sprd-iommu.c  |  5 -
 drivers/iommu/sun50i-iommu.c|  2 --
 include/linux/iommu.h   |  1 -
 10 files changed, 51 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c 
b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 8cd39abade5a..028649203d33 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -845,8 +845,6 @@ static int qcom_iommu_device_probe(struct platform_device 
*pdev)
goto err_pm_disable;
}
 
-   bus_set_iommu(_bus_type, _iommu_ops);
-
if (qcom_iommu->local_base) {
pm_runtime_get_sync(dev);
writel_relaxed(0x, qcom_iommu->local_base + 
SMMU_INTR_SEL_NS);
@@ -864,8 +862,6 @@ static int qcom_iommu_device_remove(struct platform_device 
*pdev)
 {
struct qcom_iommu_dev *qcom_iommu = platform_get_drvdata(pdev);
 
-   bus_set_iommu(_bus_type, NULL);
-
pm_runtime_force_suspend(>dev);
platform_set_drvdata(pdev, NULL);
iommu_device_sysfs_remove(_iommu->iommu);
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index d03a14386f86..ddf5ab28615c 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -480,11 +480,7 @@ int __init pamu_domain_init(void)
if (ret) {
iommu_device_sysfs_remove(_iommu);
pr_err("Can't register iommu device\n");
-   return ret;
}
 
-   bus_set_iommu(_bus_type, _pamu_ops);
-   bus_set_iommu(_bus_type, _pamu_ops);
-
return ret;
 }
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 0b24c4977dbe..49d552a96098 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4178,7 +4178,6 @@ int __init intel_iommu_init(void)
}
up_read(_global_lock);
 
-   bus_set_iommu(_bus_type, _iommu_ops);
if (si_domain && !hw_pass_through)
register_memory_notifier(_iommu_memory_nb);
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 51205c33c426..7800e342d285 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1885,30 +1885,6 @@ static int iommu_bus_init(struct bus_type *bus)
return err;
 }
 
-/**
- * bus_set_iommu - set iommu-callbacks for the bus
- * @bus: bus.
- * @ops: the callbacks provided by the iommu-driver
- *
- * This function is called by an iommu driver to set the iommu methods
- * used for a particular bus. Drivers for devices on that bus can use
- * the iommu-api after these ops are registered.
- * This special function is needed because IOMMUs are usually devices on
- * the bus itself, so the iommu drivers are not initialized when the bus
- * is set up. With this function the iommu-driver can set the iommu-ops
- * afterwards.
- */
-int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
-{
-   if (bus->iommu_ops && ops && bus->iommu_ops != ops)
-   return -EBUSY;
-
-   bus->iommu_ops = ops;
-
-   return 0;
-}
-EXPORT_SYMBOL_GPL(bus_set_iommu);
-
 bool iommu_present(struct bus_type *bus)
 {
return bus->iommu_ops != NULL;
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 50f57624610f..5b89fb16feb8 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -789,8 +789,6 @@ static int msm_iommu_probe(struct platform_device *pdev)
goto fail;
}
 
-   bus_set_iommu(_bus_type, _iommu_ops);
-
pr_info("device mapped at %p, irq %d with %d ctx banks\n",
iommu->base, iommu->irq, iommu->ncb);
 
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ab57c4b8fade..a3fc59b814ab 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1300,8 +1300,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (!dma_dev)
dma_dev = >dev;
 
-   bus_set_iommu(_bus_type, _iommu_ops);
-
pm_runtime_enable(dev);
 
for (i = 0; i < iommu->num_irq; i++) {
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index bd56e21bf754..ea4ba9de04af 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -376,9 +376,3 @@ static const struct iommu_ops s390_iommu_ops = {
.free   = s390_domain_free,

[PATCH 12/13] iommu/virtio: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/virtio-iommu.c | 24 
 1 file changed, 24 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 25be4b822aa0..371f8657c0ce 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -7,7 +7,6 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include 
 #include 
 #include 
 #include 
@@ -1146,26 +1145,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
iommu_device_register(>iommu, _ops, parent_dev);
 
-#ifdef CONFIG_PCI
-   if (pci_bus_type.iommu_ops != _ops) {
-   ret = bus_set_iommu(_bus_type, _ops);
-   if (ret)
-   goto err_unregister;
-   }
-#endif
-#ifdef CONFIG_ARM_AMBA
-   if (amba_bustype.iommu_ops != _ops) {
-   ret = bus_set_iommu(_bustype, _ops);
-   if (ret)
-   goto err_unregister;
-   }
-#endif
-   if (platform_bus_type.iommu_ops != _ops) {
-   ret = bus_set_iommu(_bus_type, _ops);
-   if (ret)
-   goto err_unregister;
-   }
-
vdev->priv = viommu;
 
dev_info(dev, "input address: %u bits\n",
@@ -1174,9 +1153,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
return 0;
 
-err_unregister:
-   iommu_device_sysfs_remove(>iommu);
-   iommu_device_unregister(>iommu);
 err_free_vqs:
vdev->config->del_vqs(vdev);
 
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 11/13] iommu/tegra-smmu: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/tegra-smmu.c | 29 ++---
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1fea68e551f1..2e4d2e4c65bb 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1086,8 +1086,8 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
/*
 * This is a bit of a hack. Ideally we'd want to simply return this
-* value. However the IOMMU registration process will attempt to add
-* all devices to the IOMMU when bus_set_iommu() is called. In order
+* value. However iommu_device_register() will attempt to add
+* all devices to the IOMMU before we get that far. In order
 * not to rely on global variables to track the IOMMU instance, we
 * set it here so that it can be looked up from the .probe_device()
 * callback via the IOMMU device's .drvdata field.
@@ -1141,32 +1141,15 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
return ERR_PTR(err);
 
err = iommu_device_register(>iommu, _smmu_ops, dev);
-   if (err)
-   goto remove_sysfs;
-
-   err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0)
-   goto unregister;
-
-#ifdef CONFIG_PCI
-   err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0)
-   goto unset_platform_bus;
-#endif
+   if (err) {
+   iommu_device_sysfs_remove(>iommu);
+   return ERR_PTR(err);
+   }
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
-
-unset_platform_bus: __maybe_unused;
-   bus_set_iommu(_bus_type, NULL);
-unregister:
-   iommu_device_unregister(>iommu);
-remove_sysfs:
-   iommu_device_sysfs_remove(>iommu);
-
-   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 10/13] iommu/omap: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/omap-iommu.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..07ee2600113c 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1776,14 +1776,8 @@ static int __init omap_iommu_init(void)
goto fail_driver;
}
 
-   ret = bus_set_iommu(_bus_type, _iommu_ops);
-   if (ret)
-   goto fail_bus;
-
return 0;
 
-fail_bus:
-   platform_driver_unregister(_iommu_driver);
 fail_driver:
kmem_cache_destroy(iopte_cachep);
return ret;
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 09/13] iommu/mtk: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure paths accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/mtk_iommu.c| 13 +
 drivers/iommu/mtk_iommu_v1.c | 13 +
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6fd75a60abd6..4278d9e032ad 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -920,19 +920,11 @@ static int mtk_iommu_probe(struct platform_device *pdev)
spin_lock_init(>tlb_lock);
list_add_tail(>list, );
 
-   if (!iommu_present(_bus_type)) {
-   ret = bus_set_iommu(_bus_type, _iommu_ops);
-   if (ret)
-   goto out_list_del;
-   }
-
ret = component_master_add_with_match(dev, _iommu_com_ops, match);
if (ret)
-   goto out_bus_set_null;
+   goto out_list_del;
return ret;
 
-out_bus_set_null:
-   bus_set_iommu(_bus_type, NULL);
 out_list_del:
list_del(>list);
iommu_device_unregister(>iommu);
@@ -952,9 +944,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)
iommu_device_sysfs_remove(>iommu);
iommu_device_unregister(>iommu);
 
-   if (iommu_present(_bus_type))
-   bus_set_iommu(_bus_type, NULL);
-
clk_disable_unprepare(data->bclk);
device_link_remove(data->smicomm_dev, >dev);
pm_runtime_disable(>dev);
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index ecff800656e6..7d17d6a21803 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -660,19 +660,11 @@ static int mtk_iommu_probe(struct platform_device *pdev)
if (ret)
goto out_sysfs_remove;
 
-   if (!iommu_present(_bus_type)) {
-   ret = bus_set_iommu(_bus_type,  _iommu_ops);
-   if (ret)
-   goto out_dev_unreg;
-   }
-
ret = component_master_add_with_match(dev, _iommu_com_ops, match);
if (ret)
-   goto out_bus_set_null;
+   goto out_dev_unreg;
return ret;
 
-out_bus_set_null:
-   bus_set_iommu(_bus_type, NULL);
 out_dev_unreg:
iommu_device_unregister(>iommu);
 out_sysfs_remove:
@@ -687,9 +679,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)
iommu_device_sysfs_remove(>iommu);
iommu_device_unregister(>iommu);
 
-   if (iommu_present(_bus_type))
-   bus_set_iommu(_bus_type, NULL);
-
clk_disable_unprepare(data->bclk);
devm_free_irq(>dev, data->irq, data);
component_master_del(>dev, _iommu_com_ops);
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 08/13] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary. This also
leaves the custom initcall effectively doing nothing but register
the driver, which no longer needs to happen early either, so convert
it to builtin_platform_driver().

Signed-off-by: Robin Murphy 
---
 drivers/iommu/ipmmu-vmsa.c | 35 +--
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..2549d32f0ddd 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev)
ret = iommu_device_register(>iommu, _ops, 
>dev);
if (ret)
return ret;
-
-#if defined(CONFIG_IOMMU_DMA)
-   if (!iommu_present(_bus_type))
-   bus_set_iommu(_bus_type, _ops);
-#endif
}
 
/*
@@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = {
.probe = ipmmu_probe,
.remove = ipmmu_remove,
 };
-
-static int __init ipmmu_init(void)
-{
-   struct device_node *np;
-   static bool setup_done;
-   int ret;
-
-   if (setup_done)
-   return 0;
-
-   np = of_find_matching_node(NULL, ipmmu_of_ids);
-   if (!np)
-   return 0;
-
-   of_node_put(np);
-
-   ret = platform_driver_register(_driver);
-   if (ret < 0)
-   return ret;
-
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
-   if (!iommu_present(_bus_type))
-   bus_set_iommu(_bus_type, _ops);
-#endif
-
-   setup_done = true;
-   return 0;
-}
-subsys_initcall(ipmmu_init);
+builtin_platform_driver(ipmmu_driver);
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 07/13] iommu/exynos: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/exynos-iommu.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 71f2018e23fe..359b255b3924 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1356,16 +1356,7 @@ static int __init exynos_iommu_init(void)
goto err_zero_lv2;
}
 
-   ret = bus_set_iommu(_bus_type, _iommu_ops);
-   if (ret) {
-   pr_err("%s: Failed to register exynos-iommu driver.\n",
-   __func__);
-   goto err_set_iommu;
-   }
-
return 0;
-err_set_iommu:
-   kmem_cache_free(lv2table_kmem_cache, zero_lv2_table);
 err_zero_lv2:
platform_driver_unregister(_sysmmu_driver);
 err_reg_driver:
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 06/13] iommu/dart: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/apple-dart.c | 30 +-
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index decafb07ad08..a679e4c02291 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -823,27 +823,6 @@ static irqreturn_t apple_dart_irq(int irq, void *dev)
return IRQ_HANDLED;
 }
 
-static int apple_dart_set_bus_ops(const struct iommu_ops *ops)
-{
-   int ret;
-
-   if (!iommu_present(_bus_type)) {
-   ret = bus_set_iommu(_bus_type, ops);
-   if (ret)
-   return ret;
-   }
-#ifdef CONFIG_PCI
-   if (!iommu_present(_bus_type)) {
-   ret = bus_set_iommu(_bus_type, ops);
-   if (ret) {
-   bus_set_iommu(_bus_type, NULL);
-   return ret;
-   }
-   }
-#endif
-   return 0;
-}
-
 static int apple_dart_probe(struct platform_device *pdev)
 {
int ret;
@@ -899,14 +878,10 @@ static int apple_dart_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, dart);
 
-   ret = apple_dart_set_bus_ops(_dart_iommu_ops);
-   if (ret)
-   goto err_free_irq;
-
ret = iommu_device_sysfs_add(>iommu, dev, NULL, "apple-dart.%s",
 dev_name(>dev));
if (ret)
-   goto err_remove_bus_ops;
+   goto err_free_irq;
 
ret = iommu_device_register(>iommu, _dart_iommu_ops, dev);
if (ret)
@@ -920,8 +895,6 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 err_sysfs_remove:
iommu_device_sysfs_remove(>iommu);
-err_remove_bus_ops:
-   apple_dart_set_bus_ops(NULL);
 err_free_irq:
free_irq(dart->irq, dart);
 err_clk_disable:
@@ -936,7 +909,6 @@ static int apple_dart_remove(struct platform_device *pdev)
 
apple_dart_hw_reset(dart);
free_irq(dart->irq, dart);
-   apple_dart_set_bus_ops(NULL);
 
iommu_device_unregister(>iommu);
iommu_device_sysfs_remove(>iommu);
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 05/13] iommu/arm-smmu-v3: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +
 1 file changed, 2 insertions(+), 51 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6699333fd17b..b221525c31b9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -28,8 +28,6 @@
 #include 
 #include 
 
-#include 
-
 #include "arm-smmu-v3.h"
 #include "../../iommu-sva-lib.h"
 
@@ -3698,43 +3696,6 @@ static unsigned long arm_smmu_resource_size(struct 
arm_smmu_device *smmu)
return SZ_128K;
 }
 
-static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
-{
-   int err;
-
-#ifdef CONFIG_PCI
-   if (pci_bus_type.iommu_ops != ops) {
-   err = bus_set_iommu(_bus_type, ops);
-   if (err)
-   return err;
-   }
-#endif
-#ifdef CONFIG_ARM_AMBA
-   if (amba_bustype.iommu_ops != ops) {
-   err = bus_set_iommu(_bustype, ops);
-   if (err)
-   goto err_reset_pci_ops;
-   }
-#endif
-   if (platform_bus_type.iommu_ops != ops) {
-   err = bus_set_iommu(_bus_type, ops);
-   if (err)
-   goto err_reset_amba_ops;
-   }
-
-   return 0;
-
-err_reset_amba_ops:
-#ifdef CONFIG_ARM_AMBA
-   bus_set_iommu(_bustype, NULL);
-#endif
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-   bus_set_iommu(_bus_type, NULL);
-#endif
-   return err;
-}
-
 static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t 
start,
  resource_size_t size)
 {
@@ -3838,27 +3799,17 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
ret = iommu_device_register(>iommu, _smmu_ops, dev);
if (ret) {
dev_err(dev, "Failed to register iommu\n");
-   goto err_sysfs_remove;
+   iommu_device_sysfs_remove(>iommu);
+   return ret;
}
 
-   ret = arm_smmu_set_bus_ops(_smmu_ops);
-   if (ret)
-   goto err_unregister_device;
-
return 0;
-
-err_unregister_device:
-   iommu_device_unregister(>iommu);
-err_sysfs_remove:
-   iommu_device_sysfs_remove(>iommu);
-   return ret;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
-   arm_smmu_set_bus_ops(NULL);
iommu_device_unregister(>iommu);
iommu_device_sysfs_remove(>iommu);
arm_smmu_device_disable(smmu);
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 03/13] iommu/amd: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary, and
garbage-collect the last remnants of amd_iommu_init_api().

Signed-off-by: Robin Murphy 
---
 drivers/iommu/amd/amd_iommu.h |  1 -
 drivers/iommu/amd/init.c  |  9 +
 drivers/iommu/amd/iommu.c | 21 -
 3 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 1ab31074f5b3..384393ce57fb 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -18,7 +18,6 @@ extern void amd_iommu_restart_event_logging(struct amd_iommu 
*iommu);
 extern int amd_iommu_init_devices(void);
 extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
-extern int amd_iommu_init_api(void);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 0467918bf7fd..1cb10d8b0df4 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1970,20 +1970,13 @@ static int __init amd_iommu_init_pci(void)
/*
 * Order is important here to make sure any unity map requirements are
 * fulfilled. The unity mappings are created and written to the device
-* table during the amd_iommu_init_api() call.
+* table during the iommu_init_pci() call.
 *
 * After that we call init_device_table_dma() to make sure any
 * uninitialized DTE will block DMA, and in the end we flush the caches
 * of all IOMMUs to make sure the changes to the device table are
 * active.
 */
-   ret = amd_iommu_init_api();
-   if (ret) {
-   pr_err("IOMMU: Failed to initialize IOMMU-API interface 
(error=%d)!\n",
-  ret);
-   goto out;
-   }
-
init_device_table_dma();
 
for_each_iommu(iommu)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6366a473ef0d..c0f8a541a7d6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -11,8 +11,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 #include 
 #include 
@@ -1838,25 +1836,6 @@ void amd_iommu_domain_update(struct protection_domain 
*domain)
amd_iommu_domain_flush_complete(domain);
 }
 
-int __init amd_iommu_init_api(void)
-{
-   int err;
-
-   err = bus_set_iommu(_bus_type, _iommu_ops);
-   if (err)
-   return err;
-#ifdef CONFIG_ARM_AMBA
-   err = bus_set_iommu(_bustype, _iommu_ops);
-   if (err)
-   return err;
-#endif
-   err = bus_set_iommu(_bus_type, _iommu_ops);
-   if (err)
-   return err;
-
-   return 0;
-}
-
 /*
  *
  * The following functions belong to the exported interface of AMD IOMMU
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 04/13] iommu/arm-smmu: Clean up bus_set_iommu()

2022-04-14 Thread Robin Murphy
Stop calling bus_set_iommu() since it's now unnecessary. With device
probes now replayed for every IOMMU instance registration, the whole
sorry ordering workaround for legacy DT bindings goes too, hooray!

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +--
 1 file changed, 2 insertions(+), 82 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c 
b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 52bd42d80b4f..34cab56b9c6d 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -37,7 +37,6 @@
 #include 
 #include 
 
-#include 
 #include 
 
 #include "arm-smmu.h"
@@ -93,8 +92,6 @@ static struct platform_driver arm_smmu_driver;
 static struct iommu_ops arm_smmu_ops;
 
 #ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS
-static int arm_smmu_bus_init(struct iommu_ops *ops);
-
 static struct device_node *dev_get_dev_node(struct device *dev)
 {
if (dev_is_pci(dev)) {
@@ -180,20 +177,6 @@ static int arm_smmu_register_legacy_master(struct device 
*dev,
kfree(sids);
return err;
 }
-
-/*
- * With the legacy DT binding in play, we have no guarantees about
- * probe order, but then we're also not doing default domains, so we can
- * delay setting bus ops until we're sure every possible SMMU is ready,
- * and that way ensure that no probe_device() calls get missed.
- */
-static int arm_smmu_legacy_bus_init(void)
-{
-   if (using_legacy_binding)
-   return arm_smmu_bus_init(_smmu_ops);
-   return 0;
-}
-device_initcall_sync(arm_smmu_legacy_bus_init);
 #else
 static int arm_smmu_register_legacy_master(struct device *dev,
   struct arm_smmu_device **smmu)
@@ -2022,52 +2005,6 @@ static int arm_smmu_device_dt_probe(struct 
arm_smmu_device *smmu,
return 0;
 }
 
-static int arm_smmu_bus_init(struct iommu_ops *ops)
-{
-   int err;
-
-   /* Oh, for a proper bus abstraction */
-   if (!iommu_present(_bus_type)) {
-   err = bus_set_iommu(_bus_type, ops);
-   if (err)
-   return err;
-   }
-#ifdef CONFIG_ARM_AMBA
-   if (!iommu_present(_bustype)) {
-   err = bus_set_iommu(_bustype, ops);
-   if (err)
-   goto err_reset_platform_ops;
-   }
-#endif
-#ifdef CONFIG_PCI
-   if (!iommu_present(_bus_type)) {
-   err = bus_set_iommu(_bus_type, ops);
-   if (err)
-   goto err_reset_amba_ops;
-   }
-#endif
-#ifdef CONFIG_FSL_MC_BUS
-   if (!iommu_present(_mc_bus_type)) {
-   err = bus_set_iommu(_mc_bus_type, ops);
-   if (err)
-   goto err_reset_pci_ops;
-   }
-#endif
-   return 0;
-
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-   bus_set_iommu(_bus_type, NULL);
-#endif
-err_reset_amba_ops: __maybe_unused;
-#ifdef CONFIG_ARM_AMBA
-   bus_set_iommu(_bustype, NULL);
-#endif
-err_reset_platform_ops: __maybe_unused;
-   bus_set_iommu(_bus_type, NULL);
-   return err;
-}
-
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
struct resource *res;
@@ -2185,7 +2122,8 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
err = iommu_device_register(>iommu, _smmu_ops, dev);
if (err) {
dev_err(dev, "Failed to register iommu\n");
-   goto err_sysfs_remove;
+   iommu_device_sysfs_remove(>iommu);
+   return err;
}
 
platform_set_drvdata(pdev, smmu);
@@ -2203,24 +2141,7 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
pm_runtime_enable(dev);
}
 
-   /*
-* For ACPI and generic DT bindings, an SMMU will be probed before
-* any device which might need it, so we want the bus ops in place
-* ready to handle default domain setup as soon as any SMMU exists.
-*/
-   if (!using_legacy_binding) {
-   err = arm_smmu_bus_init(_smmu_ops);
-   if (err)
-   goto err_unregister_device;
-   }
-
return 0;
-
-err_unregister_device:
-   iommu_device_unregister(>iommu);
-err_sysfs_remove:
-   iommu_device_sysfs_remove(>iommu);
-   return err;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
@@ -2233,7 +2154,6 @@ static int arm_smmu_device_remove(struct platform_device 
*pdev)
if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
dev_notice(>dev, "disabling translation\n");
 
-   arm_smmu_bus_init(NULL);
iommu_device_unregister(>iommu);
iommu_device_sysfs_remove(>iommu);
 
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 02/13] iommu: Move bus setup to IOMMU device registration

2022-04-14 Thread Robin Murphy
Move the bus setup to iommu_device_register(). This should allow
bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
and leaves bus_set_iommu() as a glorfied no-op to be cleaned up next.

Note that although the handling of errors from bus_iommu_probe() looks
inadequate, it is merely preserving the well-established existing
behaviour. This could be improved in future - probably combined with
equivalent cleanup for iommu_device_unregister() - but that isn't a
priority right now.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/iommu.c | 50 ++-
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 13e1a8bd5435..51205c33c426 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -175,6 +175,14 @@ static int __init iommu_subsys_init(void)
 }
 subsys_initcall(iommu_subsys_init);
 
+static int remove_iommu_group(struct device *dev, void *data)
+{
+   if (dev->iommu && dev->iommu->iommu_dev == data)
+   iommu_release_device(dev);
+
+   return 0;
+}
+
 /**
  * iommu_device_register() - Register an IOMMU hardware instance
  * @iommu: IOMMU handle for the instance
@@ -197,6 +205,22 @@ int iommu_device_register(struct iommu_device *iommu,
spin_lock(_device_lock);
list_add_tail(>list, _device_list);
spin_unlock(_device_lock);
+
+   for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+   struct bus_type *bus = iommu_buses[i];
+   const struct iommu_ops *bus_ops = bus->iommu_ops;
+   int err;
+
+   WARN_ON(bus_ops && bus_ops != ops);
+   bus->iommu_ops = ops;
+   err = bus_iommu_probe(bus);
+   if (err) {
+   bus_for_each_dev(bus, NULL, iommu, remove_iommu_group);
+   bus->iommu_ops = bus_ops;
+   return err;
+   }
+   }
+
return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_device_register);
@@ -1654,13 +1678,6 @@ static int probe_iommu_group(struct device *dev, void 
*data)
return ret;
 }
 
-static int remove_iommu_group(struct device *dev, void *data)
-{
-   iommu_release_device(dev);
-
-   return 0;
-}
-
 static int iommu_bus_notifier(struct notifier_block *nb,
  unsigned long action, void *data)
 {
@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus)
  */
 int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
 {
-   int err;
-
-   if (ops == NULL) {
-   bus->iommu_ops = NULL;
-   return 0;
-   }
-
-   if (bus->iommu_ops != NULL)
+   if (bus->iommu_ops && ops && bus->iommu_ops != ops)
return -EBUSY;
 
bus->iommu_ops = ops;
 
-   /* Do IOMMU specific setup for this bus-type */
-   err = bus_iommu_probe(bus);
-   if (err) {
-   /* Clean up */
-   bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-   bus->iommu_ops = NULL;
-   }
-
-   return err;
+   return 0;
 }
 EXPORT_SYMBOL_GPL(bus_set_iommu);
 
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 01/13] iommu: Always register bus notifiers

2022-04-14 Thread Robin Murphy
The number of bus types that the IOMMU subsystem deals with is small and
manageable, so pull that list into core code as a first step towards
cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device() before bus->iommu_ops is set will simply return
-ENODEV and not break the notifier call chain, so there should be no
harm in proactively registering all our bus notifiers at init time.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/iommu.c | 49 ---
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3d6d53917e5d..13e1a8bd5435 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt)"iommu: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -22,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -74,6 +76,7 @@ static const char * const iommu_group_resv_type_string[] = {
 #define IOMMU_CMD_LINE_DMA_API BIT(0)
 #define IOMMU_CMD_LINE_STRICT  BIT(1)
 
+static int iommu_bus_init(struct bus_type *bus);
 static int iommu_alloc_default_domain(struct iommu_group *group,
  struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -102,6 +105,19 @@ struct iommu_group_attribute iommu_group_attr_##_name =
\
 static LIST_HEAD(iommu_device_list);
 static DEFINE_SPINLOCK(iommu_device_lock);
 
+static struct bus_type * const iommu_buses[] = {
+   _bus_type,
+#ifdef CONFIG_PCI
+   _bus_type,
+#endif
+#ifdef CONFIG_ARM_AMBA
+   _bustype,
+#endif
+#ifdef CONFIG_FSL_MC_BUS
+   _mc_bus_type,
+#endif
+};
+
 /*
  * Use a function instead of an array here because the domain-type is a
  * bit-field, so an array would waste memory.
@@ -151,6 +167,10 @@ static int __init iommu_subsys_init(void)
(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
"(set via kernel command line)" : "");
 
+   /* If the system is so broken that this fails, it will WARN anyway */
+   for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+   iommu_bus_init(iommu_buses[i]);
+
return 0;
 }
 subsys_initcall(iommu_subsys_init);
@@ -1831,35 +1851,19 @@ int bus_iommu_probe(struct bus_type *bus)
return ret;
 }
 
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
+static int iommu_bus_init(struct bus_type *bus)
 {
struct notifier_block *nb;
int err;
 
-   nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
+   nb = kzalloc(sizeof(*nb), GFP_KERNEL);
if (!nb)
return -ENOMEM;
 
nb->notifier_call = iommu_bus_notifier;
-
err = bus_register_notifier(bus, nb);
if (err)
-   goto out_free;
-
-   err = bus_iommu_probe(bus);
-   if (err)
-   goto out_err;
-
-
-   return 0;
-
-out_err:
-   /* Clean up */
-   bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-   bus_unregister_notifier(bus, nb);
-
-out_free:
-   kfree(nb);
+   kfree(nb);
 
return err;
 }
@@ -1892,9 +1896,12 @@ int bus_set_iommu(struct bus_type *bus, const struct 
iommu_ops *ops)
bus->iommu_ops = ops;
 
/* Do IOMMU specific setup for this bus-type */
-   err = iommu_bus_init(bus, ops);
-   if (err)
+   err = bus_iommu_probe(bus);
+   if (err) {
+   /* Clean up */
+   bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
bus->iommu_ops = NULL;
+   }
 
return err;
 }
-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH 00/13] iommu: Retire bus_set_iommu()

2022-04-14 Thread Robin Murphy
Hi all,

Here's another chapter in my saga of moving to per-instance IOMMU ops -
iommu_present() and iommu_capable() cleanups will be ongoing for another
cycle or two, while this one is at least self-contained within the
subsystem. The next steps after this are making iommu_domain_alloc()
instance-aware - which should finish the public API - and pulling the
fwnode/of_xlate bits into __iommu_probe_device(). And then making sense
of whatever's left :)

For ease of review here I split out individual driver patches based on
whether there was any non-trivial change or affect on control flow; the
straightforward deletions are all lumped together since the whole series
needs applying together either way, but I'm happy to split the final
patch up further if anyone would like.

Patch #3 for AMD is based on Mario's SWIOTLB patch here:

https://lore.kernel.org/linux-iommu/20220404204723.9767-1-mario.limoncie...@amd.com/

since that wants merging first as fix material. The series is also based
contextually (but not functionally) on my device_iommu_capable() patches
here:

https://lore.kernel.org/linux-iommu/cover.1649089693.git.robin.mur...@arm.com/

since those are pretty much good to go now (I'll send a slightly-tweaked
final version once the iommu/core branch is open).

Thanks,
Robin.


Robin Murphy (13):
  iommu: Always register bus notifiers
  iommu: Move bus setup to IOMMU device registration
  iommu/amd: Clean up bus_set_iommu()
  iommu/arm-smmu: Clean up bus_set_iommu()
  iommu/arm-smmu-v3: Clean up bus_set_iommu()
  iommu/dart: Clean up bus_set_iommu()
  iommu/exynos: Clean up bus_set_iommu()
  iommu/ipmmu-vmsa: Clean up bus_set_iommu()
  iommu/mtk: Clean up bus_set_iommu()
  iommu/omap: Clean up bus_set_iommu()
  iommu/tegra-smmu: Clean up bus_set_iommu()
  iommu/virtio: Clean up bus_set_iommu()
  iommu: Clean up bus_set_iommu()

 drivers/iommu/amd/amd_iommu.h   |   1 -
 drivers/iommu/amd/init.c|   9 +-
 drivers/iommu/amd/iommu.c   |  21 
 drivers/iommu/apple-dart.c  |  30 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  53 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c   |  84 +--
 drivers/iommu/arm/arm-smmu/qcom_iommu.c |   4 -
 drivers/iommu/exynos-iommu.c|   9 --
 drivers/iommu/fsl_pamu_domain.c |   4 -
 drivers/iommu/intel/iommu.c |   1 -
 drivers/iommu/iommu.c   | 109 +---
 drivers/iommu/ipmmu-vmsa.c  |  35 +--
 drivers/iommu/msm_iommu.c   |   2 -
 drivers/iommu/mtk_iommu.c   |  13 +--
 drivers/iommu/mtk_iommu_v1.c|  13 +--
 drivers/iommu/omap-iommu.c  |   6 --
 drivers/iommu/rockchip-iommu.c  |   2 -
 drivers/iommu/s390-iommu.c  |   6 --
 drivers/iommu/sprd-iommu.c  |   5 -
 drivers/iommu/sun50i-iommu.c|   2 -
 drivers/iommu/tegra-smmu.c  |  29 ++
 drivers/iommu/virtio-iommu.c|  24 -
 include/linux/iommu.h   |   1 -
 23 files changed, 62 insertions(+), 401 deletions(-)

-- 
2.28.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC 00/12] IOMMUFD Generic interface

2022-04-14 Thread Yi Liu

On 2022/3/19 01:27, Jason Gunthorpe wrote:

iommufd is the user API to control the IOMMU subsystem as it relates to
managing IO page tables that point at user space memory.

It takes over from drivers/vfio/vfio_iommu_type1.c (aka the VFIO
container) which is the VFIO specific interface for a similar idea.

We see a broad need for extended features, some being highly IOMMU device
specific:
  - Binding iommu_domain's to PASID/SSID
  - Userspace page tables, for ARM, x86 and S390
  - Kernel bypass'd invalidation of user page tables
  - Re-use of the KVM page table in the IOMMU
  - Dirty page tracking in the IOMMU
  - Runtime Increase/Decrease of IOPTE size
  - PRI support with faults resolved in userspace

As well as a need to access these features beyond just VFIO, VDPA for
instance, but other classes of accelerator HW are touching on these areas
now too.

The v1 series proposed re-using the VFIO type 1 data structure, however it
was suggested that if we are doing this big update then we should also
come with a data structure that solves the limitations that VFIO type1
has. Notably this addresses:

  - Multiple IOAS/'containers' and multiple domains inside a single FD

  - Single-pin operation no matter how many domains and containers use
a page

  - A fine grained locking scheme supporting user managed concurrency for
multi-threaded map/unmap

  - A pre-registration mechanism to optimize vIOMMU use cases by
pre-pinning pages

  - Extended ioctl API that can manage these new objects and exposes
domains directly to user space

  - domains are sharable between subsystems, eg VFIO and VDPA

The bulk of this code is a new data structure design to track how the
IOVAs are mapped to PFNs.

iommufd intends to be general and consumable by any driver that wants to
DMA to userspace. From a driver perspective it can largely be dropped in
in-place of iommu_attach_device() and provides a uniform full feature set
to all consumers.

As this is a larger project this series is the first step. This series
provides the iommfd "generic interface" which is designed to be suitable
for applications like DPDK and VMM flows that are not optimized to
specific HW scenarios. It is close to being a drop in replacement for the
existing VFIO type 1.

This is part two of three for an initial sequence:
  - Move IOMMU Group security into the iommu layer

https://lore.kernel.org/linux-iommu/20220218005521.172832-1-baolu...@linux.intel.com/
  * Generic IOMMUFD implementation
  - VFIO ability to consume IOMMUFD
An early exploration of this is available here:
 https://github.com/luxis1999/iommufd/commits/iommufd-v5.17-rc6


Eric Auger and me have posted a QEMU rfc based on this branch.

https://lore.kernel.org/kvm/20220414104710.28534-1-yi.l@intel.com/

--
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH] iommu/arm-smmu-v3: Align size in __arm_smmu_tlb_inv_range

2022-04-14 Thread Robin Murphy

On 2022-04-13 21:19, Nicolin Chen wrote:

Hi Robin,

On Wed, Apr 13, 2022 at 02:40:31PM +0100, Robin Murphy wrote:

On 2022-04-13 05:17, Nicolin Chen wrote:

To calculate num_pages, the size should be aligned with
"page size", determined by the tg value. Otherwise, its
following "while (iova < end)" might become an infinite
loop if unaligned size is slightly greater than 1 << tg.


Hmm, how does a non-page-aligned invalidation request get generated in
the first place?


I don't have the testing environment because it was a bug,
reported by a client who uses SVA feature on top of SMMU.

But judging from the log, the non-page-aligned inv request
was coming from an likely incorrect end address, e.g.
{ start = 0xff1, end = 0xff2 }
So the size turned out to be 0x10001, unaligned.

I don't have a full call trace on hand right now to see if
upper callers are doing something wrong when calculate the
end address, though I've asked the owner to check.

By looking at the call trace within arm_smmu_* functions:
   __arm_smmu_tlb_inv_range
   arm_smmu_tlb_inv_range_asid
   arm_smmu_mm_invalidate_range
   {from mm_notifier_* functions}

There's no address alignment check. Although I do think we
should fix the source who passes down the non-page-aligned
parameter, the SMMU driver shouldn't silently dead loop if
a set of unaligned inputs are given, IMHO.


Oh, sure, I'm not saying we definitely don't need to fix anything, I'd 
just like to get a better understanding of *what* we're fixing. I'd have 
(naively) expected the mm layer to give us page-aligned quantities even 
in the SVA notifier case, so if we've got a clear off-by-one somewhere 
in that path we should fix that before just blindly over-invalidating to 
paper over it; if we still also want to be robust at the SMMU driver end 
just in case, something like "if (WARN_ON(num_pages == 0)) num_pages = 
1;" might be more appropriate. However if it turns out that we *can* 
actually end up with unsanitised input from some userspace unmap 
interface getting this far, then a silent fixup is the best option, but 
if so I'd still like to confirm that we're rounding in the same 
direction as whoever touched the pagetables (since it can't have been us).


Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit

2022-04-14 Thread zhangfei....@foxmail.com


On 2022/4/12 下午11:35, zhangfei@foxmail.com wrote:

Hi, Fenghua

On 2022/4/12 下午9:41, Fenghua Yu wrote:

Hi, Zhangfei,

On Tue, Apr 12, 2022 at 03:04:09PM +0800, zhangfei@foxmail.com 
wrote:


On 2022/4/11 下午10:52, Dave Hansen wrote:

On 4/11/22 07:44, zhangfei@foxmail.com wrote:

On 2022/4/11 下午10:36, Dave Hansen wrote:

On 4/11/22 07:20, zhangfei@foxmail.com wrote:
Is there nothing before this call trace?  Usually there will be 
at least

some warning text.

I added dump_stack() in ioasid_free.

Hold on a sec, though...

What's the *problem* here?  Did something break or are you just 
saying

that something looks weird to _you_?

After this, nginx is not working at all, and hardware reports error.
Suppose the the master use the ioasid for init, but got freed.

hardware reports:
[  152.731869] hisi_sec2 :76:00.0: qm_acc_do_task_timeout 
[error status=0x20] found
[  152.739657] hisi_sec2 :76:00.0: qm_acc_wb_not_ready_timeout 
[error status=0x40] found
[  152.747877] hisi_sec2 :76:00.0: sec_fsm_hbeat_rint [error 
status=0x20] found

[  152.755340] hisi_sec2 :76:00.0: Controller resetting...
[  152.762044] hisi_sec2 :76:00.0: QM mailbox operation timeout!
[  152.768198] hisi_sec2 :76:00.0: Failed to dump sqc!
[  152.773490] hisi_sec2 :76:00.0: Failed to drain out data 
for stopping!

[  152.781426] hisi_sec2 :76:00.0: QM mailbox is busy to start!
[  152.787468] hisi_sec2 :76:00.0: Failed to dump sqc!
[  152.792753] hisi_sec2 :76:00.0: Failed to drain out data 
for stopping!

[  152.800685] hisi_sec2 :76:00.0: QM mailbox is busy to start!
[  152.806730] hisi_sec2 :76:00.0: Failed to dump sqc!
[  152.812017] hisi_sec2 :76:00.0: Failed to drain out data 
for stopping!

[  152.819946] hisi_sec2 :76:00.0: QM mailbox is busy to start!
[  152.825992] hisi_sec2 :76:00.0: Failed to dump sqc!
That would have been awfully handy information to have in an 
initial bug report. :)
Is there a chance you could dump out that ioasid alloc *and* free 
information in ioasid_alloc/free()?  This could be some kind of 
problem with the allocator, or with copying the ioasid at fork.
The issue is nginx master process init resource, start daemon 
process, then

master process quit and free ioasid.
The daemon nginx process is not the original master process.

master process:  init resource
driver -> iommu_sva_bind_device -> ioasid_alloc
Which code in the master process/daemon calls 
driver->iommu_sva_unbind_device?
Our calling sequence is nginx -> openssl -> openssl engine ->  kernel 
driver

The calling entrence should be ngx_ssl_init : OPENSSL_config(NULL);

nginx:
src/event/ngx_event_openssl.c
ngx_ssl_init
if (OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL) == 0)

I add some print.

/usr/local/nginx$ sudo sbin/nginx
ngx_ssl_init pid=2361
bind_fn
ngx_openssl_create_conf pid=2361
hisi sec init Kunpeng920!
ngx_ssl_create pid=2361
ngx_ssl_certificates pid=2361
ngx_ssl_certificate pid=2361
uadk_e_wd_digest_init
hisi sec init Kunpeng920!
ngx_ssl_ciphers pid=2361
ngx_daemon pid=2361 fork daemon
master pid=2361 will exit                                // here 
master process is exit
fork return 0 pid=2364                                       // here 
daemon process started

ngx_daemon fork ngx_pid=2364, ngx_parent=2361
$ ps -aux | grep nginx
root    2364  0.0  0.0  31324 15380 ?    Ssl  15:21   0:00 
nginx: master process sbin/nginx
nobody  2366  0.0  0.0  32304 16448 ?    Sl   15:21   0:00 
nginx: worker process
linaro  2371  0.0  0.0   7696  2048 pts/0    S+   15:22   0:00 
grep --color=auto nginx


nginx
src/os/unix/ngx_daemon.c
ngx_daemon(ngx_log_t *log)
{
    int  fd;

    switch (fork()) {
    case -1:
    ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "fork() failed");
    return NGX_ERROR;

    case 0:
   // here fork daemon process
    break;

    default:
  // master process directly exit, and release mm as well as ioasid
    exit(0);
    }

 // only daemon process
    ngx_parent = ngx_pid;
    ngx_pid = ngx_getpid();

Any suggestion?
ioasid is per process or per mm. A daemon process shouldn't share the 
same
ioasid with any other process with even its parent process. Its 
parent gets

an ioasid and frees it on exit. The ioasid is gone and shouldn't be used
by its child process.

Each daemon process should call driver -> iommu_sva_bind_device -> 
ioasid_alloc

to get its own ioasid/PASID. On daemon quit, the ioasid is freed.

That means nqnix needs to be changed.


Agree with Dave, I think user space should not be broken.

Thanks


Any plan about this regression?
Currently I need this patch to workaround the issue.

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c

index 22ddd05bbdcd..2d74ac53d11c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -4,6 +4,7 @@
  */

 #include