Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Joe Perches 
On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> It is only cosmetics but even the author got lost about the loose bracket.
> I would suggest to remove all the brackets or if needed to move the [ in the
> message. We have enought memory this days.

My suggestion would be to remove the separate dev_err
and use "Event log [" in each dev_err.

So here's an actual suggested patch that does a few things:

o Coalesce formats and realigns arguments
o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
o Remove embedded "AMD-Vi: " from various formats

Some existing pr_ uses now are prefixed too.
---
 drivers/iommu/amd_iommu.c | 69 +++
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..01583a8ccaf9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -17,6 +17,9 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt pr_fmt
+
 #include 
 #include 
 #include 
@@ -273,9 +276,8 @@ static u16 get_alias(struct device *dev)
return pci_alias;
}
 
-   pr_info("AMD-Vi: Using IVRS reported alias %02x:%02x.%d "
-   "for device %s[%04x:%04x], kernel reported alias "
-   "%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
+   pr_info("Using IVRS reported alias %02x:%02x.%d for device 
%s[%04x:%04x], kernel reported alias %02x:%02x.%d\n",
+   PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
PCI_FUNC(ivrs_alias), dev_name(dev), pdev->vendor, pdev->device,
PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
PCI_FUNC(pci_alias));
@@ -287,7 +289,7 @@ static u16 get_alias(struct device *dev)
if (pci_alias == devid &&
PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
pci_add_dma_alias(pdev, ivrs_alias & 0xff);
-   pr_info("AMD-Vi: Added PCI DMA alias %02x.%d for %s\n",
+   pr_info("Added PCI DMA alias %02x.%d for %s\n",
PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias),
dev_name(dev));
}
@@ -507,8 +509,8 @@ static void dump_dte_entry(u16 devid)
int i;
 
for (i = 0; i < 4; ++i)
-   pr_err("AMD-Vi: DTE[%d]: %016llx\n", i,
-   amd_iommu_dev_table[devid].data[i]);
+   pr_err("DTE[%d]: %016llx\n",
+  i, amd_iommu_dev_table[devid].data[i]);
 }
 
 static void dump_command(unsigned long phys_addr)
@@ -517,7 +519,7 @@ static void dump_command(unsigned long phys_addr)
int i;
 
for (i = 0; i < 4; ++i)
-   pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
+   pr_err("CMD[%d]: %08x\n", i, cmd->data[i]);
 }
 
 static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
@@ -532,10 +534,10 @@ static void amd_iommu_report_page_fault(u16 devid, u16 
domain_id,
dev_data = get_dev_data(&pdev->dev);
 
if (dev_data && __ratelimit(&dev_data->rs)) {
-   dev_err(&pdev->dev, "AMD-Vi: Event logged [IO_PAGE_FAULT 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+   dev_err(&pdev->dev, "Event logged [IO_PAGE_FAULT domain=0x%04x 
address=0x%016llx flags=0x%04x]\n",
domain_id, address, flags);
} else if (printk_ratelimit()) {
-   pr_err("AMD-Vi: Event logged [IO_PAGE_FAULT device=%02x:%02x.%x 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+   pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
domain_id, address, flags);
}
@@ -562,7 +564,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
if (type == 0) {
/* Did we hit the erratum? */
if (++count == LOOP_TIMEOUT) {
-   pr_err("AMD-Vi: No event written to event log\n");
+   pr_err("No event written to event log\n");
return;
}
udelay(1);
@@ -572,43 +574,40 @@ static void iommu_print_event(struct amd_iommu *iommu, 
void *__evt)
if (type == EVENT_TYPE_IO_FAULT) {
amd_iommu_report_page_fault(devid, pasid, address, flags);
return;
-   } else {
-   dev_err(dev, "AMD-Vi: Event logged [");
}
 
switch (type) {
case EVENT_TYPE_ILL_DEV:
-   dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x 
pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY 
device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
PCI_BUS_

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Joe Perches 
On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
> On 07/03/2018 05:07 AM, Joe Perches wrote:
> > On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> > > Currently tag is being assigned but not used, it is missing from
> > > the dev_err message, so add it in.
> > > 
> > > Cleans up clang warning:
> > > warning: variable 'tag' set but not used [-Wunused-but-set-variable]
> > 
> > []
> > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > 
> > []
> > > @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu 
> > > *iommu, void *__evt)
> > >   pasid = ((event[0] >> 16) & 0x)
> > >   | ((event[1] << 6) & 0xF);
> > >   tag = event[1] & 0x03FF;
> > > - dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> > > pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> > > + dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> > > pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
> > >   PCI_BUS_NUM(devid), PCI_SLOT(devid), 
> > > PCI_FUNC(devid),
> > > - pasid, address, flags);
> > > + pasid, address, flags, tag);
> > 
> > Seems to have a superfluous ] that should be removed.
> 
> Yeah, I pretty much messed up all of the log messages in that function. 
> My apologies. I'll create a patch for that problem; it shouldn't be 
> fixed here.

I also wonder why event is declared volatile and then
dereferenced with [] multiple times.

Maybe each array dereference should be stored as a
local variable instead.


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

Re: [PATCH] dma-mapping: Relax warnings for per-device areas

2018-07-03 Thread Fredrik Noring 
Thank you Robin,

On Tue, Jul 03, 2018 at 02:08:30PM +0100, Robin Murphy wrote:
> The reasons why dma_free_attrs() should not be called from IRQ context
> are not necessarily obvious and somewhat buried in the development
> history, so let's start by documenting the warning itself to help anyone
> who does happen to hit it and wonder what the deal is.
> 
> However, this check turns out to be slightly over-restrictive for the
> way that per-device memory has been spliced into the general API, since
> for that case we know that dma_declare_coherent_memory() has created an
> appropriate CPU mapping for the entire area and nothing dynamic should
> be happening. Given that the usage model for per-device memory is often
> more akin to streaming DMA than 'real' coherent DMA (e.g. allocating and
> freeing space to copy short-lived packets in and out), it is also
> somewhat more reasonable for those operations to happen in IRQ handlers
> for such devices.
> 
> A somewhat similar line of reasoning also applies at the other end for
> the mask check in dma_alloc_attrs() too - indeed, a device which cannot
> access anything other than its own local memory probably *shouldn't*
> have a valid mask for the general coherent DMA API.
> 
> Therefore, let's move the per-device area hooks up ahead of the assorted
> checks, so that they get a chance to resolve the request before we get
> as far as definite "you're doing it wrong" territory.

