Re: [PATCH 0/4] ipmmu-vmsa cleanup

2017-10-13 Thread Arnd Bergmann 
On Fri, Oct 13, 2017 at 8:23 PM, Robin Murphy  wrote:
> Arnd reports a build warning[1] thanks to me missing ipmmu-vmsa's second
> set of ops when converting io-pgtable-arm users to the new iommu_iotlb_*
> callbacks. Rather than just treat the symptom with a point fix, this
> seemed like a good excuse to clean up the messy #ifdeffery and
> duplication in the driver that led to the oversight in the first place.
>
> Build-tested successfully for arm64, ARM, and ARM + CONFIG_IOMMU_DMA.

Very nice. I looked over the patches and while I only understood half of
what you do and why it was different to start with, I very much welcome
the end result.

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

Re: [PATCH v9 2/4] iommu/dma: Add a helper function to reserve HW MSI address regions for IOMMU drivers

2017-10-13 Thread Will Deacon 
On Fri, Oct 06, 2017 at 03:04:48PM +0100, Shameer Kolothum wrote:
> IOMMU drivers can use this to implement their .get_resv_regions callback
> for HW MSI specific reservations(e.g. ARM GICv3 ITS MSI region).
> 
> Signed-off-by: Shameer Kolothum 
> ---
>  drivers/iommu/dma-iommu.c | 20 
>  include/linux/dma-iommu.h |  7 +++
>  2 files changed, 27 insertions(+)

I'd like to see Robin's Ack on this, because this is his code and he had
ideas on ways to solve this problem properly.

