[GIT PULL] dma mapping fix for 4.17

[Christoph Hellwig]

The following changes since commit b284d4d5a6785f8cd07eda2646a95782373cd01e:

  Merge tag 'ceph-for-4.17-rc1' of git://github.com/ceph/ceph-client 
(2018-04-10 12:25:30 -0700)

are available in the Git repository at:

  git://git.infradead.org/users/hch/dma-mapping.git tags/dma-mapping-4.17-2

for you to fetch changes up to 9e7f06c8beee304ee21b791653fefcd713f48b9a:

  swiotlb: fix unexpected swiotlb_alloc_coherent failures (2018-04-10 22:30:43 
+0200)


Fix for one swiotlb regression in 2.16 from Takashi.


Takashi Iwai (1):
  swiotlb: fix unexpected swiotlb_alloc_coherent failures

 lib/swiotlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

[Yisheng Xie]

Hi Vivek,

On 2018/4/11 13:15, Vivek Gautam wrote:
> Hi Yisheng,
> 
> 
> On 4/11/2018 6:52 AM, Yisheng Xie wrote:
>> Hi Tomasz,
>>
>> On 2018/4/10 21:14, Tomasz Figa wrote:
>>> Hi Yisheng,
>>>
>>> Sorry, I think we missed your question here.
>>>
>>> On Wed, Mar 28, 2018 at 3:12 PM Yisheng Xie  wrote:
>>>
 Hi Vivek,
 On 2018/3/28 12:37, Vivek Gautam wrote:
> Hi Yisheng
>
>
> On 3/28/2018 6:54 AM, Yisheng Xie wrote:
>> Hi Vivek,
>>
>> On 2018/3/13 16:55, Vivek Gautam wrote:
>>> +- power-domains:  Specifiers for power domains required to be
>>> powered on for
>>> +  the SMMU to operate, as per generic power domain
>>> bindings.
>>> +
>> In this patchset, power-domains is not used right? And you just do the
>>> clock gating,
>> but not power gating, right?
> We are handling the power-domains too. Please see the example in this
>>> binding doc.
>>>
 I see, but I do not find the point in code of these patchset, do you mean
>>> PMIC(e.g mmcc)
 will gate the power domain of SMMU(e.g. MDSS_GDSC of mmcc) when PMIC
>>> suspend?
>>>
>>>
>>> If respective SoC power domains is registered as a standard genpd PM
>>> domain, then the runtime PM subsystem will take care of power domain
>>> control at runtime suspend and resume.
>>>
>> Get it, thanks for your explain, I should have learned about this.
> 
> Sorry, i missed your subsequent question, and Tomasz has explained it now.

Never mind about that.

> Let me know if you have further questions.

Presently, no other questions. Thanks for your kind help.

Thanks
Yisheng
> 
> regards
> Vivek
>>
>> Thanks
>> Yisheng
>>
>>> Best regards,
>>> Tomasz
>>>
>>> .
>>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> .
> 

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

Re: [VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[Rob Clark]

On Wed, Apr 11, 2018 at 2:55 PM, Jordan Crouse  wrote:
> I've been struggling with a problem for a while and I haven't been able to 
> come
> up with a clean solution. Rob convinced me to stop complaining and do
> _something_ and hopefully this can spur a good discussion.
>
> The scenario is basically this: The MSM GPU wants to manage its own IOMMU
> virtual address space in lieu of using the DMA API to do so. The most 
> important
> reason is that when per-instance pagetables [1] are enabled each file 
> descriptor
> uses its own pagetable which are dynamically switched between contexts. In
> conjunction with this we use a split pagetable scheme to allow global buffers
> be persistently mapped so even a single pagetable has multiple ranges that 
> need
> to be utilized based on buffer type.
>
> Obviously the DMA API isn't suitable for this kind of specialized use. We want
> to push it off to the side, allocate our own domain, and use it directly with
> the IOMMU APIs. In a perfect world we would just not use the DMA API and 
> attach
> our own domain and that would be the end of the story. Unfortunately this is 
> not
> a perfect world.
>
> In order to switch the pagetable from the GPU the SoC has some special wiring 
> so
> that the GPU can reprogram the TTBR0 register in the IOMMU to the appropriate
> value. For a variety of reasons the hardware is hardcoded to only be able to
> access context bank 0 in the SMMU device. Long story short; if the DMA
> domain is allocated during bus enumeration and attached to context bank 0 then
> by the time we get to the driver domain we have long since lost our chance to
> get the context bank we need.
>
> After going back and forth and trying a few possible options it seems like the
> "easiest" solution" is to skip attaching the DMA domain in the first place. 
> But
> we still need to create a default domain and set up the IOMMU groups correctly
> so that when the GPU driver comes along it can attach the device and 
> everything
> will Just Work (TM). Rob suggested that we should use a blacklist of devices
> that choose to not participate so thats what I'm offering as a starting point
> for discussion.

in an ideal world, the dma-mapping layer magic would not be forced on
a driver, but would be an opt-in thing (ie. driver's ->probe() calls
dma_setup_magic_iommu_stuff(dev)), and becomes more of a helper layer
that drivers could opt-in to.. but that changes how things work
significantly at iommu probe time and probably touches a lot of
drivers.  A property in dt would be the easy down-stream solution, but
this is mostly about the driver (but also about how the firmware
works, so maybe dt is not out of bounds?).  The black-list approach is
fairly simple, although maybe a bit of a point-solution.  (Although
the number of drivers this problem applies to might not be much
greater than 1.)

So short version, very interested if anyone has better suggestions.
Because we really need to do *something* here.

BR,
-R

> The first patch in this series allows the specific IOMMU driver to gracefully
> skip attaching a device by returning -ENOSUPP. In that case, the core will
> skip printing error messages and it won't attach the domain to the group but
> it still allows the group to get created so that the IOMMU device is properly
> set up. In arch_setup_dma_ops() the call to iommu_get_domain_for_dev() will
> return NULL and the IOMMU ops won't be set up.
>
> The second patch implements the blacklist in arm-smmu.c based on the 
> compatible
> string of the GPU device and returns -ENOTSUPP.
>
> I tested this with my current per-instance pagetable stack and it does the job
> but I am very much in the market for better ideas.
>
> [1] https://patchwork.freedesktop.org/series/38729/
>
> Jordan Crouse (2):
>   iommu: Gracefully allow drivers to not attach to a default domain
>   iommu/arm-smmu: Add list of devices to opt out of DMA domains
>
>  drivers/iommu/arm-smmu.c | 23 +++
>  drivers/iommu/iommu.c| 18 ++
>  2 files changed, 37 insertions(+), 4 deletions(-)
>
> --
> 2.16.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RFC 1/1] iommu/of: Deconfigure iommu on driver detach