I have tested this patch and it corrects both problems with the PS2 OHCI
driver. I believe there is a fair chance that drivers/usb/host/ohci-sm501.c
and drivers/usb/host/ohci-tmio.c are fixed as well, since they are similar.

Tested-by: Fredrik Noring 

Fredrik

> Reported-by: Fredrik Noring 
> Signed-off-by: Robin Murphy 
> ---
>  include/linux/dma-mapping.h | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index f9cc309507d9..ffeca3ab59c0 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -512,12 +512,12 @@ static inline void *dma_alloc_attrs(struct device *dev, 
> size_t size,
>   const struct dma_map_ops *ops = get_dma_ops(dev);
>   void *cpu_addr;
>  
> - BUG_ON(!ops);
> - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> -
>   if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
>   return cpu_addr;
>  
> + BUG_ON(!ops);
> + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> +
>   /* let the implementation decide on the zone to allocate from: */
>   flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
>  
> @@ -537,12 +537,19 @@ static inline void dma_free_attrs(struct device *dev, 
> size_t size,
>  {
>   const struct dma_map_ops *ops = get_dma_ops(dev);
>  
> - BUG_ON(!ops);
> - WARN_ON(irqs_disabled());
> -
>   if (dma_release_from_dev_coherent(dev, get_order(size), cpu_addr))
>   return;
>  
> + BUG_ON(!ops);
> + /*
> +  * On non-coherent platforms which implement DMA-coherent buffers via
> +  * non-cacheable remaps, ops->free() may call vunmap(). Thus arriving
> +  * here in IRQ context is a) at risk of a BUG_ON() or trying to sleep
> +  * on some machines, and b) an indication that the driver is probably
> +  * misusing the coherent API anyway.
> +  */
> + WARN_ON(irqs_disabled());
> +
>   if (!ops->free || !cpu_addr)
>   return;
>  
> -- 
> 2.17.1.dirty
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Joe Perches 
On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
> On 7/3/2018 11:24 AM, Colin Ian King wrote:
> > On 03/07/18 17:21, Hook, Gary wrote:
> > > On 7/3/2018 10:55 AM, Joe Perches wrote:
> > > > On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
> > > > > On 07/03/2018 05:07 AM, Joe Perches wrote:
> > > > > > On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> > > > > > > Currently tag is being assigned but not used, it is missing from
> > > > > > > the dev_err message, so add it in.
> > > > > > > 
> > > > > > > Cleans up clang warning:
> > > > > > > warning: variable 'tag' set but not used 
> > > > > > > [-Wunused-but-set-variable]
> > > > > > 
> > > > > > []
> > > > > > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > > > > > 
> > > > > > []
> > > > > > > @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu
> > > > > > > *iommu, void *__evt)
> > > > > > > pasid = ((event[0] >> 16) & 0x)
> > > > > > > | ((event[1] << 6) & 0xF);
> > > > > > > tag = event[1] & 0x03FF;
> > > > > > > -dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> > > > > > > pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> > > > > > > +dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> > > > > > > pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
> > > > > > > PCI_BUS_NUM(devid), PCI_SLOT(devid), 
> > > > > > > PCI_FUNC(devid),
> > > > > > > -pasid, address, flags);
> > > > > > > +pasid, address, flags, tag);
> > > > > > 
> > > > > > Seems to have a superfluous ] that should be removed.
> > > > > 
> > > > > Yeah, I pretty much messed up all of the log messages in that 
> > > > > function.
> > > > > My apologies. I'll create a patch for that problem; it shouldn't be
> > > > > fixed here.
> > > 
> > > Well, no, I misremembered. The extraneous square brace has been there
> > > forever. Needs fixin', though.
> > > 
> > 
> > The opening square bracket is much earlier:
> > 
> > dev_err(dev, "AMD-Vi: Event logged [");
> > 
> > ..and all the subsequent dev_err messages have the trailing square bracket.
> 
> Gah! Mystery solved. I'd forgotten about that.
> 
> "Never mind."
> 

It stems from the misconversion from printk to dev_err

The initial dev_err is fine but all the dev_err uses
in the switch/case should use pr_cont, not dev_err

(there's a suggested patch below this commit reference)

---
>From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
From: Gary R Hook 
Date: Thu, 8 Mar 2018 18:34:41 -0600
Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log

Remove printk and use a more preferable error logging function.

Signed-off-by: Gary R Hook 
Signed-off-by: Joerg Roedel 
---
 drivers/iommu/amd_iommu.c | 55 
---
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 997a947ddc3b..4cd19f93ca15 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 
domain_id,
 
 static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 {
+   struct device *dev = iommu->iommu.dev;
int type, devid, domid, flags;
volatile u32 *event = __evt;
int count = 0;
@@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu, 
void *__evt)
amd_iommu_report_page_fault(devid, domid, address, flags);
return;
} else {
-   printk(KERN_ERR "AMD-Vi: Event logged [");
+   dev_err(dev, "AMD-Vi: Event logged [");
}
 
switch (type) {
case EVENT_TYPE_ILL_DEV:
-   printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
-  "address=0x%016llx flags=0x%04x]\n",
-  PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-  address, flags);
+   dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
+   "address=0x%016llx flags=0x%04x]\n",
+   PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+   address, flags);

etc...

so this could be:

---
 drivers/iommu/amd_iommu.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..f78cfa094301 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -572,43 +572,42 @@ static void iommu_print_event(struct amd_iommu *iommu, 
void *__evt)
if (type == EVENT_TYPE_IO_FAULT) {
amd_iommu_report_page_fault(devid, pasid, address, flags);
return;
-   } else {
-   dev_err(dev, "AMD-Vi: Event logged [");
}
 
+   dev_err(dev, "AMD-Vi: Event logged [");
+
switch

Re: [PATCH 7/7 v5] arm64: dts: ls208xa: comply with the iommu map binding for fsl_mc

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

fsl-mc bus support the new iommu-map property. Comply to this binding
for fsl_mc bus.

Signed-off-by: Nipun Gupta 
Reviewed-by: Laurentiu Tudor 
---
  arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi 