Will

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9d1cebe..bae677e 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -19,6 +19,7 @@
>   * along with this program.  If not, see .
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -27,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -198,6 +200,24 @@ void iommu_dma_get_resv_regions(struct device *dev, 
> struct list_head *list)
>  }
>  EXPORT_SYMBOL(iommu_dma_get_resv_regions);
>  
> +/**
> + * iommu_dma_get_msi_resv_regions - Reserved region driver helper
> + * @dev: Device from iommu_get_resv_regions()
> + * @list: Reserved region list from iommu_get_resv_regions()
> + *
> + * IOMMU drivers can use this to implement their .get_resv_regions
> + * callback for HW MSI specific reservations. For now, this only
> + * covers ITS MSI region reservation using ACPI IORT helper function.
> + */
> +int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head 
> *list)
> +{
> + if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
> + return iort_iommu_msi_get_resv_regions(dev, list);
> +
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL(iommu_dma_get_msi_resv_regions);
> +
>  static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
>   phys_addr_t start, phys_addr_t end)
>  {
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 92f2083..6062ef0 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -74,6 +74,8 @@ void iommu_dma_unmap_resource(struct device *dev, 
> dma_addr_t handle,
>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>  void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
>  
> +int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head 
> *list);
> +
>  #else
>  
>  struct iommu_domain;
> @@ -107,6 +109,11 @@ static inline void iommu_dma_get_resv_regions(struct 
> device *dev, struct list_he
>  {
>  }
>  
> +static inline int iommu_dma_get_msi_resv_regions(struct device *dev, struct 
> list_head *list)
> +{
> + return -ENODEV;
> +}
> +
>  #endif   /* CONFIG_IOMMU_DMA */
>  #endif   /* __KERNEL__ */
>  #endif   /* __DMA_IOMMU_H */
> -- 
> 1.9.1
> 
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 4/4] PCI: hisi: blacklist hip06/hip07 controllers behind SMMUv3

2017-10-13 Thread Will Deacon 
On Fri, Oct 06, 2017 at 03:04:50PM +0100, Shameer Kolothum wrote:
> The HiSilicon erratum 161010801 describes the limitation of
> HiSilicon platforms hip06/hip07 to support the SMMUv3 mappings
> for MSI transactions.
> 
> PCIe controller on these platforms has to differentiate the MSI
> payload against other DMA payload and has to modify the MSI
> payload. This basically makes it difficult for this platforms to
> have a SMMU translation for MSI. In order to workaround this, ARM
> SMMUv3 driver requires a quirk to treat the MSI regions separately.
> Such a quirk is currently missing for DT based systems and therefore
> we need to blacklist the hip06/hip07 PCIe controllers.
> 
> Signed-off-by: Shameer Kolothum 
> ---
>  drivers/pci/dwc/pcie-hisi.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
> index a201791..6800747 100644
> --- a/drivers/pci/dwc/pcie-hisi.c
> +++ b/drivers/pci/dwc/pcie-hisi.c
> @@ -270,6 +270,12 @@ static int hisi_pcie_probe(struct platform_device *pdev)
>   struct resource *reg;
>   int ret;
>  
> + if ((IS_BUILTIN(CONFIG_ARM_SMMU_V3)) &&
> + of_property_read_bool(dev->of_node, "iommu-map")) {
> + dev_warn(dev, "HiSilicon erratum 161010801: blacklisting PCIe 
> controllers behind SMMUv3\n");
> + return -ENODEV;
> + }
> +
>   hisi_pcie = devm_kzalloc(dev, sizeof(*hisi_pcie), GFP_KERNEL);
>   if (!hisi_pcie)
>   return -ENOMEM;
> @@ -340,6 +346,12 @@ static int hisi_pcie_almost_ecam_probe(struct 
> platform_device *pdev)
>   struct device *dev = &pdev->dev;
>   struct pci_ecam_ops *ops;
>  
> + if ((IS_BUILTIN(CONFIG_ARM_SMMU_V3)) &&
> + of_property_read_bool(dev->of_node, "iommu-map")) {
> + dev_warn(dev, "HiSilicon erratum 161010801: blacklisting PCIe 
> controllers behind SMMUv3\n");
> + return -ENODEV;
> + }

This isn't the right way to solve this problem. I was really hoping you'd
come up with a solution for DT, and I know you've been trying, so I suppose
for now we'll just have to go with the ACPI workaround you have and leave DT
in the balance. I'm not at all happy with that, but I don't think this patch
really improves things.

What I think you should do is remove the relevant smmu/iommu-map entries
from the .dts files that are available for these platforms (i.e. comment
them out with a description as to why).

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

Re: [RFCv2 PATCH 12/36] dt-bindings: document stall and PASID properties for IOMMU masters

2017-10-13 Thread Rob Herring 
On Fri, Oct 06, 2017 at 02:31:39PM +0100, Jean-Philippe Brucker wrote:
> On ARM systems, some platform devices behind an IOMMU may support stall
> and PASID features. Stall is the ability to recover from page faults and
> PASID offers multiple process address spaces to the device. Together they
> allow to do paging with a device. Let the firmware tell us when a device
> supports stall and PASID.

Can't these be implied by the compatible string of the devices?

> 
> Signed-off-by: Jean-Philippe Brucker 
> ---
>  Documentation/devicetree/bindings/iommu/iommu.txt | 24 
> +++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/iommu.txt 
> b/Documentation/devicetree/bindings/iommu/iommu.txt
> index 5a8b4624defc..c589b75f7277 100644
> --- a/Documentation/devicetree/bindings/iommu/iommu.txt
> +++ b/Documentation/devicetree/bindings/iommu/iommu.txt
> @@ -86,6 +86,30 @@ have a means to turn off translation. But it is invalid in 
> such cases to
>  disable the IOMMU's device tree node in the first place because it would
>  prevent any driver from properly setting up the translations.
>  
> +Optional properties:
> +
> +- dma-can-stall: When present, the master can wait for a transaction to
> +  complete for an indefinite amount of time. Upon translation fault some
> +  IOMMUs, instead of aborting the translation immediately, may first
> +  notify the driver and keep the transaction in flight. This allows the OS
> +  to inspect the fault and, for example, make physical pages resident
> +  before updating the mappings and completing the transaction. Such IOMMU
> +  accepts a limited number of simultaneous stalled transactions before
> +  having to either put back-pressure on the master, or abort new faulting
> +  transactions.
> +
> +  Firmware has to opt-in stalling, because most buses and masters don't
> +  support it. In particular it isn't compatible with PCI, where
> +  transactions have to complete before a time limit. More generally it
> +  won't work in systems and masters that haven't been designed for
> +  stalling. For example the OS, in order to handle a stalled transaction,
> +  may attempt to retrieve pages from secondary storage in a stalled
> +  domain, leading to a deadlock.
> +
> +- pasid-bits: Some masters support multiple address spaces for DMA. By
> +  tagging DMA transactions with an address space identifier. By default,
> +  this is 0, which means that the device only has one address space.
> +
>  
>  Notes:
>  ==
> -- 
> 2.13.3
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/1] iommu/arm-smmu: Defer TLB flush in case of unmap op

2017-10-13 Thread Will Deacon 
On Wed, Sep 06, 2017 at 11:07:35AM +0530, Vivek Gautam wrote:
> We don't want to touch the TLB when smmu is suspended, so
> defer the TLB maintenance until smmu is resumed.
> On resume, we issue arm_smmu_device_reset() to restore the
> configuration and flush the TLBs.
> 
> Signed-off-by: Vivek Gautam 
> ---
> 
> Hi Robin,
> 
> This patch comes after the discussion[1] we had about defering the
> physical TLB maintenance for an unmap request if the SMMU is
> suspended. Sorry for the delay in sending the updated version
> of the patch.
> 
> As discussed, this patch now checks the PM state of smmu in
> .tlb_add_flush and .tlb_sync page-table ops and return if smmu
> is suspended. On resume without assuming that the TLBs state is
> preserved, we issue a arm_smmu_device_reset() which is the safest
> thing to do.
> 
> Alternatively, without going into the TLB defer thing, we can simply
> avoid calling pm_runtime_get/put() in case of atomic context, as we
> discussed in the other thread[3]. This will look something like this:
> 
>   static size_t arm_smmu_unmap(struct iommu_domain *domain, )
>   {
>   
> 
>  -   return ops->unmap(ops, iova, size);
>  +   if (!in_atomic())
>  +  pm_runtime_get_sync(smmu_domain->smmu->dev);
>  +   ret = ops->unmap(ops, iova, size);
>  +   if (!in_atomic())
>  +  pm_runtime_put_sync(smmu_domain->smmu->dev);
>  +
>  +   return ret;
>   }
> 
> Let me know which approach should work.
> 
> One other concern that we were discussing was of distributed SMMU
> configuration.
> "Say the GPU and its local TBU are in the same clock domain - if the GPU has
> just gone idle and we've clock-gated it, but "the SMMU" (i.e. the TCU)
> is still active servicing other devices, we will assume we can happily
> unmap GPU buffers and issue TLBIs, but what happens with entries held in
> the unclocked TBU's micro-TLB?"
> 
> In such scenerio, when master is clock gated and TCU is still running:
>  -> If TCU and TBU are in same clock/power domain, then we can still
> issue TLBIs as long as the smmu is clocked.
>  -> If TCU and TBU are in separate clock/power domains, then we better
> check the power state for TBUs and defer TLB maintenance if TBUs are
> clock gated.
> In such scenerio will it make sense to represent a distributed smmu
> as TCU device with multiple TBU child devices?

This is one of the cases that *really* worries me, particular if we can
end up freeing parts of the page table before the TLB maintenance has
been completed. Speculative table walks from the TCU could lead to all
sorts of horribly system behaviour, such as deadlock and/or data
corruption so I'm really not happy with this approach.

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

Re: [PATCH v2 0/4] SMMUv3 CMD_SYNC optimisation

2017-10-13 Thread Will Deacon 
Hi Robin,

On Thu, Aug 31, 2017 at 02:44:24PM +0100, Robin Murphy wrote:
> Since Nate reported a reasonable performance boost from the out-of-line
> MSI polling in v1 [1], I've now implemented the equivalent for cons
> polling as well - that has been boot-tested on D05 with some trivial I/O
> and at least doesn't seem to lock up or explode. There's also a little
> cosmetic tweaking to make the patches a bit cleaner as a series.
> 
> Robin.
> 
> [1] 
> https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg19657.html
> 
> Robin Murphy (5):
>   iommu/arm-smmu-v3: Specialise CMD_SYNC handling
>   iommu/arm-smmu-v3: Forget about cmdq-sync interrupt
>   iommu/arm-smmu-v3: Use CMD_SYNC completion interrupt
>   iommu/arm-smmu-v3: Poll for CMD_SYNC outside cmdq lock
>   iommu/arm-smmu-v3: Use burst-polling for sync completion

What's this final mythical patch about? I don't see it in the series.

Anyway, the first two patches look fine to me, but this doesn't apply
on top of my iommu/devel branch so they will need rebasing.

Cheers,

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

Re: [PATCH v2 4/4] iommu/arm-smmu-v3: Poll for CMD_SYNC outside cmdq lock

2017-10-13 Thread Will Deacon 
Hi Robin,

Some of my comments on patch 3 are addressed here, but I'm really struggling
to convince myself that this algorithm is correct. My preference would
be to leave the code as it is for SMMUs that don't implement MSIs, but
comments below anyway because it's an interesting idea.

On Thu, Aug 31, 2017 at 02:44:28PM +0100, Robin Murphy wrote:
> Even without the MSI trick, we can still do a lot better than hogging
> the entire queue while it drains. All we actually need to do for the
> necessary guarantee of completion is wait for our particular command to
> have been consumed, and as long as we keep track of where it is there is
> no need to block other CPUs from adding further commands in the
> meantime. There is one theoretical (but incredibly unlikely) edge case
> to avoid, where cons has wrapped twice to still appear 'behind' the sync
> position - this is easily disambiguated by adding a generation count to
> the queue to indicate when prod wraps, since cons cannot wrap twice
> without prod having wrapped at least once.
> 
> Signed-off-by: Robin Murphy 
> ---
> 
> v2: New
> 
>  drivers/iommu/arm-smmu-v3.c | 81 
> -
>  1 file changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 311f482b93d5..f5c5da553803 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -417,7 +417,6 @@
>  
>  /* High-level queue structures */
>  #define ARM_SMMU_POLL_TIMEOUT_US 100
> -#define ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US   100 /* 1s! */
>  #define ARM_SMMU_SYNC_TIMEOUT_US 100 /* 1s! */
>  
>  #define MSI_IOVA_BASE0x800
> @@ -540,6 +539,7 @@ struct arm_smmu_queue {
>  struct arm_smmu_cmdq {
>   struct arm_smmu_queue   q;
>   spinlock_t  lock;
> + int generation;
>  };
>  
>  struct arm_smmu_evtq {
> @@ -770,21 +770,12 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
>   writel(q->prod, q->prod_reg);
>  }
>  
> -/*
> - * Wait for the SMMU to consume items. If drain is true, wait until the queue
> - * is empty. Otherwise, wait until there is at least one free slot.
> - */
> -static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> +/* Wait for the SMMU to consume items, until there is at least one free slot 
> */
> +static int queue_poll_cons(struct arm_smmu_queue *q, bool wfe)
>  {
> - ktime_t timeout;
> - unsigned int delay = 1;
> + ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
>  
> - /* Wait longer if it's queue drain */
> - timeout = ktime_add_us(ktime_get(), drain ?
> - ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US :
> - ARM_SMMU_POLL_TIMEOUT_US);
> -
> - while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> + while (queue_sync_cons(q), queue_full(q)) {
>   if (ktime_compare(ktime_get(), timeout) > 0)
>   return -ETIMEDOUT;
>  
> @@ -792,8 +783,7 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool 
> drain, bool wfe)
>   wfe();
>   } else {
>   cpu_relax();
> - udelay(delay);
> - delay *= 2;
> + udelay(1);
>   }
>   }
>  
> @@ -959,15 +949,20 @@ static void arm_smmu_cmdq_skip_err(struct 
> arm_smmu_device *smmu)
>   queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);
>  }
>  
> -static void arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
> +static u32 arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
>  {
>   struct arm_smmu_queue *q = &smmu->cmdq.q;
>   bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
>  
>   while (queue_insert_raw(q, cmd) == -ENOSPC) {
> - if (queue_poll_cons(q, false, wfe))
> + if (queue_poll_cons(q, wfe))
>   dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
>   }
> +
> + if (Q_IDX(q, q->prod) == 0)
> + smmu->cmdq.generation++;

The readers of generation are using READ_ONCE, so you're missing something
here.

> +
> + return q->prod;
>  }
>  
>  static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
> @@ -997,15 +992,54 @@ static int arm_smmu_sync_poll_msi(struct 
> arm_smmu_device *smmu, u32 sync_idx)
>   return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;
>  }
>  
> +static int arm_smmu_sync_poll_cons(struct arm_smmu_device *smmu, u32 
> sync_idx,
> +int sync_gen)
> +{
> + ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_SYNC_TIMEOUT_US);
> + struct arm_smmu_queue *q = &smmu->cmdq.q;
> + bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
> + unsigned int delay = 1;
> +
> + do {
> + queue_sync_cons(q);
> +

Re: [PATCH v2 3/4] iommu/arm-smmu-v3: Use CMD_SYNC completion MSI

2017-10-13 Thread Will Deacon 
Hi Robin,

This mostly looks good. Just a few comments below.

On Thu, Aug 31, 2017 at 02:44:27PM +0100, Robin Murphy wrote:
> As an IRQ, the CMD_SYNC interrupt is not particularly useful, not least
> because we often need to wait for sync completion within someone else's
> IRQ handler anyway. However, when the SMMU is both coherent and supports
> MSIs, we can have a lot more fun by not using it as an interrupt at all.
> Following the example suggested in the architecture and using a write
> targeting normal memory, we can let callers wait on a status variable
> outside the lock instead of having to stall the entire queue or even
> touch MMIO registers. Since multiple sync commands are guaranteed to
> complete in order, a simple incrementing sequence count is all we need
> to unambiguously support any realistic number of overlapping waiters.
> 
> Signed-off-by: Robin Murphy 
> ---
> 
> v2: Remove redundant 'bool msi' command member, other cosmetic tweaks
> 
>  drivers/iommu/arm-smmu-v3.c | 47 
> +++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index f066725298cd..311f482b93d5 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -377,7 +377,16 @@
>  
>  #define CMDQ_SYNC_0_CS_SHIFT 12
>  #define CMDQ_SYNC_0_CS_NONE  (0UL << CMDQ_SYNC_0_CS_SHIFT)
> +#define CMDQ_SYNC_0_CS_IRQ   (1UL << CMDQ_SYNC_0_CS_SHIFT)
>  #define CMDQ_SYNC_0_CS_SEV   (2UL << CMDQ_SYNC_0_CS_SHIFT)
> +#define CMDQ_SYNC_0_MSH_SHIFT22
> +#define CMDQ_SYNC_0_MSH_ISH  (3UL << CMDQ_SYNC_0_MSH_SHIFT)
> +#define CMDQ_SYNC_0_MSIATTR_SHIFT24
> +#define CMDQ_SYNC_0_MSIATTR_OIWB (0xfUL << CMDQ_SYNC_0_MSIATTR_SHIFT)
> +#define CMDQ_SYNC_0_MSIDATA_SHIFT32
> +#define CMDQ_SYNC_0_MSIDATA_MASK 0xUL
> +#define CMDQ_SYNC_1_MSIADDR_SHIFT0
> +#define CMDQ_SYNC_1_MSIADDR_MASK 0xcUL
>  
>  /* Event queue */
>  #define EVTQ_ENT_DWORDS  4
> @@ -409,6 +418,7 @@
>  /* High-level queue structures */
>  #define ARM_SMMU_POLL_TIMEOUT_US 100
>  #define ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US   100 /* 1s! */
> +#define ARM_SMMU_SYNC_TIMEOUT_US 100 /* 1s! */

We only ever do this when waiting for the queue to drain, so may as well
just reuse the drain timeout.

>  #define MSI_IOVA_BASE0x800
>  #define MSI_IOVA_LENGTH  0x10
> @@ -504,6 +514,10 @@ struct arm_smmu_cmdq_ent {
>   } pri;
>  
>   #define CMDQ_OP_CMD_SYNC0x46
> + struct {
> + u32 msidata;
> + u64 msiaddr;
> + } sync;
>   };
>  };
>  
> @@ -617,6 +631,9 @@ struct arm_smmu_device {
>   int gerr_irq;
>   int combined_irq;
>  
> + atomic_tsync_nr;
> + u32 sync_count;

It's probably worth sticking these in separate cachelines so we don't
get spurious wakeups when sync_nr is incremented. (yes, I know it should
be the ERG, but that can be unreasonably huge!).

> +
>   unsigned long   ias; /* IPA */
>   unsigned long   oas; /* PA */
>   unsigned long   pgsize_bitmap;
> @@ -878,7 +895,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct 
> arm_smmu_cmdq_ent *ent)
>   }
>   break;
>   case CMDQ_OP_CMD_SYNC:
> - cmd[0] |= CMDQ_SYNC_0_CS_SEV;
> + if (ent->sync.msiaddr)
> + cmd[0] |= CMDQ_SYNC_0_CS_IRQ;
> + else
> + cmd[0] |= CMDQ_SYNC_0_CS_SEV;
> + cmd[0] |= CMDQ_SYNC_0_MSH_ISH | CMDQ_SYNC_0_MSIATTR_OIWB;
> + cmd[0] |= (u64)ent->sync.msidata << CMDQ_SYNC_0_MSIDATA_SHIFT;
> + cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
>   break;
>   default:
>   return -ENOENT;
> @@ -964,21 +987,40 @@ static void arm_smmu_cmdq_issue_cmd(struct 
> arm_smmu_device *smmu,
>   spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
>  }
>  
> +static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx)
> +{
> + ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_SYNC_TIMEOUT_US);
> + u32 val = smp_cond_load_acquire(&smmu->sync_count,
> + (int)(VAL - sync_idx) >= 0 ||
> + !ktime_before(ktime_get(), timeout));
> +
> + return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;