[Vivek Gautam]

Hi Robin,


On 4/3/2018 4:27 PM, Robin Murphy wrote:

On 28/03/18 11:25, Vivek Gautam wrote:

As part of dma_deconfigure, lets deconfigure the iommu too
on driver detach, so that we clear the iommu domain and
related group.

Signed-off-by: Vivek Gautam 
---

As a part of dma_deconfigure, shouldn't we deconfigure the iommu
as well? This should reverse all that we do in of_iommu_configure.
So that we call the .remove_device ops for iommu and eventually
clear all the iommu domain, related group infrastructure.
I am seeing that the loadable modules get the same iommu configurations
after re-loading them, i.e. iommu domain, and iova_domain 
configurations,

as we didn't cleared them.
So should we be clearing these configurations, and therefore do a
of_iommu_deconfigure() or sort?


Nope. Unbinding a driver does not cause the device to stop existing, 
nor remove it from its parent bus, which is what .remove_device 
represents. Device grouping is based on the underlying hardware 
topology, which isn't going to change at runtime, so there's little 
point in the software state pretending otherwise. If a module is 
leaking DMA mappings in the default domain when unloaded, that's a bug 
in that module (and certainly not specific to the IOMMU layer).


Thanks for your review, and really sorry for the delayed response.
I can follow what you said. So the device will only be removed (by 
.remove_device) when there's a BUS_NOTIFY_REMOVED_DEVICE notifier call.

We can drop this patch.

Best regards
Vivek



For reference, the only reason we touch .add_device in 
of_iommu_configure() is because iommu_bus_init() is not quite reliable 
enough for multiple IOMMU instances serving the platform bus (or 
similar). It's an ugly workaround which should go away if and when we 
manage to get rid of the notion of per-bus ops in the API.


Robin.



  drivers/iommu/of_iommu.c | 11 +++
  drivers/of/device.c  |  1 +
  include/linux/of_iommu.h |  5 +
  3 files changed, 17 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5c36a8b7656a..1a23e6204ade 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -160,6 +160,17 @@ static int of_pci_iommu_init(struct pci_dev 
*pdev, u16 alias, void *data)

  return err;
  }
  +void of_iommu_deconfigure(struct device *dev)
+{
+    const struct iommu_ops *ops = NULL;
+
+    if (dev->iommu_fwspec && dev->iommu_fwspec->ops)
+    ops = dev->iommu_fwspec->ops;
+
+    if (ops && ops->remove_device && dev->iommu_group)
+    ops->remove_device(dev);
+}
+
  const struct iommu_ops *of_iommu_configure(struct device *dev,
 struct device_node *master_np)
  {
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 064c818105bd..e13cb7914dbe 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -177,6 +177,7 @@ EXPORT_SYMBOL_GPL(of_dma_configure);
  void of_dma_deconfigure(struct device *dev)
  {
  arch_teardown_dma_ops(dev);
+    of_iommu_deconfigure(dev);
  }
    int of_device_register(struct platform_device *pdev)
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 4fa654e4b5a9..3d4c22e95c0c 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -15,6 +15,8 @@ extern int of_get_dma_window(struct device_node 
*dn, const char *prefix,

  extern const struct iommu_ops *of_iommu_configure(struct device *dev,
  struct device_node *master_np);
  +extern void of_iommu_deconfigure(struct device *dev);
+
  #else
    static inline int of_get_dma_window(struct device_node *dn, const 
char *prefix,
@@ -30,6 +32,9 @@ static inline const struct iommu_ops 
*of_iommu_configure(struct device *dev,

  return NULL;
  }
  +static inline void of_iommu_deconfigure(struct device *dev)
+{ }
+
  #endif    /* CONFIG_OF_IOMMU */
    extern struct of_device_id __iommu_of_table;



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

[PATCH 2/2] iommu/arm-smmu: Add list of devices to opt out of DMA domains

[Jordan Crouse]

Add a list of compatible strings for devices that wish to opt out
of attaching to a DMA domain.  This is for devices that prefer to
manage their own IOMMU space for any number of reasons. Returning
ENOTSUPP for attach device will filter down and force
arch_setup_dma_ops() to not set up the iommu DMA ops. Later
the client device in question can set up and attach their own
domain.

Signed-off-by: Jordan Crouse 
---
 drivers/iommu/arm-smmu.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60792a8..09fa03a15f4f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1187,6 +1187,15 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
return 0;
 }
 
+/*
+ * This is a list of compatible strings for devices that wish to manage their
+ * own IOMMU space instead of the DMA IOMMU ops. Devices on this list will not
+ * allow themselves to be attached to a IOMMU_DOMAIN_DMA domain
+ */
+static const char * const arm_smmu_dma_blacklist[] = {
+   "qcom,adreno",
+};
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
int ret;
@@ -1209,6 +1218,20 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
if (!fwspec->iommu_priv)
return -ENODEV;
 
+   /*
+* If this is the dfeault DMA domain, check to see if the device is on
+* the blacklist and reject if so
+*/
+   if (domain->type == IOMMU_DOMAIN_DMA && dev->of_node) {
+   int i;
+
+   for (i = 0; i < ARRAY_SIZE(arm_smmu_dma_blacklist); i++) {
+   if (of_device_is_compatible(dev->of_node,
+   arm_smmu_dma_blacklist[i]))
+   return -ENOTSUPP;
+   }
+   }
+
smmu = fwspec_smmu(fwspec);
/* Ensure that the domain is finalised */
ret = arm_smmu_init_domain_context(domain, smmu);
-- 
2.16.1

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

[PATCH 1/2] iommu: Gracefully allow drivers to not attach to a default domain

[Jordan Crouse]

Provide individual device drivers the chance to gracefully refuse
to attach a device to the default domain. If the attach_device
op returns -ENOTSUPP don't print a error message and don't set
group->domain but still return success from iommu_group_add_dev().

This allows all the usual APIs to work and the next domain to try
to attach will take group->domain for itself and everything will
proceed as normal.

Signed-off-by: Jordan Crouse 
---
 drivers/iommu/iommu.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 69fef991c651..2403cce138d2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -592,7 +592,7 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
if (group->domain)
ret = __iommu_attach_device(group->domain, dev);
mutex_unlock(>mutex);
-   if (ret)
+   if (ret && ret != -ENOTSUPP)
goto err_put_group;
 
/* Notify any listeners about change to group. */
@@ -617,7 +617,9 @@ int iommu_group_add_device(struct iommu_group *group, 
struct device *dev)
sysfs_remove_link(>kobj, "iommu_group");
 err_free_device:
kfree(device);
-   pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), 
group->id, ret);
+   if (ret != -ENOTSUPP)
+   pr_err("Failed to add device %s to group %d: %d\n",
+   dev_name(dev), group->id, ret);
return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -1039,8 +1041,16 @@ struct iommu_group *iommu_group_get_for_dev(struct 
device *dev)
 