b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
index 137ef4d..6010505 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
@@ -184,6 +184,7 @@
#address-cells = <2>;
#size-cells = <2>;
ranges;
+   dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x>;
  
  		clockgen: clocking@130 {

compatible = "fsl,ls2080a-clockgen";
@@ -357,6 +358,8 @@
reg = <0x0008 0x0c00 0 0x40>,  /* MC portal 
base */
  <0x 0x0834 0 0x4>; /* MC control 
reg */
msi-parent = <&its>;
+   iommu-map = <0 &smmu 0 0>;/* This is fixed-up by 
u-boot */
+   dma-coherent;
#address-cells = <3>;
#size-cells = <1>;
  
@@ -460,6 +463,8 @@

compatible = "arm,mmu-500";
reg = <0 0x500 0 0x80>;
#global-interrupts = <12>;
+   #iommu-cells = <1>;
+   stream-match-mask = <0x7C00>;
interrupts = <0 13 4>, /* global secure fault */
 <0 14 4>, /* combined secure interrupt */
 <0 15 4>, /* global non-secure fault */
@@ -502,7 +507,6 @@
 <0 204 4>, <0 205 4>,
 <0 206 4>, <0 207 4>,
 <0 208 4>, <0 209 4>;
-   mmu-masters = <&fsl_mc 0x300 0>;


Since we're in here, is the SMMU itself also coherent? If it is, you 
probably want to say so and avoid the overhead of pointlessly cleaning 
cache lines on every page table update.


Robin.


};
  
  		dspi: dspi@210 {



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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Hook, Gary 

On 7/3/2018 11:24 AM, Colin Ian King wrote:

On 03/07/18 17:21, Hook, Gary wrote:

On 7/3/2018 10:55 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:

On 07/03/2018 05:07 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]


[]

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c


[]

@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu
*iommu, void *__evt)
    pasid = ((event[0] >> 16) & 0x)
    | ((event[1] << 6) & 0xF);
    tag = event[1] & 0x03FF;
-    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
    PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-    pasid, address, flags);
+    pasid, address, flags, tag);


Seems to have a superfluous ] that should be removed.


Yeah, I pretty much messed up all of the log messages in that function.
My apologies. I'll create a patch for that problem; it shouldn't be
fixed here.


Well, no, I misremembered. The extraneous square brace has been there
forever. Needs fixin', though.



The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.


Gah! Mystery solved. I'd forgotten about that.

"Never mind."


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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Colin Ian King 
On 03/07/18 17:21, Hook, Gary wrote:
> On 7/3/2018 10:55 AM, Joe Perches wrote:
>> On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
>>> On 07/03/2018 05:07 AM, Joe Perches wrote:
 On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> Currently tag is being assigned but not used, it is missing from
> the dev_err message, so add it in.
>
> Cleans up clang warning:
> warning: variable 'tag' set but not used [-Wunused-but-set-variable]

 []
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c

 []
> @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu
> *iommu, void *__evt)
>    pasid = ((event[0] >> 16) & 0x)
>    | ((event[1] << 6) & 0xF);
>    tag = event[1] & 0x03FF;
> -    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> +    dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
>    PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> -    pasid, address, flags);
> +    pasid, address, flags, tag);

 Seems to have a superfluous ] that should be removed.
>>>
>>> Yeah, I pretty much messed up all of the log messages in that function.
>>> My apologies. I'll create a patch for that problem; it shouldn't be
>>> fixed here.
> 
> Well, no, I misremembered. The extraneous square brace has been there
> forever. Needs fixin', though.
> 

The opening square bracket is much earlier:

dev_err(dev, "AMD-Vi: Event logged [");

..and all the subsequent dev_err messages have the trailing square bracket.



> 
>> I also wonder why event is declared volatile and then
>> dereferenced with [] multiple times.
>>
>> Maybe each array dereference should be stored as a
>> local variable instead.
> 
> (I know you know this, but as I understand it) Event is pointing into
> the (hardware's) event buffer, and the data structure has the potential
> of changing out from under us if the device does something without our
> knowledge. Since volatile hints to the compiler of this possibility, I
> believe the compiler should manage this situation. But I could be wrong.
> 
> I don't know that we need to atomically copy all 16 bytes into a local
> buffer, as I don't think it's possible for the device to step on itself.
> It will just stop recording events if the buffer gets full. At this
> moment I think volatile is overkill, at least for the EPYC/Ryzen IOMMU.

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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Hook, Gary 

On 7/3/2018 10:55 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:

On 07/03/2018 05:07 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]


[]

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c


[]

@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
pasid = ((event[0] >> 16) & 0x)
| ((event[1] << 6) & 0xF);
tag = event[1] & 0x03FF;
-   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x tag=0x%03x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-   pasid, address, flags);
+   pasid, address, flags, tag);


Seems to have a superfluous ] that should be removed.


Yeah, I pretty much messed up all of the log messages in that function.
My apologies. I'll create a patch for that problem; it shouldn't be
fixed here.


Well, no, I misremembered. The extraneous square brace has been there 
forever. Needs fixin', though.




I also wonder why event is declared volatile and then
dereferenced with [] multiple times.

Maybe each array dereference should be stored as a
local variable instead.


(I know you know this, but as I understand it) Event is pointing into 
the (hardware's) event buffer, and the data structure has the potential 
of changing out from under us if the device does something without our 
knowledge. Since volatile hints to the compiler of this possibility, I 
believe the compiler should manage this situation. But I could be wrong.


I don't know that we need to atomically copy all 16 bytes into a local 
buffer, as I don't think it's possible for the device to step on itself. 
It will just stop recording events if the buffer gets full. At this 
moment I think volatile is overkill, at least for the EPYC/Ryzen IOMMU.

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

Re: [PATCH 6/7 v5] bus: fsl-mc: set coherent dma mask for devices on fsl-mc bus

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

of_dma_configure() API expects coherent_dma_mask to be correctly
set in the devices. This patch does the needful.


Reviewed-by: Robin Murphy 


Signed-off-by: Nipun Gupta 
---
  drivers/bus/fsl-mc/fsl-mc-bus.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index fa43c7d..624828b 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -627,6 +627,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
mc_dev->icid = parent_mc_dev->icid;
mc_dev->dma_mask = FSL_MC_DEFAULT_DMA_MASK;
mc_dev->dev.dma_mask = &mc_dev->dma_mask;
+   mc_dev->dev.coherent_dma_mask = mc_dev->dma_mask;
dev_set_msi_domain(&mc_dev->dev,
   dev_get_msi_domain(&parent_mc_dev->dev));
}


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