There are some theoretical overflow issues here which I don't think will
ever occur in practice, but deserve at least a comment to explain why.

> +}
> +
>  static void arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
>  {
>   u64 cmd[CMD

[PATCH 4/4] iommu/ipmmu-vmsa: Unify ipmmu_ops

2017-10-13 Thread Robin Murphy 
The remaining difference between the ARM-specific and iommu-dma ops is
in the {add,remove}_device implementations, but even those have some
overlap and duplication. By stubbing out the few arm_iommu_*() calls,
we can get rid of the rest of the inline #ifdeffery to both simplify the
code and improve build coverage.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/ipmmu-vmsa.c | 69 +-
 1 file changed, 19 insertions(+), 50 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 4d69c08145a1..2005aad09f2f 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -27,6 +27,11 @@
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include 
 #include 
+#else
+#define arm_iommu_create_mapping(...)  NULL
+#define arm_iommu_attach_device(...)   -ENODEV
+#define arm_iommu_release_mapping(...) do {} while (0)
+#define arm_iommu_detach_device(...)   do {} while (0)
 #endif
 
 #include "io-pgtable.h"
@@ -678,26 +683,17 @@ static int ipmmu_of_xlate(struct device *dev,
return ipmmu_init_platform_device(dev, spec);
 }
 
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
-
-static int ipmmu_add_device(struct device *dev)
+static int ipmmu_init_arm_mapping(struct device *dev)
 {
struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct iommu_group *group;
int ret;
 
-   /*
-* Only let through devices that have been verified in xlate()
-*/
-   if (!mmu)
-   return -ENODEV;
-
/* Create a device group and add the device to it. */
group = iommu_group_alloc();
if (IS_ERR(group)) {
dev_err(dev, "Failed to allocate IOMMU group\n");
-   ret = PTR_ERR(group);
-   goto error;
+   return PTR_ERR(group);
}
 
ret = iommu_group_add_device(group, dev);
@@ -705,8 +701,7 @@ static int ipmmu_add_device(struct device *dev)
 
if (ret < 0) {
dev_err(dev, "Failed to add device to IPMMU group\n");
-   group = NULL;
-   goto error;
+   return ret;
}
 
/*
@@ -742,41 +737,14 @@ static int ipmmu_add_device(struct device *dev)
return 0;
 
 error:
-   if (mmu)
+   iommu_group_remove_device(dev);
+   if (mmu->mapping)
arm_iommu_release_mapping(mmu->mapping);
 
-   if (!IS_ERR_OR_NULL(group))
-   iommu_group_remove_device(dev);
-
return ret;
 }
 
-static void ipmmu_remove_device(struct device *dev)
-{
-   arm_iommu_detach_device(dev);
-   iommu_group_remove_device(dev);
-}
-
-static const struct iommu_ops ipmmu_ops = {
-   .domain_alloc = ipmmu_domain_alloc,
-   .domain_free = ipmmu_domain_free,
-   .attach_dev = ipmmu_attach_device,
-   .detach_dev = ipmmu_detach_device,
-   .map = ipmmu_map,
-   .unmap = ipmmu_unmap,
-   .map_sg = default_iommu_map_sg,
-   .iova_to_phys = ipmmu_iova_to_phys,
-   .add_device = ipmmu_add_device,
-   .remove_device = ipmmu_remove_device,
-   .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
-   .of_xlate = ipmmu_of_xlate,
-};
-
-#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
-
-#ifdef CONFIG_IOMMU_DMA
-
-static int ipmmu_add_device_dma(struct device *dev)
+static int ipmmu_add_device(struct device *dev)
 {
struct iommu_group *group;
 
@@ -786,15 +754,20 @@ static int ipmmu_add_device_dma(struct device *dev)
if (!to_ipmmu(dev))
return -ENODEV;
 
+   if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA))
+   return ipmmu_init_arm_mapping(dev);
+
group = iommu_group_get_for_dev(dev);
if (IS_ERR(group))
return PTR_ERR(group);
 
+   iommu_group_put(group);
return 0;
 }
 
-static void ipmmu_remove_device_dma(struct device *dev)
+static void ipmmu_remove_device(struct device *dev)
 {
+   arm_iommu_detach_device(dev);
iommu_group_remove_device(dev);
 }
 
@@ -824,15 +797,13 @@ static const struct iommu_ops ipmmu_ops = {
.iotlb_sync = ipmmu_iotlb_sync,
.map_sg = default_iommu_map_sg,
.iova_to_phys = ipmmu_iova_to_phys,
-   .add_device = ipmmu_add_device_dma,
-   .remove_device = ipmmu_remove_device_dma,
+   .add_device = ipmmu_add_device,
+   .remove_device = ipmmu_remove_device,
.device_group = ipmmu_find_group,
.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
.of_xlate = ipmmu_of_xlate,
 };
 
-#endif /* CONFIG_IOMMU_DMA */
-
 /* 
-
  * Probe/remove and init
  */
@@ -929,9 +900,7 @@ static int ipmmu_remove(struct platform_device *pdev)
iommu_device_sysfs_remove(&mmu->iommu);
iommu_device_unregister(&mmu->iommu);
 
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
arm_iommu_release_mapping(mmu->mapping);
-#endif
 
ipmmu_device_r

[PATCH 2/4] iommu/ipmmu-vmsa: Simplify group allocation

2017-10-13 Thread Robin Murphy 
We go through quite the merry dance in order to find masters behind the
same IPMMU instance, so that we can ensure they are grouped together.
None of which is really necessary, since the master's private data
already points to the particular IPMMU it is associated with, and that
IPMMU instance data is the perfect place to keep track of a per-instance
group directly.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/ipmmu-vmsa.c | 53 --
 1 file changed, 9 insertions(+), 44 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index dedb386fa81d..d8c9bd4dc657 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -43,6 +43,7 @@ struct ipmmu_vmsa_device {
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
+   struct iommu_group *group;
struct dma_iommu_mapping *mapping;
 };
 
@@ -59,8 +60,6 @@ struct ipmmu_vmsa_domain {
 
 struct ipmmu_vmsa_iommu_priv {
struct ipmmu_vmsa_device *mmu;
-   struct device *dev;
-   struct list_head list;
 };
 
 static struct ipmmu_vmsa_domain *to_vmsa_domain(struct iommu_domain *dom)
@@ -674,7 +673,6 @@ static int ipmmu_init_platform_device(struct device *dev,
return -ENOMEM;
 
priv->mmu = platform_get_drvdata(ipmmu_pdev);
-   priv->dev = dev;
dev->iommu_fwspec->iommu_priv = priv;
return 0;
 }
@@ -790,9 +788,6 @@ static const struct iommu_ops ipmmu_ops = {
 
 #ifdef CONFIG_IOMMU_DMA
 
-static DEFINE_SPINLOCK(ipmmu_slave_devices_lock);
-static LIST_HEAD(ipmmu_slave_devices);
-
 static int ipmmu_add_device_dma(struct device *dev)
 {
struct iommu_group *group;
@@ -807,55 +802,25 @@ static int ipmmu_add_device_dma(struct device *dev)
if (IS_ERR(group))
return PTR_ERR(group);
 
-   spin_lock(&ipmmu_slave_devices_lock);
-   list_add(&to_priv(dev)->list, &ipmmu_slave_devices);
-   spin_unlock(&ipmmu_slave_devices_lock);
return 0;
 }
 
 static void ipmmu_remove_device_dma(struct device *dev)
 {
-   struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
-
-   spin_lock(&ipmmu_slave_devices_lock);
-   list_del(&priv->list);
-   spin_unlock(&ipmmu_slave_devices_lock);
-
iommu_group_remove_device(dev);
 }
 
-static struct device *ipmmu_find_sibling_device(struct device *dev)
+static struct iommu_group *ipmmu_find_group(struct device *dev)
 {
struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
-   struct ipmmu_vmsa_iommu_priv *sibling_priv = NULL;
-   bool found = false;
-
-   spin_lock(&ipmmu_slave_devices_lock);
-
-   list_for_each_entry(sibling_priv, &ipmmu_slave_devices, list) {
-   if (priv == sibling_priv)
-   continue;
-   if (sibling_priv->mmu == priv->mmu) {
-   found = true;
-   break;
-   }
-   }
-
-   spin_unlock(&ipmmu_slave_devices_lock);
-
-   return found ? sibling_priv->dev : NULL;
-}
-
-static struct iommu_group *ipmmu_find_group_dma(struct device *dev)
-{
struct iommu_group *group;
-   struct device *sibling;
 
-   sibling = ipmmu_find_sibling_device(dev);
-   if (sibling)
-   group = iommu_group_get(sibling);
-   if (!sibling || IS_ERR(group))
-   group = generic_device_group(dev);
+   if (priv->mmu->group)
+   return iommu_group_ref_get(priv->mmu->group);
+
+   group = iommu_group_alloc();
+   if (!IS_ERR(group))
+   priv->mmu->group = group;
 
return group;
 }
@@ -873,7 +838,7 @@ static const struct iommu_ops ipmmu_ops = {
.iova_to_phys = ipmmu_iova_to_phys,
.add_device = ipmmu_add_device_dma,
.remove_device = ipmmu_remove_device_dma,
-   .device_group = ipmmu_find_group_dma,
+   .device_group = ipmmu_find_group,
.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
.of_xlate = ipmmu_of_xlate,
 };
-- 
2.13.4.dirty

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

[PATCH 3/4] iommu/ipmmu-vmsa: Clean up struct ipmmu_vmsa_iommu_priv

2017-10-13 Thread Robin Murphy 
Now that the IPMMU instance pointer is the only thing remaining in the
private data structure, we no longer need the extra level of indirection
and can simply stash that directlty in the fwspec.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/ipmmu-vmsa.c | 36 
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index d8c9bd4dc657..4d69c08145a1 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -58,16 +58,12 @@ struct ipmmu_vmsa_domain {
spinlock_t lock;/* Protects mappings */
 };
 
-struct ipmmu_vmsa_iommu_priv {
-   struct ipmmu_vmsa_device *mmu;
-};
-
 static struct ipmmu_vmsa_domain *to_vmsa_domain(struct iommu_domain *dom)
 {
return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
 }
 
-static struct ipmmu_vmsa_iommu_priv *to_priv(struct device *dev)
+static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
 {
return dev->iommu_fwspec ? dev->iommu_fwspec->iommu_priv : NULL;
 }
@@ -565,15 +561,14 @@ static void ipmmu_domain_free(struct iommu_domain 
*io_domain)
 static int ipmmu_attach_device(struct iommu_domain *io_domain,
   struct device *dev)
 {
-   struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
struct iommu_fwspec *fwspec = dev->iommu_fwspec;
-   struct ipmmu_vmsa_device *mmu = priv->mmu;
+   struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
unsigned int i;
int ret = 0;
 
-   if (!priv || !priv->mmu) {
+   if (!mmu) {
dev_err(dev, "Cannot attach to IPMMU\n");
return -ENXIO;
}
@@ -662,18 +657,12 @@ static int ipmmu_init_platform_device(struct device *dev,
  struct of_phandle_args *args)
 {
struct platform_device *ipmmu_pdev;
-   struct ipmmu_vmsa_iommu_priv *priv;
 
ipmmu_pdev = of_find_device_by_node(args->np);
if (!ipmmu_pdev)
return -ENODEV;
 
-   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-   if (!priv)
-   return -ENOMEM;
-
-   priv->mmu = platform_get_drvdata(ipmmu_pdev);
-   dev->iommu_fwspec->iommu_priv = priv;
+   dev->iommu_fwspec->iommu_priv = platform_get_drvdata(ipmmu_pdev);
return 0;
 }
 
@@ -683,7 +672,7 @@ static int ipmmu_of_xlate(struct device *dev,
iommu_fwspec_add_ids(dev, spec->args, 1);
 
/* Initialize once - xlate() will call multiple times */
-   if (to_priv(dev))
+   if (to_ipmmu(dev))
return 0;
 
return ipmmu_init_platform_device(dev, spec);
@@ -693,14 +682,14 @@ static int ipmmu_of_xlate(struct device *dev,
 
 static int ipmmu_add_device(struct device *dev)
 {
-   struct ipmmu_vmsa_device *mmu = NULL;
+   struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct iommu_group *group;
int ret;
 
/*
 * Only let through devices that have been verified in xlate()
 */
-   if (!to_priv(dev))
+   if (!mmu)
return -ENODEV;
 
/* Create a device group and add the device to it. */
@@ -729,7 +718,6 @@ static int ipmmu_add_device(struct device *dev)
 * - Make the mapping size configurable ? We currently use a 2GB mapping
 *   at a 1GB offset to ensure that NULL VAs will fault.
 */
-   mmu = to_priv(dev)->mmu;
if (!mmu->mapping) {
struct dma_iommu_mapping *mapping;
 
@@ -795,7 +783,7 @@ static int ipmmu_add_device_dma(struct device *dev)
/*
 * Only let through devices that have been verified in xlate()
 */
-   if (!to_priv(dev))
+   if (!to_ipmmu(dev))
return -ENODEV;
 
group = iommu_group_get_for_dev(dev);
@@ -812,15 +800,15 @@ static void ipmmu_remove_device_dma(struct device *dev)
 
 static struct iommu_group *ipmmu_find_group(struct device *dev)
 {
-   struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+   struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
struct iommu_group *group;
 
-   if (priv->mmu->group)
-   return iommu_group_ref_get(priv->mmu->group);
+   if (mmu->group)
+   return iommu_group_ref_get(mmu->group);
 
group = iommu_group_alloc();
if (!IS_ERR(group))
-   priv->mmu->group = group;
+   mmu->group = group;
 
return group;
 }
-- 
2.13.4.dirty

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

[PATCH 1/4] iommu/ipmmu-vmsa: Unify domain alloc/free

2017-10-13 Thread Robin Murphy 
We have two implementations for ipmmu_ops->alloc depending on
CONFIG_IOMMU_DMA, the difference being whether they accept the
IOMMU_DOMAIN_DMA type or not. However, iommu_dma_get_cookie() is
guaranteed to return an error when !CONFIG_IOMMU_DMA, so if
ipmmu_domain_alloc_dma() was actually checking and handling the return
value correctly, it would behave the same as ipmmu_domain_alloc()
anyway.

Similarly for freeing; iommu_put_dma_cookie() is robust by design.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/ipmmu-vmsa.c | 65 +-
 1 file changed, 24 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 0bef160de7dd..dedb386fa81d 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -528,6 +528,27 @@ static struct iommu_domain *__ipmmu_domain_alloc(unsigned 
type)
return &domain->io_domain;
 }
 
+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+   struct iommu_domain *io_domain = NULL;
+
+   switch (type) {
+   case IOMMU_DOMAIN_UNMANAGED:
+   io_domain = __ipmmu_domain_alloc(type);
+   break;
+
+   case IOMMU_DOMAIN_DMA:
+   io_domain = __ipmmu_domain_alloc(type);
+   if (io_domain && iommu_get_dma_cookie(io_domain)) {
+   kfree(io_domain);
+   io_domain = NULL;
+   }
+   break;
+   }
+
+   return io_domain;
+}
+
 static void ipmmu_domain_free(struct iommu_domain *io_domain)
 {
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
@@ -536,6 +557,7 @@ static void ipmmu_domain_free(struct iommu_domain 
*io_domain)
 * Free the domain resources. We assume that all devices have already
 * been detached.
 */
+   iommu_put_dma_cookie(io_domain);
ipmmu_domain_destroy_context(domain);
free_io_pgtable_ops(domain->iop);
kfree(domain);
@@ -671,14 +693,6 @@ static int ipmmu_of_xlate(struct device *dev,
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
-   if (type != IOMMU_DOMAIN_UNMANAGED)
-   return NULL;
-
-   return __ipmmu_domain_alloc(type);
-}
-
 static int ipmmu_add_device(struct device *dev)
 {
struct ipmmu_vmsa_device *mmu = NULL;
@@ -779,37 +793,6 @@ static const struct iommu_ops ipmmu_ops = {
 static DEFINE_SPINLOCK(ipmmu_slave_devices_lock);
 static LIST_HEAD(ipmmu_slave_devices);
 
-static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
-{
-   struct iommu_domain *io_domain = NULL;
-
-   switch (type) {
-   case IOMMU_DOMAIN_UNMANAGED:
-   io_domain = __ipmmu_domain_alloc(type);
-   break;
-
-   case IOMMU_DOMAIN_DMA:
-   io_domain = __ipmmu_domain_alloc(type);
-   if (io_domain)
-   iommu_get_dma_cookie(io_domain);
-   break;
-   }
-
-   return io_domain;
-}
-
-static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
-{
-   switch (io_domain->type) {
-   case IOMMU_DOMAIN_DMA:
-   iommu_put_dma_cookie(io_domain);
-   /* fall-through */
-   default:
-   ipmmu_domain_free(io_domain);
-   break;
-   }
-}
-
 static int ipmmu_add_device_dma(struct device *dev)
 {
struct iommu_group *group;
@@ -878,8 +861,8 @@ static struct iommu_group *ipmmu_find_group_dma(struct 
device *dev)
 }
 
 static const struct iommu_ops ipmmu_ops = {
-   .domain_alloc = ipmmu_domain_alloc_dma,
-   .domain_free = ipmmu_domain_free_dma,
+   .domain_alloc = ipmmu_domain_alloc,
+   .domain_free = ipmmu_domain_free,
.attach_dev = ipmmu_attach_device,
.detach_dev = ipmmu_detach_device,
.map = ipmmu_map,
-- 
2.13.4.dirty

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

[PATCH 0/4] ipmmu-vmsa cleanup

2017-10-13 Thread Robin Murphy 
Arnd reports a build warning[1] thanks to me missing ipmmu-vmsa's second
set of ops when converting io-pgtable-arm users to the new iommu_iotlb_*
callbacks. Rather than just treat the symptom with a point fix, this
seemed like a good excuse to clean up the messy #ifdeffery and
duplication in the driver that led to the oversight in the first place.

Build-tested successfully for arm64, ARM, and ARM + CONFIG_IOMMU_DMA.

Robin.

[1]:https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1510423.html

Robin Murphy (4):
  iommu/ipmmu-vmsa: Unify domain alloc/free
  iommu/ipmmu-vmsa: Simplify group allocation
  iommu/ipmmu-vmsa: Clean up struct ipmmu_vmsa_iommu_priv
  iommu/ipmmu-vmsa: Unify ipmmu_ops

 drivers/iommu/ipmmu-vmsa.c | 251 ++---
 1 file changed, 78 insertions(+), 173 deletions(-)

-- 
2.13.4.dirty

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

Re: [PATCH 4/4] vfio/type1: Gather TLB-syncs and pages to unpin

2017-10-13 Thread Alex Williamson 
On Fri, 13 Oct 2017 16:40:13 +0200
Joerg Roedel  wrote:

> From: Joerg Roedel 
> 
> After every unmap VFIO unpins the pages that where mapped by
> the IOMMU. This requires an IOTLB flush after every unmap
> and puts a high load on the IOMMU hardware and the device
> TLBs.
> 
> Gather up to 32 ranges to flush and unpin and do the IOTLB
> flush once for all these ranges. This significantly reduces
> the number of IOTLB flushes in the unmapping path.
> 
> Signed-off-by: Joerg Roedel 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 106 
> ++--
>  1 file changed, 101 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2b1e81f..86fc1da 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -107,6 +107,92 @@ struct vfio_pfn {
>  
>  static int put_pfn(unsigned long pfn, int prot);
>  
> +static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
> + unsigned long pfn, long npage,
> + bool do_accounting);
> +
> +#define GATHER_ENTRIES   32

What heuristics make us arrive at this number and how would we evaluate
changing it in the future?  Ideally we'd only want to do a single
flush, but we can't unpin pages until after the iommu sync and we need
the iommu to track iova-phys mappings, so it's a matter of how much do
we want to allocate to buffer those translations.  I wonder if a cache
pool would help here, but this is probably fine for a first pass with
some comment about this trade-off and why 32 was chosen.

> +
> +/*
> + * Gather TLB flushes before unpinning pages
> + */
> +struct vfio_gather_entry {
> + dma_addr_t iova;
> + phys_addr_t phys;
> + size_t size;
> +};
> +
> +struct vfio_gather {
> + unsigned fill;
> + struct vfio_gather_entry entries[GATHER_ENTRIES];
> +};
> +
> +/*
> + * The vfio_gather* functions below keep track of flushing the IOMMU TLB
> + * and unpinning the pages. It is safe to call them gather == NULL, in
> + * which case they will fall-back to flushing the TLB and unpinning the
> + * pages at every call.
> + */
> +static long vfio_gather_flush(struct iommu_domain *domain,
> +   struct vfio_dma *dma,
> +   struct vfio_gather *gather)
> +{
> + long unlocked = 0;
> + unsigned i;
> +
> + if (!gather)

|| !gather->fill

We might have gotten lucky that our last add triggered a flush.

> + goto out;
> +
> + /* First flush unmapped TLB entries */
> + iommu_tlb_sync(domain);
> +
> + for (i = 0; i < gather->fill; i++) {
> + dma_addr_t iova = gather->entries[i].iova;
> + phys_addr_t phys = gather->entries[i].phys;
> + size_t size = gather->entries[i].size;
> +
> + unlocked += vfio_unpin_pages_remote(dma, iova,
> + phys >> PAGE_SHIFT,
> + size >> PAGE_SHIFT,
> + false);
> + }
> +
> + gather->fill = 0;


A struct vfio_gather_entry* would clean this up and eliminate some
variables, including i.

> +
> +out:
> + return unlocked;
> +}
> +
> +static long vfio_gather_add(struct iommu_domain *domain,
> + struct vfio_dma *dma,
> + struct vfio_gather *gather,
> + dma_addr_t iova, phys_addr_t phys, size_t size)
> +{
> + long unlocked = 0;
> +
> + if (gather) {
> + unsigned index;
> +
> + if (gather->fill == GATHER_ENTRIES)
> + unlocked = vfio_gather_flush(domain, dma, gather);

unlocked += vfio_unpin_pages_remote(...);
} else {

IOW, vfio_gather_flush() has already done the iommu_tlb_sync() for the
mapping that called us, there's no point in adding these to our list,
unpin them immediate.

> +
> + index = gather->fill++;
> +
> + gather->entries[index].iova = iova;
> + gather->entries[index].phys = phys;
> + gather->entries[index].size = size;

Alternatively, do the test and flush here instead.

Thanks,
Alex

> + } else {
> + iommu_tlb_sync(domain);
> +
> + unlocked = vfio_unpin_pages_remote(dma, iova,
> +phys >> PAGE_SHIFT,
> +size >> PAGE_SHIFT,
> +false);
> + }
> +
> + return unlocked;
> +}
> +
>  /*
>   * This code handles mapping and unmapping of user data buffers
>   * into DMA'ble space using the IOMMU
> @@ -653,6 +739,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
> struct vfio_dma *dma,
>  {
>   dma_addr_t iova = dma->iova, end = dma->iova + dma->size;
>   struct vfio_domain *

[git pull] IOMMU Fixes for Linux v4.14-rc4

2017-10-13 Thread Joerg Roedel 
Hi Linus,

The following changes since commit 8a5776a5f49812d29fe4b2d0a2d71675c3facf3f:

  Linux 4.14-rc4 (2017-10-08 20:53:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git 
tags/iommu-fixes-v4.14-rc4

for you to fetch changes up to ce76353f169a6471542d999baf3d29b121dce9c0:

  iommu/amd: Finish TLB flush in amd_iommu_unmap() (2017-10-13 17:32:19 +0200)


IOMMU Fixes for Linux v4.14-rc4

Three fixes:

- Keep an important data structure in the Exynos driver around
  after kernel-init to fix a kernel-oops

- Keep SWIOTLB enabled when SME is active in the AMD IOMMU
  driver

- Add a missing IOTLB sync to the AMD IOMMU driver


Joerg Roedel (1):
  iommu/amd: Finish TLB flush in amd_iommu_unmap()

Marek Szyprowski (1):
  iommu/exynos: Remove initconst attribute to avoid potential kernel oops

Tom Lendacky (1):
  iommu/amd: Do not disable SWIOTLB if SME is active

 drivers/iommu/amd_iommu.c| 11 +++
 drivers/iommu/exynos-iommu.c |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

Please pull.

Thanks,

Joerg


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

Re: [PATCH v2] iommu/ipmmu-vmsa: Do not replace bus IOMMU ops on driver init.

2017-10-13 Thread Robin Murphy 
Hi Joerg,

On 20/09/17 15:13, Liviu Dudau wrote:
> If the IPMMU driver is compiled in the kernel it will replace the
> platform bus IOMMU ops on running the ipmmu_init() function, regardless
> if there is any IPMMU hardware present or not. This screws up systems
> that just want to build a generic kernel that runs on multiple platforms
> and use a different IOMMU implementation.
> 
> Move the bus_set_iommu() call at the end of the ipmmu_probe() function
> when we know that hardware is present. With current IOMMU framework it
> should be safe (at least for OF case).
> 
> Now that the ipmmu_init() and ipmmu_exit() functions are simple calls to
> platform_driver_register() and platform_driver_unregister(), replace
> them with the module_platform_driver() macro call.

Are you OK with taking this patch as a fix for 4.14, or would you rather
have something that can safely backport past 4.12 without implicit
dependencies? This is a config/link-order dependent thing that's been
lurking since the beginning, but only coming to light now that other
drivers are changing their behaviour, so I don't think there's really a
single Fixes: commit that can be singled out.

Robin.

> Signed-off-by: Liviu Dudau 
> Cc: Laurent Pinchart 
> ---
>  drivers/iommu/ipmmu-vmsa.c | 29 +
>  1 file changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 195d6e93ac718..31912997bffdf 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -966,10 +966,11 @@ static int ipmmu_probe(struct platform_device *pdev)
>   return ret;
>  
>   /*
> -  * We can't create the ARM mapping here as it requires the bus to have
> -  * an IOMMU, which only happens when bus_set_iommu() is called in
> -  * ipmmu_init() after the probe function returns.
> +  * Now that we have validated the presence of the hardware, set
> +  * the bus IOMMU ops to enable future domain and device setup.
>*/
> + if (!iommu_present(&platform_bus_type))
> + bus_set_iommu(&platform_bus_type, &ipmmu_ops);
>  
>   platform_set_drvdata(pdev, mmu);
>  
> @@ -1006,27 +1007,7 @@ static struct platform_driver ipmmu_driver = {
>   .remove = ipmmu_remove,
>  };
>  
> -static int __init ipmmu_init(void)
> -{
> - int ret;
> -
> - ret = platform_driver_register(&ipmmu_driver);
> - if (ret < 0)
> - return ret;
> -
> - if (!iommu_present(&platform_bus_type))
> - bus_set_iommu(&platform_bus_type, &ipmmu_ops);
> -
> - return 0;
> -}
> -
> -static void __exit ipmmu_exit(void)
> -{
> - return platform_driver_unregister(&ipmmu_driver);
> -}
> -
> -subsys_initcall(ipmmu_init);
> -module_exit(ipmmu_exit);
> +module_platform_driver(ipmmu_driver);
>  
>  MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");
>  MODULE_AUTHOR("Laurent Pinchart ");
> 

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

[PATCH 1/4] iommu/amd: Finish TLB flush in amd_iommu_unmap()

2017-10-13 Thread Joerg Roedel 
From: Joerg Roedel 

The function only sends the flush command to the IOMMU(s),
but does not wait for its completion when it returns. Fix
that.

Fixes: 601367d76bd1 ('x86/amd-iommu: Remove iommu_flush_domain function')
Cc: sta...@vger.kernel.org # >= 2.6.33
Signed-off-by: Joerg Roedel 
---
 drivers/iommu/amd_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index cb7c531..3a702c4 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3046,6 +3046,7 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, 
unsigned long iova,
mutex_unlock(&domain->api_lock);
 
domain_flush_tlb_pde(domain);
+   domain_flush_complete(domain);
 
return unmap_size;
 }
-- 
2.7.4

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

[PATCH 2/4] iommu/amd: Implement IOMMU-API TLB flush interface

2017-10-13 Thread Joerg Roedel 
From: Joerg Roedel 

Make use of the new IOTLB flush-interface in the IOMMU-API.
We don't implement the iotlb_range_add() call-back for now,
as this will put too many pressure on the command buffer.
Instead, we do a full TLB flush in the iotlb_sync()
call-back.

Signed-off-by: Joerg Roedel 
---
 drivers/iommu/amd_iommu.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 3a702c4..8804264 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3032,6 +3032,14 @@ static int amd_iommu_map(struct iommu_domain *dom, 
unsigned long iova,
return ret;
 }
 
+static void amd_iommu_flush_iotlb_all(struct iommu_domain *dom)
+{
+   struct protection_domain *domain = to_pdomain(dom);
+
+   domain_flush_tlb_pde(domain);
+   domain_flush_complete(domain);
+}
+
 static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
   size_t page_size)
 {
@@ -3045,9 +3053,6 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, 
unsigned long iova,
unmap_size = iommu_unmap_page(domain, iova, page_size);
mutex_unlock(&domain->api_lock);
 
-   domain_flush_tlb_pde(domain);
-   domain_flush_complete(domain);
-
return unmap_size;
 }
 
@@ -3174,6 +3179,8 @@ const struct iommu_ops amd_iommu_ops = {
.map = amd_iommu_map,
.unmap = amd_iommu_unmap,
.map_sg = default_iommu_map_sg,
+   .flush_iotlb_all = amd_iommu_flush_iotlb_all,
+   .iotlb_sync = amd_iommu_flush_iotlb_all,
.iova_to_phys = amd_iommu_iova_to_phys,
.add_device = amd_iommu_add_device,
.remove_device = amd_iommu_remove_device,
-- 
2.7.4

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

[PATCH 3/4] vfio/type1: Make use of iommu_unmap_fast()

2017-10-13 Thread Joerg Roedel 
From: Joerg Roedel 

Switch from using iommu_unmap to iommu_unmap_fast() and add
the necessary calls the the IOTLB invalidation routines.

Signed-off-by: Joerg Roedel 
---
 drivers/vfio/vfio_iommu_type1.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 92155cc..2b1e81f 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -672,10 +672,13 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
struct vfio_dma *dma,
  struct vfio_domain, next);
 
list_for_each_entry_continue(d, &iommu->domain_list, next) {
-   iommu_unmap(d->domain, dma->iova, dma->size);
+   iommu_unmap_fast(d->domain, dma->iova, dma->size);
+   iommu_tlb_range_add(d->domain, dma->iova, dma->size);
cond_resched();
}
 
+   iommu_tlb_sync(domain->domain);
+
while (iova < end) {
size_t unmapped, len;
phys_addr_t phys, next;
@@ -698,10 +701,13 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
struct vfio_dma *dma,
break;
}
 
-   unmapped = iommu_unmap(domain->domain, iova, len);
+   unmapped = iommu_unmap_fast(domain->domain, iova, len);
if (WARN_ON(!unmapped))
break;
 
+   iommu_tlb_range_add(domain->domain, iova, unmapped);
+   iommu_tlb_sync(domain->domain);
+
unlocked += vfio_unpin_pages_remote(dma, iova,
phys >> PAGE_SHIFT,
unmapped >> PAGE_SHIFT,
@@ -885,7 +891,9 @@ static int map_try_harder(struct vfio_domain *domain, 
dma_addr_t iova,
}
 
for (; i < npage && i > 0; i--, iova -= PAGE_SIZE)
-   iommu_unmap(domain->domain, iova, PAGE_SIZE);
+   iommu_unmap_fast(domain->domain, iova, PAGE_SIZE);
+
+   iommu_tlb_sync(domain->domain);
 
return ret;
 }
@@ -912,7 +920,9 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, 
dma_addr_t iova,
 
 unwind:
list_for_each_entry_continue_reverse(d, &iommu->domain_list, next)
-   iommu_unmap(d->domain, iova, npage << PAGE_SHIFT);
+   iommu_unmap_fast(d->domain, iova, npage << PAGE_SHIFT);
+
+   iommu_tlb_sync(d->domain);
 
return ret;
 }
@@ -1136,12 +1146,14 @@ static void vfio_test_domain_fgsp(struct vfio_domain 
*domain)
ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
IOMMU_READ | IOMMU_WRITE | domain->prot);
if (!ret) {
-   size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
+   size_t unmapped = iommu_unmap_fast(domain->domain, 0, 
PAGE_SIZE);
 
if (unmapped == PAGE_SIZE)
-   iommu_unmap(domain->domain, PAGE_SIZE, PAGE_SIZE);
+   iommu_unmap_fast(domain->domain, PAGE_SIZE, PAGE_SIZE);
else
domain->fgsp = true;
+
+   iommu_tlb_sync(domain->domain);
}
 
__free_pages(pages, order);
-- 
2.7.4

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

[PATCH 4/4] vfio/type1: Gather TLB-syncs and pages to unpin

2017-10-13 Thread Joerg Roedel 
From: Joerg Roedel 

After every unmap VFIO unpins the pages that where mapped by
the IOMMU. This requires an IOTLB flush after every unmap
and puts a high load on the IOMMU hardware and the device
TLBs.

Gather up to 32 ranges to flush and unpin and do the IOTLB
flush once for all these ranges. This significantly reduces
the number of IOTLB flushes in the unmapping path.

Signed-off-by: Joerg Roedel 
---
 drivers/vfio/vfio_iommu_type1.c | 106 ++--
 1 file changed, 101 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2b1e81f..86fc1da 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -107,6 +107,92 @@ struct vfio_pfn {
 
 static int put_pfn(unsigned long pfn, int prot);
 
+static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
+   unsigned long pfn, long npage,
+   bool do_accounting);
+
+#define GATHER_ENTRIES 32
+
+/*
+ * Gather TLB flushes before unpinning pages
+ */
+struct vfio_gather_entry {
+   dma_addr_t iova;
+   phys_addr_t phys;
+   size_t size;
+};
+
+struct vfio_gather {
+   unsigned fill;
+   struct vfio_gather_entry entries[GATHER_ENTRIES];
+};
+
+/*
+ * The vfio_gather* functions below keep track of flushing the IOMMU TLB
+ * and unpinning the pages. It is safe to call them gather == NULL, in
+ * which case they will fall-back to flushing the TLB and unpinning the
+ * pages at every call.
+ */
+static long vfio_gather_flush(struct iommu_domain *domain,
+ struct vfio_dma *dma,
+ struct vfio_gather *gather)
+{
+   long unlocked = 0;
+   unsigned i;
+
+   if (!gather)
+   goto out;
+
+   /* First flush unmapped TLB entries */
+   iommu_tlb_sync(domain);
+
+   for (i = 0; i < gather->fill; i++) {
+   dma_addr_t iova = gather->entries[i].iova;
+   phys_addr_t phys = gather->entries[i].phys;
+   size_t size = gather->entries[i].size;
+
+   unlocked += vfio_unpin_pages_remote(dma, iova,
+   phys >> PAGE_SHIFT,
+   size >> PAGE_SHIFT,
+   false);
+   }
+
+   gather->fill = 0;
+
+out:
+   return unlocked;
+}
+
+static long vfio_gather_add(struct iommu_domain *domain,
+   struct vfio_dma *dma,
+   struct vfio_gather *gather,
+   dma_addr_t iova, phys_addr_t phys, size_t size)
+{
+   long unlocked = 0;
+
+   if (gather) {
+   unsigned index;
+
+   if (gather->fill == GATHER_ENTRIES)
+   unlocked = vfio_gather_flush(domain, dma, gather);
+
+   index = gather->fill++;
+
+   gather->entries[index].iova = iova;
+   gather->entries[index].phys = phys;
+   gather->entries[index].size = size;
+   } else {
+   iommu_tlb_sync(domain);
+
+   unlocked = vfio_unpin_pages_remote(dma, iova,
+  phys >> PAGE_SHIFT,
+  size >> PAGE_SHIFT,
+  false);
+   }
+
+   return unlocked;
+}
+
 /*
  * This code handles mapping and unmapping of user data buffers
  * into DMA'ble space using the IOMMU
@@ -653,6 +739,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
struct vfio_dma *dma,
 {
dma_addr_t iova = dma->iova, end = dma->iova + dma->size;
struct vfio_domain *domain, *d;
+   struct vfio_gather *gather;
long unlocked = 0;
 
if (!dma->size)
@@ -662,6 +749,12 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
struct vfio_dma *dma,
return 0;
 
/*
+* No need to check return value - It is safe to continue with a
+* NULL pointer.
+*/
+   gather = kzalloc(sizeof(*gather), GFP_KERNEL);
+
+   /*
 * We use the IOMMU to track the physical addresses, otherwise we'd
 * need a much more complicated tracking system.  Unfortunately that
 * means we need to use one of the iommu domains to figure out the
@@ -706,17 +799,20 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, 
struct vfio_dma *dma,
break;
 
iommu_tlb_range_add(domain->domain, iova, unmapped);
-   iommu_tlb_sync(domain->domain);
 
-   unlocked += vfio_unpin_pages_remote(dma, iova,
-   phys >> PAGE_SHIFT,
-   unmapped >> PAGE_SHIFT,
-   false);
+   unlo

[PATCH 0/4] iommu/amd, vfio: Reduce IOTLB Flushm Rate

2017-10-13 Thread Joerg Roedel 
Hi,

these patches implement the new IOTLB flush interface in the
AMD IOMMU driver. But for it to take effect, changes in VFIO
are also necessary, because VFIO unpins the pages after
every successful iommu_unmap() call. This requires an IOTLB
flush, so that we don't save flushes.

So I implemented vfio-gather code to collect up to 32 ranges
before they are unpinned together. This significantly
reduces the number of necessary IOTLB flushes in my tests.

Please review.

Thanks,

Joerg

Joerg Roedel (4):
  iommu/amd: Finish TLB flush in amd_iommu_unmap()
  iommu/amd: Implement IOMMU-API TLB flush interface
  vfio/type1: Make use of iommu_unmap_fast()
  vfio/type1: Gather TLB-syncs and pages to unpin

 drivers/iommu/amd_iommu.c   |  12 +++-
 drivers/vfio/vfio_iommu_type1.c | 128 
 2 files changed, 128 insertions(+), 12 deletions(-)

-- 
2.7.4

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

Re: [PATCH] dt-bindings: iommu: ipmmu-vmsa: Use generic node name

2017-10-13 Thread Rob Herring 
On Wed, Oct 04, 2017 at 02:33:08PM +0200, Geert Uytterhoeven wrote:
> Use the preferred generic node name in the example.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied.

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

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

2017-10-13 Thread Dmitry Osipenko 
On 13.10.2017 11:38, Joerg Roedel wrote:
> On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
>> I'm not talking about any specific bug, but in general if allocator re-maps
>> already mapped region or unmaps the wrong-and-used region. I had those 
>> bug-cases
>> during of development of the 'scattered' graphics allocations for Tegra20.
> 
> The dma-iommu code does not re-map already mapped regions and it doesn't
> unmap wrong regions. If it does it should be reported and fixed.
> 

Certainly iommu_map_sg doesn't perform itself any debug checks that I'm
proposing to add to the GART.

Yet we don't use GART in the mainline, right now you may take a look at the WIP
patches here:

https://github.com/grate-driver/linux/commit/9853371164a7f1b5698caee476e7cffe1b446afa

https://github.com/grate-driver/linux/commit/ea1fca4ac932464e7907a7ada8ea2698cab8db65

> So if you hit any bug there, please report it so that it can be fixed.
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] xhci: Set DMA parameters appropriately

2017-10-13 Thread Robin Murphy 
Hi Marek,

On 13/10/17 09:15, Marek Szyprowski wrote:
> Hi Robin,
> 
> On 2017-10-11 15:56, Robin Murphy wrote:
>> xHCI requires that data buffers do not cross 64KB boundaries (and are
>> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
>> already split their input buffers into individual TRBs as necessary,
>> it's still a good idea to advertise the limitations via the standard DMA
>> API mechanism, so that most producers like the block layer and the DMA
>> mapping implementations can lay things out correctly to begin with.
>>
>> Signed-off-by: Robin Murphy 
>> ---
>>   drivers/usb/host/xhci.c | 4 
>>   drivers/usb/host/xhci.h | 3 +++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 74b4500641c2..1e7e1e3d8c48 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>> xhci_get_quirks_t get_quirks)
>>   dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>   }
>>   +    dev->dma_parms = &xhci->dma_parms;
>> +    dma_set_max_seg_size(dev, SZ_64K);
>> +    dma_set_seg_boundary(dev, SZ_64K - 1);
>> +
>>   xhci_dbg(xhci, "Calling HCD init\n");
>>   /* Initialize HCD and host controller data structures. */
>>   retval = xhci_init(hcd);
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 7ef69ea0b480..afcae4cc908d 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
>>   struct dma_pool    *small_streams_pool;
>>   struct dma_pool    *medium_streams_pool;
>>   +    /* DMA alignment restrictions */
>> +    struct device_dma_parameters dma_parms;
>> +
>>   /* Host controller watchdog timer structures */
>>   unsigned int    xhc_state;
>>   
> 
> Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
> that when driver gets removed and xhci_hcd is freed, the dma_parms will
> point to freed memory. Maybe it would make sense to clear dev->dma_parms
> somewhere or definitely change the way dma_parms are allocated?

AFAICS it lives until the last usb_put_hcd() call, which is pretty much
the last thing in the drivers' .remove paths, so it looks to follow the
standard paradigm evidenced by other dma_parms users. Any dangling
pointer after the driver has been unbound will be reinitialised by a
subsequent probe, and anyone using an unbound device for DMA API calls
is very wrong anyway.

> On the other hand 64K is the default segment size if no dma_parms are
> provided, so there is very little value added by this patch.

I prefer to explicitly set the segment size for cleanliness and to
emphasize the difference between "whatever the default value is is fine"
and "we have a real hardware limit of 64K". What really matters here
though is the non-default segment boundary mask - that's the motiviation
for the patch.

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

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

2017-10-13 Thread Joerg Roedel 
On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
> I'm not talking about any specific bug, but in general if allocator re-maps
> already mapped region or unmaps the wrong-and-used region. I had those 
> bug-cases
> during of development of the 'scattered' graphics allocations for Tegra20.

The dma-iommu code does not re-map already mapped regions and it doesn't
unmap wrong regions. If it does it should be reported and fixed.

So if you hit any bug there, please report it so that it can be fixed.


Thanks,

Joerg

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

Re: [PATCH] xhci: Set DMA parameters appropriately

2017-10-13 Thread Marek Szyprowski 

Hi Robin,

On 2017-10-11 15:56, Robin Murphy wrote:

xHCI requires that data buffers do not cross 64KB boundaries (and are
thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
already split their input buffers into individual TRBs as necessary,
it's still a good idea to advertise the limitations via the standard DMA
API mechanism, so that most producers like the block layer and the DMA
mapping implementations can lay things out correctly to begin with.

Signed-off-by: Robin Murphy 
---
  drivers/usb/host/xhci.c | 4 
  drivers/usb/host/xhci.h | 3 +++
  2 files changed, 7 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 74b4500641c2..1e7e1e3d8c48 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
}
  
+	dev->dma_parms = &xhci->dma_parms;

+   dma_set_max_seg_size(dev, SZ_64K);
+   dma_set_seg_boundary(dev, SZ_64K - 1);
+
xhci_dbg(xhci, "Calling HCD init\n");
/* Initialize HCD and host controller data structures. */
retval = xhci_init(hcd);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7ef69ea0b480..afcae4cc908d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1767,6 +1767,9 @@ struct xhci_hcd {
struct dma_pool *small_streams_pool;
struct dma_pool *medium_streams_pool;
  
+	/* DMA alignment restrictions */

+   struct device_dma_parameters dma_parms;
+
/* Host controller watchdog timer structures */
unsigned intxhc_state;
  


Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
that when driver gets removed and xhci_hcd is freed, the dma_parms will
point to freed memory. Maybe it would make sense to clear dev->dma_parms
somewhere or definitely change the way dma_parms are allocated?

On the other hand 64K is the default segment size if no dma_parms are
provided, so there is very little value added by this patch.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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