ret = iommu_group_add_device(group, dev);
if (ret) {
-   iommu_group_put(group);
-   return ERR_PTR(ret);
+   /*
+* If the driver chooses not to bind the device, reset
+* group->domain so a new domain can be added later
+*/
+   if (ret == -ENOTSUPP)
+   group->domain = NULL;
+   else {
+   iommu_group_put(group);
+   return ERR_PTR(ret);
+   }
}
 
return group;
-- 
2.16.1

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

[VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[Jordan Crouse]

I've been struggling with a problem for a while and I haven't been able to come
up with a clean solution. Rob convinced me to stop complaining and do
_something_ and hopefully this can spur a good discussion.

The scenario is basically this: The MSM GPU wants to manage its own IOMMU
virtual address space in lieu of using the DMA API to do so. The most important
reason is that when per-instance pagetables [1] are enabled each file descriptor
uses its own pagetable which are dynamically switched between contexts. In
conjunction with this we use a split pagetable scheme to allow global buffers 
be persistently mapped so even a single pagetable has multiple ranges that need
to be utilized based on buffer type.

Obviously the DMA API isn't suitable for this kind of specialized use. We want
to push it off to the side, allocate our own domain, and use it directly with
the IOMMU APIs. In a perfect world we would just not use the DMA API and attach
our own domain and that would be the end of the story. Unfortunately this is not
a perfect world.

In order to switch the pagetable from the GPU the SoC has some special wiring so
that the GPU can reprogram the TTBR0 register in the IOMMU to the appropriate
value. For a variety of reasons the hardware is hardcoded to only be able to
access context bank 0 in the SMMU device. Long story short; if the DMA
domain is allocated during bus enumeration and attached to context bank 0 then
by the time we get to the driver domain we have long since lost our chance to
get the context bank we need.

After going back and forth and trying a few possible options it seems like the
"easiest" solution" is to skip attaching the DMA domain in the first place. But
we still need to create a default domain and set up the IOMMU groups correctly
so that when the GPU driver comes along it can attach the device and everything
will Just Work (TM). Rob suggested that we should use a blacklist of devices
that choose to not participate so thats what I'm offering as a starting point
for discussion.

The first patch in this series allows the specific IOMMU driver to gracefully
skip attaching a device by returning -ENOSUPP. In that case, the core will
skip printing error messages and it won't attach the domain to the group but
it still allows the group to get created so that the IOMMU device is properly
set up. In arch_setup_dma_ops() the call to iommu_get_domain_for_dev() will
return NULL and the IOMMU ops won't be set up.

The second patch implements the blacklist in arm-smmu.c based on the compatible
string of the GPU device and returns -ENOTSUPP.

I tested this with my current per-instance pagetable stack and it does the job
but I am very much in the market for better ideas.

[1] https://patchwork.freedesktop.org/series/38729/

Jordan Crouse (2):
  iommu: Gracefully allow drivers to not attach to a default domain
  iommu/arm-smmu: Add list of devices to opt out of DMA domains

 drivers/iommu/arm-smmu.c | 23 +++
 drivers/iommu/iommu.c| 18 ++
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.16.1

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

Re: [PATCH 1/4] iommu: Add virtio-iommu driver

[Jean-Philippe Brucker]

On 23/03/18 08:27, Tian, Kevin wrote:
>>> The host kernel needs to have *some* MSI region in place before the
>>> guest can start configuring interrupts, otherwise it won't know what
>>> address to give to the underlying hardware. However, as soon as the host
>>> kernel has picked a region, host userspace needs to know that it can no
>>> longer use addresses in that region for DMA-able guest memory. It's a
>>> lot easier when the address is fixed in hardware and the host userspace
>>> will never be stupid enough to try and VFIO_IOMMU_DMA_MAP it, but in
>>> the
>>> more general case where MSI writes undergo IOMMU address translation
>>> so
>>> it's an arbitrary IOVA, this has the potential to conflict with stuff
>>> like guest memory hotplug.
>>>
>>> What we currently have is just the simplest option, with the host kernel
>>> just picking something up-front and pretending to host userspace that
>>> it's a fixed hardware address. There's certainly scope for it to be a
>>> bit more dynamic in the sense of adding an interface to let userspace
>>> move it around (before attaching any devices, at least), but I don't
>>> think it's feasible for the host kernel to second-guess userspace enough
>>> to make it entirely transparent like it is in the DMA API domain case.
>>>
>>> Of course, that's all assuming the host itself is using a virtio-iommu
>>> (e.g. in a nested virt or emulation scenario). When it's purely within a
>>> guest then an MSI reservation shouldn't matter so much, since the guest
>>> won't be anywhere near the real hardware configuration anyway.
>>>
>>> Robin.
>>
>> Curious since anyway we are defining a new iommu architecture
>> is it possible to avoid those ARM-specific burden completely?
>>
> 
> OK, after some study around those tricks below is my learning:
> 
> - MSI_IOVA window is used only on request (iommu_dma_get
> _msi_page), not meant to take effect on all architectures once 
> initialized. e.g. ARM GIC does it but not x86. So it is reasonable 
> for virtio-iommu driver to implement such capability;
> 
> - I thought whether hardware MSI doorbell can be always reported
> on virtio-iommu since it's newly defined. Looks there is a problem
> if underlying IOMMU is sw-managed MSI style - valid mapping is
> expected in all level of translations, meaning guest has to manage
> stage-1 mapping in nested configuration since stage-1 is owned
> by guest. 
> 
> Then virtio-iommu is naturally expected to report the same MSI 
> model as supported by underlying hardware. Below are some
> further thoughts along this route (use 'IOMMU' to represent the
> physical one and 'virtio-iommu' for virtual one):
> 
> 
> 
> In the scope of current virtio-iommu spec v.6, there is no nested
> consideration yet. Guest driver is expected to use MAP/UNMAP
> interface on assigned endpoints. In this case the MAP requests
> (IOVA->GPA) is caught and maintained within Qemu which then 
> further talks to VFIO to map IOVA->HPA in IOMMU.
> 
> Qemu can learn the MSI model of IOMMU from sysfs.
> 
> For hardware MSI doorbell (x86 and some ARM):
> * Host kernel reports to Qemu as IOMMU_RESV_MSI
> * Qemu report to guest as VIRTIO_IOMMU_RESV_MEM_T_MSI
> * Guest takes the range as IOMMU_RESV_MSI. reserved
> * Qemu MAP database has no mapping for the doorbell
> * Physical IOMMU page table has no mapping for the doorbell
> * MSI from passthrough device bypass IOMMU
> * MSI from emulated device bypass virtio-iommu
> 
> For software MSI doorbell (most ARM):
> * Host kernel reports to Qemu as IOMMU_RESV_SW_MSI
> * Qemu report to guest as VIRTIO_IOMMU_RESV_MEM_T_RESERVED
> * Guest takes the range as IOMMU_RESV_RESERVED
> * vGIC requests to map 'GPA of the virtual doorbell'
> * a map request (IOVA->GPA) sent on endpoint
> * Qemu maintains the mapping in MAP database
>   * but no VFIO_MAP request since it's purely virtual
> * GIC requests to map 'HPA of the physical doorbell'
>   * e.g. triggered by VFIO enable msi
> * IOMMU now includes a valid mapping (IOVA->HPA)
> * MSI from emulated device go through Qemu MAP
> database (IOVA->'GPA of virtual doorbell') and then hit vGIC
> * MSI from passthrough device go through IOMMU
> (IOVA->'HPA of physical doorbell') and then hit GIC
> 
> In this case, host doorbell is treated as reserved resource in
> guest side. Guest has its own sw-management for virtual
> doorbell which is only used for emulated device. two paths 
> are completely separated.
> 
> If above captures the right flow, current v0.6 spec is complete
> regarding to required function definition.

Yes I think this summarizes well the current state or SW/HW MSI

> Then comes nested case, with two level page tables (stage-1
> and stage-2) in IOMMU. stage-1 is for IOVA->GPA and stage-2
> is for GPA->HPA. VFIO map/unmap happens on stage-2, 
> while stage-1 is directly managed by guest (and bound to
> IOMMU which enables nested translation from IOVA->GPA
> ->HPA).
> 
> For hardware MSI, there is nothing special compared to
> 

Re: [PATCH 2/4] iommu/virtio: Add probe request

[Jean-Philippe Brucker]

On 23/03/18 15:00, Robin Murphy wrote:
[...]
>> +/*
>> + * Treat unknown subtype as RESERVED, but urge users to update their
>> + * driver.
>> + */
>> +if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
>> +mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI)
>> +pr_warn("unknown resv mem subtype 0x%x\n", mem->subtype);
> 
> Might as well avoid the extra comparisons by incorporating this into the 
> switch statement, i.e.:
> 
>   default:
>   dev_warn(vdev->viommu_dev->dev, ...);
>   /* Fallthrough */
>   case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
>   ...
> 
> (dev_warn is generally preferable to pr_warn when feasible)