Re: [PATCH 5/7 v5] bus: fsl-mc: support dma configure for devices on fsl-mc bus

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

This patch adds support of dma configuration for devices on fsl-mc
bus using 'dma_configure' callback for busses. Also, directly calling
arch_setup_dma_ops is removed from the fsl-mc bus.


Looks like this is the final arch_setup_dma_ops offender, yay!


Signed-off-by: Nipun Gupta 
Reviewed-by: Laurentiu Tudor 
---
  drivers/bus/fsl-mc/fsl-mc-bus.c | 15 +++
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 5d8266c..fa43c7d 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -127,6 +127,16 @@ static int fsl_mc_bus_uevent(struct device *dev, struct 
kobj_uevent_env *env)
return 0;
  }
  
+static int fsl_mc_dma_configure(struct device *dev)

+{
+   struct device *dma_dev = dev;
+
+   while (dev_is_fsl_mc(dma_dev))
+   dma_dev = dma_dev->parent;
+
+   return of_dma_configure(dev, dma_dev->of_node, 0);
+}
+
  static ssize_t modalias_show(struct device *dev, struct device_attribute 
*attr,
 char *buf)
  {
@@ -148,6 +158,7 @@ struct bus_type fsl_mc_bus_type = {
.name = "fsl-mc",
.match = fsl_mc_bus_match,
.uevent = fsl_mc_bus_uevent,
+   .dma_configure  = fsl_mc_dma_configure,
.dev_groups = fsl_mc_dev_groups,
  };
  EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
@@ -633,10 +644,6 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
goto error_cleanup_dev;
}
  
-	/* Objects are coherent, unless 'no shareability' flag set. */

-   if (!(obj_desc->flags & FSL_MC_OBJ_FLAG_NO_MEM_SHAREABILITY))


Although it seems we do end up without any handling of this 
"non-coherent object behind coherent MC" case, and I'm not sure how 
easily that could be accommodated by generic code... :/ How important is 
the quirk?


Robin.


-   arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
-
/*
 * The device-specific probe callback will get invoked by device_add()
 */


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

Re: [PATCH 4/7 v5] iommu: arm-smmu: Add support for the fsl-mc bus

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

Implement bus specific support for the fsl-mc bus including
registering arm_smmu_ops and bus specific device add operations.

Signed-off-by: Nipun Gupta 
---
  drivers/iommu/arm-smmu.c |  7 +++
  drivers/iommu/iommu.c| 21 +
  include/linux/fsl/mc.h   |  8 
  include/linux/iommu.h|  2 ++
  4 files changed, 38 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60..e1d5090 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -52,6 +52,7 @@
  #include 
  
  #include 

+#include 
  
  #include "io-pgtable.h"

  #include "arm-smmu-regs.h"
@@ -1459,6 +1460,8 @@ static struct iommu_group *arm_smmu_device_group(struct 
device *dev)
  
  	if (dev_is_pci(dev))

group = pci_device_group(dev);
+   else if (dev_is_fsl_mc(dev))
+   group = fsl_mc_device_group(dev);
else
group = generic_device_group(dev);
  
@@ -2037,6 +2040,10 @@ static void arm_smmu_bus_init(void)

bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
}
  #endif
+#ifdef CONFIG_FSL_MC_BUS
+   if (!iommu_present(&fsl_mc_bus_type))
+   bus_set_iommu(&fsl_mc_bus_type, &arm_smmu_ops);
+#endif
  }
  
  static int arm_smmu_device_probe(struct platform_device *pdev)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d2aa2320..6d4ce35 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  
  static struct kset *iommu_group_kset;

@@ -987,6 +988,26 @@ struct iommu_group *pci_device_group(struct device *dev)
return iommu_group_alloc();
  }
  
+/* Get the IOMMU group for device on fsl-mc bus */

+struct iommu_group *fsl_mc_device_group(struct device *dev)
+{
+   struct device *cont_dev = fsl_mc_cont_dev(dev);
+   struct iommu_group *group;
+
+   /* Container device is responsible for creating the iommu group */
+   if (fsl_mc_is_cont_dev(dev)) {


Why duplicate what fsl_mc_cont_dev() has already figured out?

AFAICS the overall operation here boils down to something like:

cont_dev = fsl_mc_cont_dev(dev);
group = iommu_group_get(cont_dev);
if (!group)
group = iommu_group_alloc();
return group;


+   group = iommu_group_alloc();
+   if (IS_ERR(group))
+   return NULL;


iommu_group_get_for_dev() expects a PTR_ERR, so I don't think munging 
the return here is the right thing to do.



+   } else {
+   get_device(cont_dev);


If races are a concern, isn't this a bit late? Maybe in that case you 
want {get,put}_fsl_mc_cont_dev() routines instead of the simple macro 
below. But on the other hand if dev already has cont_dev as its parent, 
wouldn't the reference taken in device_add() be sufficient to prevent it 
from vanishing unexpectedly in this timescale?


Robin.


+   group = iommu_group_get(cont_dev);
+   put_device(cont_dev);
+   }
+
+   return group;
+}
+
  /**
   * iommu_group_get_for_dev - Find or create the IOMMU group for a device
   * @dev: target device
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index f27cb14..dddaca1 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -351,6 +351,14 @@ struct fsl_mc_io {
  #define dev_is_fsl_mc(_dev) (0)
  #endif
  
+/* Macro to check if a device is a container device */

+#define fsl_mc_is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & \
+   FSL_MC_IS_DPRC)
+
+/* Macro to get the container device of a MC device */
+#define fsl_mc_cont_dev(_dev) (fsl_mc_is_cont_dev(_dev) ? \
+   (_dev) : (_dev)->parent)
+
  /*
   * module_fsl_mc_driver() - Helper macro for drivers that don't do
   * anything special in module init/exit.  This eliminates a lot of
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19938ee..2981200 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -389,6 +389,8 @@ static inline size_t iommu_map_sg(struct iommu_domain 
*domain,
  extern struct iommu_group *pci_device_group(struct device *dev);
  /* Generic device grouping function */
  extern struct iommu_group *generic_device_group(struct device *dev);
+/* FSL-MC device grouping function */
+struct iommu_group *fsl_mc_device_group(struct device *dev);
  
  /**

   * struct iommu_fwspec - per-device IOMMU instance data


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

Re: [PATCH 3/7 v5] iommu: support iommu configuration for fsl-mc devices

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

With of_pci_map_rid available for all the busses, use the function
for configuration of devices on fsl-mc bus


FWIW I had a quick hack at factoring out the commonality with 
of_pci_iommu_init(), at which point I reckon this change is easier to 
follow as-is for the moment, so:


Reviewed-by: Robin Murphy 


Signed-off-by: Nipun Gupta 
---
  drivers/iommu/of_iommu.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 811e160..284474d 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -24,6 +24,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #define NO_IOMMU	1
  
@@ -159,6 +160,23 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)

return err;
  }
  
+static int of_fsl_mc_iommu_init(struct fsl_mc_device *mc_dev,

+   struct device_node *master_np)
+{
+   struct of_phandle_args iommu_spec = { .args_count = 1 };
+   int err;
+
+   err = of_map_rid(master_np, mc_dev->icid, "iommu-map",
+"iommu-map-mask", &iommu_spec.np,
+iommu_spec.args);
+   if (err)
+   return err == -ENODEV ? NO_IOMMU : err;
+
+   err = of_iommu_xlate(&mc_dev->dev, &iommu_spec);
+   of_node_put(iommu_spec.np);
+   return err;
+}
+
  const struct iommu_ops *of_iommu_configure(struct device *dev,
   struct device_node *master_np)
  {
@@ -190,6 +208,8 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
  
  		err = pci_for_each_dma_alias(to_pci_dev(dev),

 of_pci_iommu_init, &info);
+   } else if (dev_is_fsl_mc(dev)) {
+   err = of_fsl_mc_iommu_init(to_fsl_mc_device(dev), master_np);
} else {
struct of_phandle_args iommu_spec;
int idx = 0;


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

Re: [PATCH 2/7 v5] iommu: of: make of_pci_map_rid() available for other devices too

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

iommu-map property is also used by devices with fsl-mc. This
patch moves the of_pci_map_rid to generic location, so that it
can be used by other busses too.

'of_pci_map_rid' is renamed here to 'of_map_rid' and there is no
functional change done in the API.


Reviewed-by: Robin Murphy 

Signed-off-by: Nipun Gupta 
Reviewed-by: Rob Herring 
Acked-by: Bjorn Helgaas 
---
  drivers/iommu/of_iommu.c |   5 +--
  drivers/of/base.c| 102 +++
  drivers/of/irq.c |   5 +--
  drivers/pci/of.c | 101 --
  include/linux/of.h   |  11 +
  include/linux/of_pci.h   |  10 -
  6 files changed, 117 insertions(+), 117 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5c36a8b..811e160 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -149,9 +149,8 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 
alias, void *data)
struct of_phandle_args iommu_spec = { .args_count = 1 };
int err;
  
-	err = of_pci_map_rid(info->np, alias, "iommu-map",

-"iommu-map-mask", &iommu_spec.np,
-iommu_spec.args);
+   err = of_map_rid(info->np, alias, "iommu-map", "iommu-map-mask",
+&iommu_spec.np, iommu_spec.args);
if (err)
return err == -ENODEV ? NO_IOMMU : err;
  
diff --git a/drivers/of/base.c b/drivers/of/base.c

index 848f549..c7aac81 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1995,3 +1995,105 @@ int of_find_last_cache_level(unsigned int cpu)
  
  	return cache_level;

  }
+
+/**
+ * of_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @rid: device requester ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a device requester ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int of_map_rid(struct device_node *np, u32 rid,
+  const char *map_name, const char *map_mask_name,
+  struct device_node **target, u32 *id_out)
+{
+   u32 map_mask, masked_rid;
+   int map_len;
+   const __be32 *map = NULL;
+
+   if (!np || !map_name || (!target && !id_out))
+   return -EINVAL;
+
+   map = of_get_property(np, map_name, &map_len);
+   if (!map) {
+   if (target)
+   return -ENODEV;
+   /* Otherwise, no map implies no translation */
+   *id_out = rid;
+   return 0;
+   }
+
+   if (!map_len || map_len % (4 * sizeof(*map))) {
+   pr_err("%pOF: Error: Bad %s length: %d\n", np,
+   map_name, map_len);
+   return -EINVAL;
+   }
+
+   /* The default is to select all bits. */
+   map_mask = 0x;
+
+   /*
+* Can be overridden by "{iommu,msi}-map-mask" property.
+* If of_property_read_u32() fails, the default is used.
+*/
+   if (map_mask_name)
+   of_property_read_u32(np, map_mask_name, &map_mask);
+
+   masked_rid = map_mask & rid;
+   for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
+   struct device_node *phandle_node;
+   u32 rid_base = be32_to_cpup(map + 0);
+   u32 phandle = be32_to_cpup(map + 1);
+   u32 out_base = be32_to_cpup(map + 2);
+   u32 rid_len = be32_to_cpup(map + 3);
+
+   if (rid_base & ~map_mask) {
+   pr_err("%pOF: Invalid %s translation - %s-mask (0x%x) 
ignores rid-base (0x%x)\n",
+   np, map_name, map_name,
+   map_mask, rid_base);
+   return -EFAULT;
+   }
+
+   if (masked_rid < rid_base || masked_rid >= rid_base + rid_len)
+   continue;
+
+   phandle_node = of_find_node_by_phandle(phandle);
+   if (!phandle_node)
+   return -ENODEV;
+
+   if (target) {
+   if (*target)
+   of_node_put(phandle_node);
+   else
+

Re: [PATCH 1/7 v5] Docs: dt: add fsl-mc iommu-map device-tree binding

2018-07-03 Thread Robin Murphy 

On 20/05/18 14:49, Nipun Gupta wrote:

The existing IOMMU bindings cannot be used to specify the relationship
between fsl-mc devices and IOMMUs. This patch adds a generic binding for
mapping fsl-mc devices to IOMMUs, using iommu-map property.

Signed-off-by: Nipun Gupta 
Reviewed-by: Rob Herring 
---
  .../devicetree/bindings/misc/fsl,qoriq-mc.txt  | 39 ++
  1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt 
b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
index 6611a7c..8cbed4f 100644
--- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
+++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
@@ -9,6 +9,25 @@ blocks that can be used to create functional hardware 
objects/devices
  such as network interfaces, crypto accelerator instances, L2 switches,
  etc.
  
+For an overview of the DPAA2 architecture and fsl-mc bus see:

+drivers/staging/fsl-mc/README.txt


Nit: Looks like that's Documentation/networking/dpaa2/overview.rst now.


+
+As described in the above overview, all DPAA2 objects in a DPRC share the
+same hardware "isolation context" and a 10-bit value called an ICID
+(isolation context id) is expressed by the hardware to identify
+the requester.
+
+The generic 'iommus' property is insufficient to describe the relationship
+between ICIDs and IOMMUs, so an iommu-map property is used to define
+the set of possible ICIDs under a root DPRC and how they map to
+an IOMMU.
+
+For generic IOMMU bindings, see
+Documentation/devicetree/bindings/iommu/iommu.txt.
+
+For arm-smmu binding, see:
+Documentation/devicetree/bindings/iommu/arm,smmu.txt.
+
  Required properties:
  
  - compatible

@@ -88,14 +107,34 @@ Sub-nodes:
Value type: 
Definition: Specifies the phandle to the PHY device node 
associated
with the this dpmac.
+Optional properties:
+
+- iommu-map: Maps an ICID to an IOMMU and associated iommu-specifier
+  data.
+
+  The property is an arbitrary number of tuples of
+  (icid-base,iommu,iommu-base,length).
+
+  Any ICID i in the interval [icid-base, icid-base + length) is
+  associated with the listed IOMMU, with the iommu-specifier
+  (i - icid-base + iommu-base).
  
  Example:
  
+smmu: iommu@500 {

+   compatible = "arm,mmu-500";
+   #iommu-cells = <2>;


This should be 1 if stream-match-mask is present. Bad example is bad :)

Robin.


+   stream-match-mask = <0x7C00>;
+   ...
+};
+
  fsl_mc: fsl-mc@80c00 {
  compatible = "fsl,qoriq-mc";
  reg = <0x0008 0x0c00 0 0x40>,/* MC portal base */
<0x 0x0834 0 0x4>; /* MC control reg */
  msi-parent = <&its>;
+/* define map for ICIDs 23-64 */
+iommu-map = <23 &smmu 23 41>;
  #address-cells = <3>;
  #size-cells = <1>;
  


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

[PATCH] dma-mapping: Relax warnings for per-device areas

2018-07-03 Thread Robin Murphy 
The reasons why dma_free_attrs() should not be called from IRQ context
are not necessarily obvious and somewhat buried in the development
history, so let's start by documenting the warning itself to help anyone
who does happen to hit it and wonder what the deal is.

However, this check turns out to be slightly over-restrictive for the
way that per-device memory has been spliced into the general API, since
for that case we know that dma_declare_coherent_memory() has created an
appropriate CPU mapping for the entire area and nothing dynamic should
be happening. Given that the usage model for per-device memory is often
more akin to streaming DMA than 'real' coherent DMA (e.g. allocating and
freeing space to copy short-lived packets in and out), it is also
somewhat more reasonable for those operations to happen in IRQ handlers
for such devices.

A somewhat similar line of reasoning also applies at the other end for
the mask check in dma_alloc_attrs() too - indeed, a device which cannot
access anything other than its own local memory probably *shouldn't*
have a valid mask for the general coherent DMA API.

Therefore, let's move the per-device area hooks up ahead of the assorted
checks, so that they get a chance to resolve the request before we get
as far as definite "you're doing it wrong" territory.

Reported-by: Fredrik Noring 
Signed-off-by: Robin Murphy 
---
 include/linux/dma-mapping.h | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f9cc309507d9..ffeca3ab59c0 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -512,12 +512,12 @@ static inline void *dma_alloc_attrs(struct device *dev, 
size_t size,
const struct dma_map_ops *ops = get_dma_ops(dev);
void *cpu_addr;
 
-   BUG_ON(!ops);
-   WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
-
if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
return cpu_addr;
 
+   BUG_ON(!ops);
+   WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
+
/* let the implementation decide on the zone to allocate from: */
flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
 
@@ -537,12 +537,19 @@ static inline void dma_free_attrs(struct device *dev, 
size_t size,
 {
const struct dma_map_ops *ops = get_dma_ops(dev);
 
-   BUG_ON(!ops);
-   WARN_ON(irqs_disabled());
-
if (dma_release_from_dev_coherent(dev, get_order(size), cpu_addr))
return;
 
+   BUG_ON(!ops);
+   /*
+* On non-coherent platforms which implement DMA-coherent buffers via
+* non-cacheable remaps, ops->free() may call vunmap(). Thus arriving
+* here in IRQ context is a) at risk of a BUG_ON() or trying to sleep
+* on some machines, and b) an indication that the driver is probably
+* misusing the coherent API anyway.
+*/
+   WARN_ON(irqs_disabled());
+
if (!ops->free || !cpu_addr)
return;
 
-- 
2.17.1.dirty

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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Gary R Hook 

On 07/03/2018 05:07 AM, Joe Perches wrote:

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]

[]

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c

[]

@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
pasid = ((event[0] >> 16) & 0x)
| ((event[1] << 6) & 0xF);
tag = event[1] & 0x03FF;
-   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x tag=0x%03x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-   pasid, address, flags);
+   pasid, address, flags, tag);


Seems to have a superfluous ] that should be removed.