Alright, that's nicer

[...]
>>  /*
>>   * Last step creates a default domain and attaches to it. Everything
>>   * must be ready.
>> @@ -735,7 +849,19 @@ static int viommu_add_device(struct device *dev)
>>   
>>   static void viommu_remove_device(struct device *dev)
>>   {
>> -kfree(dev->iommu_fwspec->iommu_priv);
>> +struct viommu_endpoint *vdev;
>> +struct iommu_resv_region *entry, *next;
>> +struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +
>> +if (!fwspec || fwspec->ops != _ops)
>> +return;
> 
> Oh good :) I guess that was just a patch-splitting issue. The group 
> thing still applies, though.

Ok

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

Re: [PATCH 1/4] iommu: Add virtio-iommu driver

[Jean-Philippe Brucker]

On 23/03/18 14:48, Robin Murphy wrote:
[..]
>> + * Virtio driver for the paravirtualized IOMMU
>> + *
>> + * Copyright (C) 2018 ARM Limited
>> + * Author: Jean-Philippe Brucker 
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
> 
> This wants to be a // comment at the very top of the file (thankfully 
> the policy is now properly documented in-tree since 
> Documentation/process/license-rules.rst got merged)

Ok

[...]
>> +
>> +/*
>> + * viommu_del_mappings - remove mappings from the internal tree
>> + *
>> + * @vdomain: the domain
>> + * @iova: start of the range
>> + * @size: size of the range. A size of 0 corresponds to the entire address
>> + *  space.
>> + * @out_mapping: if not NULL, the first removed mapping is returned in 
>> there.
>> + *  This allows the caller to reuse the buffer for the unmap request. When
>> + *  the returned size is greater than zero, if a mapping is returned, the
>> + *  caller must free it.
> 
> This "free multiple mappings except maybe hand one of them off to the 
> caller" interface is really unintuitive. AFAICS it's only used by 
> viommu_unmap() to grab mapping->req, but that doesn't seem to care about 
> mapping itself, so I wonder whether it wouldn't make more sense to just 
> have a global kmem_cache of struct virtio_iommu_req_unmap for that and 
> avoid a lot of complexity...

Well it's a small complication for what I hoped would be a meanignful
performance difference, but more below.

>> +
>> +/* IOMMU API */
>> +
>> +static bool viommu_capable(enum iommu_cap cap)
>> +{
>> +return false;
>> +}
> 
> The .capable callback is optional, so it's only worth implementing once 
> you want it to do something beyond the default behaviour.
> 