Yeah, I pretty much messed up all of the log messages in that function. 
My apologies. I'll create a patch for that problem; it shouldn't be 
fixed here.


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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Gary R Hook 

On 07/03/2018 01:40 AM, Colin King wrote:

From: Colin Ian King 

Currently tag is being assigned but not used, it is missing from
the dev_err message, so add it in.

Cleans up clang warning:
warning: variable 'tag' set but not used [-Wunused-but-set-variable]

Fixes: e7f63ffc1bf1 ("iommu/amd: Update logging information for new event type")
Signed-off-by: Colin Ian King 

---
This is a re-working of an earlier incorrect fix that I posted
yesterday ("iommu/amd: remove redundant variable tag")


Acked-by: Gary R Hook 



---
  drivers/iommu/amd_iommu.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..6adab2690793 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
pasid = ((event[0] >> 16) & 0x)
| ((event[1] << 6) & 0xF);
tag = event[1] & 0x03FF;
-   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x pasid=0x%05x 
address=0x%016llx flags=0x%04x tag=0x%03x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-   pasid, address, flags);
+   pasid, address, flags, tag);
break;
default:
dev_err(dev, "UNKNOWN event[0]=0x%08x event[1]=0x%08x 
event[2]=0x%08x event[3]=0x%08x\n",



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

Re: [RFC PATCH] iommu/vt-d: Exclude known RMRRs from reserved ranges

2018-07-03 Thread David Woodhouse 
On Thu, 2018-06-07 at 09:01 -0600, Alex Williamson wrote:
> 
> > intel_iommu_get_resv_regions is used not just for IOMMU API. I'm
> > afraid doing so will make RMRR completely ignored, even in normal
> > DMA API path...
> 
> Well, I'm a bit stuck then, we have existing IOMMU API users that
> ignore these ranges and in fact conflict with these ranges blocking us
> from restricting mappings within these ranges.  My impression is that
> IOMMU reserved ranges should only be ranges which have some fundamental
> limitation in the IOMMU, not simply mappings for which firmware has
> requested an identity mapped range.  The latter should simply be a
> pre-allocation of the IOVA space, for the cases where we choose to
> honor the RMRR.  Thanks,

Unfortunately, Intel likes to encourage vendors to do batshit insane
things in firmware, at runtime, using DMA.

Perhaps the way to handle this is for the device drivers to cancel any
RMRR for their devices, as they take it over. So USB and graphics
drivers would clear the corresponding RMRRs as they take ownership of
the device... but anything without a driver, or things like the SCSI
host controllers which know that there might be some HP insanity going
on in SMM in parallel with what they're doing, would not (and hence
would not be assignable, etc.).