Ah, right

[...]
>> +static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
>> +   size_t size)
>> +{
>> +int ret = 0;
>> +size_t unmapped;
>> +struct viommu_mapping *mapping = NULL;
>> +struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> +unmapped = viommu_del_mappings(vdomain, iova, size, );
>> +if (unmapped < size) {
>> +ret = -EINVAL;
>> +goto out_free;
>> +}
>> +
>> +/* Device already removed all mappings after detach. */
>> +if (!vdomain->endpoints)
>> +goto out_free;
>> +
>> +if (WARN_ON(!mapping))
>> +return 0;
>> +
>> +mapping->req.unmap = (struct virtio_iommu_req_unmap) {
>> +.head.type  = VIRTIO_IOMMU_T_UNMAP,
>> +.domain = cpu_to_le32(vdomain->id),
>> +.virt_start = cpu_to_le64(iova),
>> +.virt_end   = cpu_to_le64(iova + unmapped - 1),
>> +};
> 
> ...In fact, the kmem_cache idea might be moot since it looks like with a 
> bit of restructuring you could get away with just a single per-viommu 
> virtio_iommu_req_unmap structure; this lot could be passed around on the 
> stack until request_lock is taken, at which point it would be copied 
> into the 'real' DMA-able structure. The equivalent might apply to 
> viommu_map() too - now that I'm looking at it, it seems like it would 
> take pretty minimal effort to encapsulate the whole business cleanly in 
> viommu_send_req_sync(), which could do something like this instead of 
> going through viommu_send_reqs_sync():
> 
>   ...
>   spin_lock_irqsave(>request_lock, flags);
>   viommu_copy_req(viommu->dma_req, req);
>   ret = _viommu_send_reqs_sync(viommu, viommu->dma_req, 1, );
>   spin_unlock_irqrestore(>request_lock, flags);
>   ...

I'll have to come back to that sorry, still conducting some experiments
with map/unmap.

I'd rather avoid introducing a single dma_req per viommu, because I'd
like to move to the iotlb_range_add/iotlb_sync interface as soon as
possible, and the request logic changes a lot when multiple threads are
susceptible to interleave map/unmap requests.

I ran some tests, and adding a kmem_cache (or simply using kmemdup, it
doesn't make a noticeable difference at our scale) reduces netperf
stream/maerts throughput by 1.1%/1.4% (+/- 0.5% over 30 tests). That's
for a virtio-net device (1 tx/rx vq), and with a vfio device the
difference isn't measurable. At this point I'm not fussy about such
small difference, so don't mind simplifying viommu_del_mapping.

[...]
>> +/*
>> + * Last step creates a default domain and attaches to it. Everything
>> + * must be ready.
>> + */
>> +group = iommu_group_get_for_dev(dev);
>> +if (!IS_ERR(group))
>> +iommu_group_put(group);
> 
> Since you create the sysfs IOMMU device, maybe also create the links for 
> the masters?

Ok

>> +
>> +return PTR_ERR_OR_ZERO(group);
>> +}
>> +
>> +static void viommu_remove_device(struct device *dev)
>> +{
> 
> You need to remove dev from its group, too (basically, .remove_device 
> should always undo everything .add_device did)
> 
> It would also be good practice to verify that 

Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Marc Zyngier]

Hi Sammer,

On 11/04/18 16:58, Goel, Sameer wrote:
> 
> 
> On 3/28/2018 9:00 AM, Marc Zyngier wrote:
>> On 2018-03-28 15:39, Timur Tabi wrote:
>>> From: Sameer Goel 
>>>
>>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>>> when SMMUEN==0.
>>>
>>> This prevents a race condition where a stray DMA from the crashed primary
>>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>>> disabled during reset in the crash kernel.
>>>
>>> Signed-off-by: Sameer Goel 
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 12 
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index 3f2f1fc68b52..c04a89310c59 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct
>>> arm_smmu_device *smmu, bool bypass)
>>>  if (reg & CR0_SMMUEN)
>>>  dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>>
>>> +    /*
>>> + * Abort all incoming translations. This can happen in a kdump case
>>> + * where SMMU is initialized when a prior DMA is pending. Just
>>> + * disabling the SMMU in this case might result in writes to invalid
>>> + * PAs.
>>> + */
>>> +    ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>>> +    if (ret) {
>>> +    dev_err(smmu->dev, "GBPA not responding to update\n");
>>> +    return ret;
>>> +    }
>>> +
>>>  ret = arm_smmu_device_disable(smmu);
>>>  if (ret)
>>>  return ret;
>>
>> A tangential question: can we reliably detect that the SMMU already
>> has valid mappings, which would indicate that we're in a pretty bad
>> shape already by the time we set that bit? For all we know, memory
>> could have been corrupted long before we hit this point, and this
>> patch barely narrows the window of opportunity.
>
> :) Yes that is correct. This only covers the kdump scenario. Trying
> to get some reliability when booting up the crash kernel. The system
> is already in a bad state. I don't think that this will happen in a
> normal scenario. But please point me to the GICv3 change and I'll
> have a look.

See this:
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407

M.
-- 
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Goel, Sameer]

On 3/28/2018 9:00 AM, Marc Zyngier wrote:
> On 2018-03-28 15:39, Timur Tabi wrote:
>> From: Sameer Goel 
>>
>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>> when SMMUEN==0.
>>
>> This prevents a race condition where a stray DMA from the crashed primary
>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>> disabled during reset in the crash kernel.
>>
>> Signed-off-by: Sameer Goel 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 3f2f1fc68b52..c04a89310c59 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct
>> arm_smmu_device *smmu, bool bypass)
>>  if (reg & CR0_SMMUEN)
>>  dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>
>> +    /*
>> + * Abort all incoming translations. This can happen in a kdump case
>> + * where SMMU is initialized when a prior DMA is pending. Just
>> + * disabling the SMMU in this case might result in writes to invalid
>> + * PAs.
>> + */
>> +    ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>> +    if (ret) {
>> +    dev_err(smmu->dev, "GBPA not responding to update\n");
>> +    return ret;
>> +    }
>> +
>>  ret = arm_smmu_device_disable(smmu);
>>  if (ret)
>>  return ret;
> 
> A tangential question: can we reliably detect that the SMMU already has valid 
> mappings, which would indicate that we're in a pretty bad shape already by 
> the time we set that bit? For all we know, memory could have been corrupted 
> long before we hit this point, and this patch barely narrows the window of 
> opportunity.
:) Yes that is correct. This only covers the kdump scenario. Trying to get some 
reliability when booting up the crash kernel. The system is already in a bad 
state. I don't think that this will happen in a normal scenario. But please 
point me to the GICv3 change and I'll have a look.
Thanks,
Sameer 
> 
> At the very least, we should emit a warning and taint the kernel (we've 
> recently added such measures to the GICv3 driver).
> 
> Thanks,
> 
>     M.

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Goel, Sameer]

On 4/5/2018 5:26 AM, Will Deacon wrote:
> On Wed, Mar 28, 2018 at 09:39:40AM -0500, Timur Tabi wrote:
>> From: Sameer Goel 
>>
>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>> when SMMUEN==0.
>>
>> This prevents a race condition where a stray DMA from the crashed primary
>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>> disabled during reset in the crash kernel.
>>
>> Signed-off-by: Sameer Goel 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 3f2f1fc68b52..c04a89310c59 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct 
>> arm_smmu_device *smmu, bool bypass)
>>  if (reg & CR0_SMMUEN)
>>  dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>  
>> +/*
>> + * Abort all incoming translations. This can happen in a kdump case
>> + * where SMMU is initialized when a prior DMA is pending. Just
>> + * disabling the SMMU in this case might result in writes to invalid
>> + * PAs.
>> + */
>> +ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>> +if (ret) {
>> +dev_err(smmu->dev, "GBPA not responding to update\n");
>> +return ret;
>> +}
> 
> This needs to be predicated on the disable_bypass option, otherwise I think
> it will cause regressions for systems that rely on passthrough.
Ok, I'll make the change.
> 
> Will
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[git pull] IOMMU Updates for Linux v4.17

[Joerg Roedel]

Hi Linus,

The following changes since commit 3eb2ce825ea1ad89d20f7a3b5780df850e4be274:

  Linux 4.16-rc7 (2018-03-25 12:44:30 -1000)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git 
tags/iommu-updates-v4.17

for you to fetch changes up to d4f96fd5c249defda290299f9646287dd3df0803:

  Merge branches 'x86/amd', 'x86/vt-d', 'arm/rockchip', 'arm/omap', 
'arm/mediatek', 'arm/exynos', 'arm/renesas', 'arm/smmu' and 'core' into next 
(2018-03-29 15:24:40 +0200)


IOMMU Updates for Linux v4.17

These updates come with:

- OF_IOMMU support for the Rockchip iommu driver so that it can
  use generic DT bindings

- Rework of locking in the AMD IOMMU interrupt remapping code to
  make it work better in RT kernels

- Support for improved iotlb flushing in the AMD IOMMU driver

- Support for 52-bit physical and virtual addressing in the
  ARM-SMMU

- Various other small fixes and cleanups


Biju Das (1):
  dt-bindings: iommu: ipmmu-vmsa: Add device tree support for r8a774[35]

Dmitry Safonov (2):
  iommu/vt-d: Add __init for dmar_register_bus_notifier()
  iommu/vt-d: Clean/document fault status flags

Gary R Hook (1):
  iommu/amd: Use dev_err to send events to the system log

Jeffy Chen (10):
  iommu/omap: Increase group ref in .device_group()
  iommu/rockchip: Prohibit unbind and remove
  iommu/rockchip: Fix error handling in probe
  iommu/rockchip: Request irqs in rk_iommu_probe()
  dt-bindings: iommu/rockchip: Add clock property
  iommu/rockchip: Use IOMMU device for dma mapping operations
  iommu/rockchip: Use OF_IOMMU to attach devices automatically
  iommu/rockchip: Fix error handling in init
  iommu/rockchip: Add runtime PM support
  iommu/rockchip: Support sharing IOMMU between masters

Joerg Roedel (2):
  Merge branch 'for-joerg/arm-smmu/updates' of 
git://git.kernel.org/.../will/linux into arm/smmu
  Merge branches 'x86/amd', 'x86/vt-d', 'arm/rockchip', 'arm/omap', 
'arm/mediatek', 'arm/exynos', 'arm/renesas', 'arm/smmu' and 'core' into next

Lu Baolu (2):
  iommu/vt-d: Fix a potential memory leak
  iommu/vt-d: Use real PASID for flush in caching mode

Marc Zyngier (1):
  iommu/rockchip: Perform a reset on shutdown

Nate Watterson (1):
  iommu/arm-smmu-v3: limit reporting of MSI allocation failures

Robin Murphy (10):
  iommu/exynos: Use generic group callback
  iommu/arm-smmu-v3: Warn about missing IRQs
  iommu/arm-smmu-v3: Clean up address masking
  iommu/arm-smmu-v3: Clean up register definitions
  iommu/arm-smmu-v3: Clean up table definitions
  iommu/arm-smmu-v3: Clean up queue definitions
  iommu/io-pgtable-arm: Support 52-bit physical address
  iommu/arm-smmu-v3: Support 52-bit physical address
  iommu/arm-smmu-v3: Support 52-bit virtual address
  iommu/io-pgtable-arm: Avoid warning with 32-bit phys_addr_t

Scott Wood (3):
  iommu/amd: Use raw locks on atomic context paths
  iommu/amd: Don't use dev_data in irte_ga_set_affinity()
  iommu/amd: Avoid locking get_irq_table() from atomic context

Sebastian Andrzej Siewior (10):
  iommu/amd: Take into account that alloc_dev_data() may return NULL
  iommu/amd: Turn dev_data_list into a lock less list
  iommu/amd: Split domain id out of amd_iommu_devtable_lock
  iommu/amd: Split irq_lookup_table out of the amd_iommu_devtable_lock
  iommu/amd: Remove the special case from alloc_irq_table()
  iommu/amd: Use `table' instead `irt' as variable name in 
amd_iommu_update_ga()
  iommu/amd: Factor out setting the remap table for a devid
  iommu/amd: Drop the lock while allocating new irq remap table
  iommu/amd: Make amd_iommu_devtable_lock a spin_lock
  iommu/amd: Return proper error code in irq_remapping_alloc()

Shameer Kolothum (2):
  ACPI/IORT: Add msi address regions reservation helper
  iommu/dma: Add HW MSI(GICv3 ITS) address regions reservation

Shaokun Zhang (1):
  iommu/vt-d:Remove unused variable

Suravee Suthikulpanit (2):
  iommu: Do not return error code for APIs with size_t return type
  iommu/amd: Add support for fast IOTLB flushing

Tomasz Figa (4):
  iommu/rockchip: Fix error handling in attach
  iommu/rockchip: Use iopoll helpers to wait for hardware
  iommu/rockchip: Fix TLB flush of secondary IOMMUs
  iommu/rockchip: Control clocks needed to access the IOMMU

Vivek Gautam (1):
  iommu/io-pgtable: Use size_t return type for all foo_unmap

Yong Wu (2):
  iommu/mediatek: Move attach_device after iommu-group is ready for M4Uv1
  iommu/mediatek: Fix protect memory setting

 .../bindings/iommu/renesas,ipmmu-vmsa.txt  |   5 +-
 .../devicetree/bindings/iommu/rockchip,iommu.txt   |   7 +
 

Re: [PATCH V2] drm/amdgpu: limit DMA size to PAGE_SIZE for scatter-gather buffers

[Robin Murphy]

On 11/04/18 15:33, Sinan Kaya wrote:

On 4/11/2018 8:03 AM, Robin Murphy wrote:

On 10/04/18 21:59, Sinan Kaya wrote:

Code is expecing to observe the same number of buffers returned
from dma_map_sg() function compared to
sg_alloc_table_from_pages(). This doesn't hold true universally
especially for systems with IOMMU.


So why not fix said code? It's clearly not a real hardware
limitation, and the map_sg() APIs have potentially returned fewer
than nents since forever, so there's really no excuse.


Sure, I'll take a better fix if there is one.




IOMMU driver tries to combine buffers into a single DMA address
as much as it can. The right thing is to tell the DMA layer how
much combining IOMMU can do.


Disagree; this is a dodgy hack, since you'll now end up passing
scatterlists into dma_map_sg() which already violate max_seg_size
to begin with, and I think a conscientious DMA API implementation
would be at rights to fail the mapping for that reason (I know
arm64 happens not to, but that was a deliberate design decision to
make my life easier at the time).

As a short-term fix, at least do something like what i915 does and
constrain the table allocation to the desired segment size as well,
so things remain self-consistent. But still never claim that faking
a hardware constraint as a workaround for a driver shortcoming is
"the right thing to do" ;)


You are asking for something like this from here, right?

https://elixir.bootlin.com/linux/v4.16.1/source/drivers/gpu/drm/i915/i915_gem_dmabuf.c#L58


I just looked for callers of __sg_alloc_table_from_pages() with a limit 
other than SCATTERLIST_MAX_SEGMENT, and found 
__i915_gem_userptr_alloc_pages(), which at first glance appears to be 
doing something vaguely similar to amdgpu_ttm_tt_pin_userptr().


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

Re: [PATCH V2] drm/amdgpu: limit DMA size to PAGE_SIZE for scatter-gather buffers

[Sinan Kaya]

On 4/11/2018 8:03 AM, Robin Murphy wrote:
> On 10/04/18 21:59, Sinan Kaya wrote:
>> Code is expecing to observe the same number of buffers returned from
>> dma_map_sg() function compared to sg_alloc_table_from_pages(). This
>> doesn't hold true universally especially for systems with IOMMU.
> 
> So why not fix said code? It's clearly not a real hardware limitation, and 
> the map_sg() APIs have potentially returned fewer than nents since forever, 
> so there's really no excuse.

Sure, I'll take a better fix if there is one.

> 
>> IOMMU driver tries to combine buffers into a single DMA address as much
>> as it can. The right thing is to tell the DMA layer how much combining
>> IOMMU can do.
> 
> Disagree; this is a dodgy hack, since you'll now end up passing scatterlists 
> into dma_map_sg() which already violate max_seg_size to begin with, and I 
> think a conscientious DMA API implementation would be at rights to fail the 
> mapping for that reason (I know arm64 happens not to, but that was a 
> deliberate design decision to make my life easier at the time).
> 
> As a short-term fix, at least do something like what i915 does and constrain 
> the table allocation to the desired segment size as well, so things remain 
> self-consistent. But still never claim that faking a hardware constraint as a 
> workaround for a driver shortcoming is "the right thing to do" ;)

You are asking for something like this from here, right?

https://elixir.bootlin.com/linux/v4.16.1/source/drivers/gpu/drm/i915/i915_gem_dmabuf.c#L58


ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);
if (ret)
goto err_free;

src = obj->mm.pages->sgl;
dst = st->sgl;
for (i = 0; i < obj->mm.pages->nents; i++) {
sg_set_page(dst, sg_page(src), src->length, 0);
dst = sg_next(dst);
src = sg_next(src);
}

This seems to allocate the scatter gather list and fill it in manually before 
passing it
to dma_map_sg(). I'll give it a try. 

Just double checking.

> 
> Robin.
> 
>> Signed-off-by: Sinan Kaya 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] drm/amdgpu: limit DMA size to PAGE_SIZE for scatter-gather buffers

[Robin Murphy]

On 10/04/18 21:59, Sinan Kaya wrote:

Code is expecing to observe the same number of buffers returned from
dma_map_sg() function compared to sg_alloc_table_from_pages(). This
doesn't hold true universally especially for systems with IOMMU.


So why not fix said code? It's clearly not a real hardware limitation, 
and the map_sg() APIs have potentially returned fewer than nents since 
forever, so there's really no excuse.



IOMMU driver tries to combine buffers into a single DMA address as much
as it can. The right thing is to tell the DMA layer how much combining
IOMMU can do.


Disagree; this is a dodgy hack, since you'll now end up passing 
scatterlists into dma_map_sg() which already violate max_seg_size to 
begin with, and I think a conscientious DMA API implementation would be 
at rights to fail the mapping for that reason (I know arm64 happens not 
to, but that was a deliberate design decision to make my life easier at 
the time).


As a short-term fix, at least do something like what i915 does and 
constrain the table allocation to the desired segment size as well, so 
things remain self-consistent. But still never claim that faking a 
hardware constraint as a workaround for a driver shortcoming is "the 
right thing to do" ;)


Robin.


Signed-off-by: Sinan Kaya 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 1 +
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 1 +
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 +
  4 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 8e28270..1b031eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -851,7 +851,7 @@ static int gmc_v6_0_sw_init(void *handle)
pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
}
-
+   dma_set_max_seg_size(adev->dev, PAGE_SIZE);
r = gmc_v6_0_init_microcode(adev);
if (r) {
dev_err(adev->dev, "Failed to load mc firmware!\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 86e9d682..0a4b2cc1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -999,6 +999,7 @@ static int gmc_v7_0_sw_init(void *handle)
pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
pr_warn("amdgpu: No coherent DMA available\n");
}
+   dma_set_max_seg_size(adev->dev, PAGE_SIZE);
  
  	r = gmc_v7_0_init_microcode(adev);

if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 9a813d8..b171529 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1096,6 +1096,7 @@ static int gmc_v8_0_sw_init(void *handle)
pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
pr_warn("amdgpu: No coherent DMA available\n");
}
+   dma_set_max_seg_size(adev->dev, PAGE_SIZE);
  
  	r = gmc_v8_0_init_microcode(adev);

if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 3b7e7af..36e658ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -855,6 +855,7 @@ static int gmc_v9_0_sw_init(void *handle)
pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
}
+   dma_set_max_seg_size(adev->dev, PAGE_SIZE);
  
  	r = gmc_v9_0_mc_init(adev);

if (r)


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

Re: [PATCH] swiotlb: Fix unexpected swiotlb_alloc_coherent() failures

[Takashi Iwai]

On Tue, 10 Apr 2018 20:10:20 +0200,
Christoph Hellwig wrote:
> 
> On Tue, Apr 10, 2018 at 06:50:04PM +0100, Robin Murphy wrote:
> > In the first one, the machine appears to have enough RAM that most of it is 
> > beyond the device's 36-bit DMA mask, thus it's fairly likely for the 
> > initial direct alloc to come back with something unsuitable.
> 
> But we should try a GFP_DMA32 allocation first, so this is a bit
> surprising.

Hm, do we really try that?
Through a quick glance, dma_alloc_coherent_gfp_flags() gives GFP_DMA32
only when coherent mask <= DMA_BIT_MASK(32); in the case of iwlwifi,
it's 36bit, so GFP_DMA isn't set.

We had a fallback allocation with GFP_DMA32 in the past, but this
seems gone long time ago along with cleanups (commit c647c3bb2d16).

But I haven't followed about this topic for long time, so I might have
missed obviously...


thanks,

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