smime.p7s
Description: S/MIME cryptographic signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

2018-07-03 Thread lijiang 
在 2018年07月03日 19:14, Borislav Petkov 写道:
> On Tue, Jul 03, 2018 at 06:58:14PM +0800, lijiang wrote:
>> For kdump, the elf header finally use the crash kernel reserved memory, it 
>> is not an old memory.
> 
> Lamme repeat my suggestion:
> 
> So beef up the logic in __ioremap_caller() to figure out based on the
> address whether to access the memory encrypted or not. In general, you
> can deduce, based on the region you're mapping, whether you need to map
> in encrypted or decrypted.
> 
> For example:
> 
> addr = elfcorehdr_addr;
> 
> /* Read Elf header */
> rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
> if (rc < 0)
> return rc;
> 
> elfcorehdr_addr has that elfcorehdr address. So you can check which address
> you're mapping and do:
> 
> __ioremap_caller:
> 
> ...
>   prot = __ioremap_compute_prot(...);
> 
> and that __ioremap_compute_prot() function which you will add will have
> all that logic to determine encrypted or not by comparing addresses etc.
> 
> Does that make more sense?
> 
Thank you, Boris. Good idea, I will rethink about this issue and post it again.

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

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

2018-07-03 Thread lijiang 
在 2018年07月03日 17:39, Borislav Petkov 写道:
> On Tue, Jul 03, 2018 at 10:17:19AM +0800, lijiang wrote:
>> for example, the elfcorehdr. In fact, the elfcorehdr and notes
> 
> You mean this?
> 
>  ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
>  {
> -   return read_from_oldmem(buf, count, ppos, 0);
> +   return read_from_oldmem(buf, count, ppos, 0, sme_active());
> 
> That looks encrypted to me.
> 
The elf notes is an old memory, it is encrypted.

But the elf header is a crash kernel reserved memory, which is unencrypted.
 ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
 {
-   return read_from_oldmem(buf, count, ppos, 0);
+   return read_from_oldmem(buf, count, ppos, 0, false);
 }

They call the same function(read_from_oldmem->ioremap_cache), so it is hard to
properly remap the memory if we don't use the parameter to distinguish. 

Regards,
Lianbo
>> call the same function(read_from_oldmem->ioremap_cache), in this case,
>> it is very difficult to properly remap the memory if the caller don't
>> care whether the memory is encrypted.
> 
> So beef up the logic in __ioremap_caller() to figure out based on the
> address whether to access the memory encrypted or not. You can find out
> the elfcorehdr address in the capture kernel.
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

2018-07-03 Thread Borislav Petkov 
On Tue, Jul 03, 2018 at 06:58:14PM +0800, lijiang wrote:
> For kdump, the elf header finally use the crash kernel reserved memory, it is 
> not an old memory.

Lamme repeat my suggestion:

So beef up the logic in __ioremap_caller() to figure out based on the
address whether to access the memory encrypted or not. In general, you
can deduce, based on the region you're mapping, whether you need to map
in encrypted or decrypted.

For example:

addr = elfcorehdr_addr;

/* Read Elf header */
rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
if (rc < 0)
return rc;

elfcorehdr_addr has that elfcorehdr address. So you can check which address
you're mapping and do:

__ioremap_caller:

...
prot = __ioremap_compute_prot(...);

and that __ioremap_compute_prot() function which you will add will have
all that logic to determine encrypted or not by comparing addresses etc.

Does that make more sense?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

2018-07-03 Thread lijiang 
在 2018年07月03日 10:17, lijiang 写道:
> 在 2018年07月02日 18:14, Borislav Petkov 写道:
>> On Mon, Jul 02, 2018 at 03:26:35PM +0800, Lianbo Jiang wrote:
>>> @@ -131,7 +132,8 @@ static void __ioremap_check_mem(resource_size_t addr, 
>>> unsigned long size,
>>>   * caller shouldn't need to know that small detail.
>>>   */
>>>  static void __iomem *__ioremap_caller(resource_size_t phys_addr,
>>> -   unsigned long size, enum page_cache_mode pcm, void *caller)
>>> +   unsigned long size, enum page_cache_mode pcm,
>>> +   void *caller, bool encrypted)
>>
>> So instead of sprinkling that @encrypted argument everywhere and then
>> setting it based on sme_active() ...
>>
>>>  {
>>> unsigned long offset, vaddr;
>>> resource_size_t last_addr;
>>> @@ -199,7 +201,7 @@ static void __iomem *__ioremap_caller(resource_size_t 
>>> phys_addr,
>>>  * resulting mapping.
>>>  */
>>> prot = PAGE_KERNEL_IO;
>>> -   if (sev_active() && mem_flags.desc_other)
>>> +   if ((sev_active() && mem_flags.desc_other) || encrypted)
>>
>> ... why can't you simply do your checks:
>>
>>  sme_active() && is_kdump_kernel()
>>
>> here so that __ioremap_caller() can automatically choose the proper
>> pgprot value when ioremapping the memory in the kdump kernel?
>>
>> And this way the callers don't even have to care whether the memory is
>> encrypted or not?
>>
> Thank you, Boris. I'm very glad to read your comments. That's a good idea, 
> but it has some
> unencrypted memory in kdump mode, for example, the elfcorehdr. In fact, the 
> elfcorehdr and
> notes call the same function(read_from_oldmem->ioremap_cache), in this case, 
> it is very
> difficult to properly remap the memory if the caller don't care whether the 
> memory is encrypted.
> 
Hi, Boris,
About this, maybe I should describe the more details.
For kdump, the elf header finally use the crash kernel reserved memory, it is 
not an old memory.
When we load the crash kernel image, kexec tools will copy the elf header from 
encrypted memory
region to unencrypted memory region, we know that a page of memory that is 
marked encrypted will
be automatically decrypted when read from DRAM if SME is enabled. This 
operation make us get the
plaintext, that is to say, it becomes unencrypted data, so we need to remap the 
elfcorehdr in un-
encrypted manners in kdump kernel, but elf notes use an old memory, it is 
encrypted. That is the
real reason why we need to use the different 
functions(ioremap_cache/ioremap_encrypted).
If the elf core header is set to be encrypted, we could need to know which part 
of memory is
allocated for the elfcorehdr in kernel space and change the page attribute, 
maybe which will have
to modify another common codes and kexec-tools.

If you agree with my explanation, i will add them to patch log and also post it 
again.
Welcome to review my patches again.

Thanks.
Lianbo

> Regards,
> Lianbo>>> prot = pgprot_encrypted(prot);
>>>  
>>> switch (pcm) {
>>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH] iommu/vt-d: Exclude known RMRRs from reserved ranges

2018-07-03 Thread Shameerali Kolothum Thodi 
Hi Alex,

> -Original Message-
> From: Tian, Kevin [mailto:kevin.t...@intel.com]
> Sent: 08 June 2018 03:56
> To: Alex Williamson 
> Cc: dw...@infradead.org; iommu@lists.linux-foundation.org;
> k...@vger.kernel.org; linux-ker...@vger.kernel.org; Shameerali Kolothum
> Thodi 
> Subject: RE: [RFC PATCH] iommu/vt-d: Exclude known RMRRs from reserved
> ranges
> 
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Thursday, June 7, 2018 11:02 PM
> >
> > On Wed, 6 Jun 2018 05:29:58 +
> > "Tian, Kevin"  wrote:
> >
> > > > From: Alex Williamson
> > > > Sent: Wednesday, June 6, 2018 3:07 AM
> > > >
> > > > device_is_rmrr_locked() allows graphics and USB devices to participate
> > > > in the IOMMU API despite, and ignoring their RMRR association,
> > however
> > > > intel_iommu_get_resv_regions() still includes the RMRRs as unavailable
> > > > IOVA space for the device.  Are we ignoring the RMRR for these devices
> > > > or are we not?  If vfio starts consuming reserved regions, perhaps we
> > > > no longer need to consider devices with RMRRs excluded from the
> > IOMMU
> > > > API interface, but we have a transitional problem that these allowed
> > > > devices still impose incompatible IOVA restrictions per the reserved
> > > > region reporting.  Dive further down the rabbit hole by also ignoring
> > > > RMRRs for "known" devices in the reserved region reporting.
> > >
> > > intel_iommu_get_resv_regions is used not just for IOMMU API. I'm
> > > afraid doing so will make RMRR completely ignored, even in normal
> > > DMA API path...
> >
> > Well, I'm a bit stuck then, we have existing IOMMU API users that
> > ignore these ranges and in fact conflict with these ranges blocking us
> > from restricting mappings within these ranges.  My impression is that
> > IOMMU reserved ranges should only be ranges which have some
> > fundamental
> > limitation in the IOMMU, not simply mappings for which firmware has
> > requested an identity mapped range.  The latter should simply be a
> > pre-allocation of the IOVA space, for the cases where we choose to
> > honor the RMRR.  Thanks,
> >
> 
> Then possibly need introduce a different interface for pre-allocation
> scenario, if above definition of reserved ranges is agreed. Currently
> two categories are both called reserved resources, e.g. IOMMU_RESV
> _DIRECT for rmrr and IOMMU_RESV_MSI for MSI...

Sorry, but I just thought of checking on the future plans/directions for solving
this issue(Unfortunately vfio iova list series depends on this).

Please let me know if there is any plans for a v2 soon.

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

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

2018-07-03 Thread Joe Perches 
On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> Currently tag is being assigned but not used, it is missing from
> the dev_err message, so add it in.
> 
> Cleans up clang warning:
> warning: variable 'tag' set but not used [-Wunused-but-set-variable]
[]
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
[]
> @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, 
> void *__evt)
>   pasid = ((event[0] >> 16) & 0x)
>   | ((event[1] << 6) & 0xF);
>   tag = event[1] & 0x03FF;
> - dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> + dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
>   PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> - pasid, address, flags);
> + pasid, address, flags, tag);

Seems to have a superfluous ] that should be removed.

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

Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

2018-07-03 Thread Borislav Petkov 
On Tue, Jul 03, 2018 at 10:17:19AM +0800, lijiang wrote:
> for example, the elfcorehdr. In fact, the elfcorehdr and notes

You mean this?

 ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
 {
-   return read_from_oldmem(buf, count, ppos, 0);
+   return read_from_oldmem(buf, count, ppos, 0, sme_active());

That looks encrypted to me.

> call the same function(read_from_oldmem->ioremap_cache), in this case,
> it is very difficult to properly remap the memory if the caller don't
> care whether the memory is encrypted.

So beef up the logic in __ioremap_caller() to figure out based on the
address whether to access the memory encrypted or not. You can find out
the elfcorehdr address in the capture kernel.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu