diff mbox series

[RESEND,1/2] : Fix DMAR Error NO_PASID when IOMMU is enabled

Message ID CALYqZ9myK4rD6gds3j2WeuFq52i6_wghnZ9BVQAaEcVvZ6RxZA@mail.gmail.com (mailing list archive)
State New
Headers show
Series [RESEND,1/2] : Fix DMAR Error NO_PASID when IOMMU is enabled | expand

Commit Message

Eric Debief May 22, 2024, 12:12 p.m. UTC
Hi,

We had a "DMAR Error  NO PASID" error reported in the kernel's log
when the IOMMU was enabled.

This is due to the missing WriteBack area for the C2H stream.
Below my patch.
One point : I didn't compile it within the latest kernel's sources'
tree as it is an extract of our backport of the XDMA support.
Feel free to contact me on any issue with this.

Hope this helps,
Eric.

Below my patch (corrected).

From b8d71851e6a146dcb448b01a671f455afb09ae90 Mon Sep 17 00:00:00 2001
From: Eric DEBIEF <debief@digigram.com>
Date: Wed, 22 May 2024 12:33:06 +0200
Subject: FIX: DMAR Error with IO_MMU enabled.

C2H write-back area was not allocated and set.
This leads to the DMAR Error.

Add the Writeback structure, allocate and set it as
the descriptors's Src field.
Done for all preps functions.

Signed-off-by: Eric DEBIEF <debief@digigram.com>
---
 drivers/dma/xilinx/xdma.c | 44 +++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

     }
@@ -705,7 +723,8 @@ xdma_prep_dma_cyclic(struct dma_chan *chan,
dma_addr_t address,
         src = &addr;
         dst = &dev_addr;
     } else {
-        dev_addr = xdma_chan->cfg.src_addr;
+        dev_addr = xdma_chan->cfg.src_addr ?
+            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
         src = &dev_addr;
         dst = &addr;
     }
@@ -803,6 +822,9 @@ static void xdma_free_chan_resources(struct dma_chan *chan)
     struct xdma_chan *xdma_chan = to_xdma_chan(chan);

     vchan_free_chan_resources(&xdma_chan->vchan);
+    dma_pool_free(xdma_chan->desc_pool,
+                xdma_chan->write_back,
+               xdma_chan->write_back->dma_addr);
     dma_pool_destroy(xdma_chan->desc_pool);
     xdma_chan->desc_pool = NULL;
 }
@@ -816,6 +838,7 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
     struct xdma_chan *xdma_chan = to_xdma_chan(chan);
     struct xdma_device *xdev = xdma_chan->xdev_hdl;
     struct device *dev = xdev->dma_dev.dev;
+    dma_addr_t write_back_addr;

     while (dev && !dev_is_pci(dev))
         dev = dev->parent;
@@ -824,13 +847,26 @@ static int xdma_alloc_chan_resources(struct
dma_chan *chan)
         return -EINVAL;
     }

-    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
XDMA_DESC_BLOCK_SIZE,
-                           XDMA_DESC_BLOCK_ALIGN, XDMA_DESC_BLOCK_BOUNDARY);
+    //Allocate the pool WITH the H2C write back
+    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
+                            dev,
+                            XDMA_DESC_BLOCK_SIZE +
+                                sizeof(struct xdma_c2h_write_back),
+                            XDMA_DESC_BLOCK_ALIGN,
+                            XDMA_DESC_BLOCK_BOUNDARY);
     if (!xdma_chan->desc_pool) {
         xdma_err(xdev, "unable to allocate descriptor pool");
         return -ENOMEM;
     }

+    /* Allocate the C2H write back out of the pool*/
+    xdma_chan->write_back = dma_pool_alloc(xdma_chan->desc_pool,
GFP_NOWAIT, &write_back_addr);
+    if (!xdma_chan->write_back) {
+        xdma_err(xdev, "unable to allocate C2H write back block");
+        return -ENOMEM;
+    }
+    xdma_chan->write_back->dma_addr = write_back_addr;
+
     return 0;
 }

--
2.34.1

Comments

Lizhi Hou May 22, 2024, 4:59 p.m. UTC | #1
Hi Eric,


Current xdma driver supports only Memory Mapped (writeback polling is 
off) channel. And it is tested with iommu on. With this configuration, 
the hardware should not generate any writeback.

So, what you are working on is  something like "add stream channel support".

To support stream channel, I would suggest to add "bool stream_mode" in 
struct xdma_chan and set it based on identifer register in 
xdma_alloc_channels().

The C2H writeback allocations and other stream specific operation should 
be based on stream_mode flag.

At last, the wb buf should be per descriptor instead of per channel. And 
the interrupt handler routine should look at the length and EOP in wb 
buf. With stream mode, the C2H desc may partially filled up due to an 
end of packet.


Thanks,

Lizhi

On 5/22/24 05:12, Eric Debief wrote:
> Hi,
>
> We had a "DMAR Error  NO PASID" error reported in the kernel's log
> when the IOMMU was enabled.
>
> This is due to the missing WriteBack area for the C2H stream.
> Below my patch.
> One point : I didn't compile it within the latest kernel's sources'
> tree as it is an extract of our backport of the XDMA support.
> Feel free to contact me on any issue with this.
>
> Hope this helps,
> Eric.
>
> Below my patch (corrected).
>
>  From b8d71851e6a146dcb448b01a671f455afb09ae90 Mon Sep 17 00:00:00 2001
> From: Eric DEBIEF <debief@digigram.com>
> Date: Wed, 22 May 2024 12:33:06 +0200
> Subject: FIX: DMAR Error with IO_MMU enabled.
>
> C2H write-back area was not allocated and set.
> This leads to the DMAR Error.
>
> Add the Writeback structure, allocate and set it as
> the descriptors's Src field.
> Done for all preps functions.
>
> Signed-off-by: Eric DEBIEF <debief@digigram.com>
> ---
>   drivers/dma/xilinx/xdma.c | 44 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index 74d4a953b50f..9ae615165cb6 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -51,6 +51,20 @@ struct xdma_desc_block {
>       dma_addr_t    dma_addr;
>   };
>
> +/**
> + * struct xdma_c2h_write_back  - Write back block , written by the XDMA.
> + * @magic_status_bit : magic (0x52B4) once written
> + * @length: effective transfer length (in bytes)
> + * @PADDING to be aligned on 32 bytes
> + * @associated dma address
> + */
> +struct xdma_c2h_write_back {
> +    __le32 magic_status_bit;
> +    __le32 length;
> +    u32 padding_1[6];
> +    dma_addr_t dma_addr;
> +};
> +
>   /**
>    * struct xdma_chan - Driver specific DMA channel structure
>    * @vchan: Virtual channel
> @@ -61,6 +75,8 @@ struct xdma_desc_block {
>    * @dir: Transferring direction of the channel
>    * @cfg: Transferring config of the channel
>    * @irq: IRQ assigned to the channel
> + * @write_back : C2H meta data write back
> +
>    */
>   struct xdma_chan {
>       struct virt_dma_chan        vchan;
> @@ -73,6 +89,7 @@ struct xdma_chan {
>       u32                irq;
>       struct completion        last_interrupt;
>       bool                stop_requested;
> +    struct xdma_c2h_write_back *write_back;
>   };
>
>   /**
> @@ -628,7 +645,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct
> scatterlist *sgl,
>           src = &addr;
>           dst = &dev_addr;
>       } else {
> -        dev_addr = xdma_chan->cfg.src_addr;
> +        dev_addr = xdma_chan->cfg.src_addr ?
> +            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
>           src = &dev_addr;
>           dst = &addr;
>       }
> @@ -705,7 +723,8 @@ xdma_prep_dma_cyclic(struct dma_chan *chan,
> dma_addr_t address,
>           src = &addr;
>           dst = &dev_addr;
>       } else {
> -        dev_addr = xdma_chan->cfg.src_addr;
> +        dev_addr = xdma_chan->cfg.src_addr ?
> +            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
>           src = &dev_addr;
>           dst = &addr;
>       }
> @@ -803,6 +822,9 @@ static void xdma_free_chan_resources(struct dma_chan *chan)
>       struct xdma_chan *xdma_chan = to_xdma_chan(chan);
>
>       vchan_free_chan_resources(&xdma_chan->vchan);
> +    dma_pool_free(xdma_chan->desc_pool,
> +                xdma_chan->write_back,
> +               xdma_chan->write_back->dma_addr);
>       dma_pool_destroy(xdma_chan->desc_pool);
>       xdma_chan->desc_pool = NULL;
>   }
> @@ -816,6 +838,7 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
>       struct xdma_chan *xdma_chan = to_xdma_chan(chan);
>       struct xdma_device *xdev = xdma_chan->xdev_hdl;
>       struct device *dev = xdev->dma_dev.dev;
> +    dma_addr_t write_back_addr;
>
>       while (dev && !dev_is_pci(dev))
>           dev = dev->parent;
> @@ -824,13 +847,26 @@ static int xdma_alloc_chan_resources(struct
> dma_chan *chan)
>           return -EINVAL;
>       }
>
> -    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
> XDMA_DESC_BLOCK_SIZE,
> -                           XDMA_DESC_BLOCK_ALIGN, XDMA_DESC_BLOCK_BOUNDARY);
> +    //Allocate the pool WITH the H2C write back
> +    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
> +                            dev,
> +                            XDMA_DESC_BLOCK_SIZE +
> +                                sizeof(struct xdma_c2h_write_back),
> +                            XDMA_DESC_BLOCK_ALIGN,
> +                            XDMA_DESC_BLOCK_BOUNDARY);
>       if (!xdma_chan->desc_pool) {
>           xdma_err(xdev, "unable to allocate descriptor pool");
>           return -ENOMEM;
>       }
>
> +    /* Allocate the C2H write back out of the pool*/
> +    xdma_chan->write_back = dma_pool_alloc(xdma_chan->desc_pool,
> GFP_NOWAIT, &write_back_addr);
> +    if (!xdma_chan->write_back) {
> +        xdma_err(xdev, "unable to allocate C2H write back block");
> +        return -ENOMEM;
> +    }
> +    xdma_chan->write_back->dma_addr = write_back_addr;
> +
>       return 0;
>   }
>
> --
> 2.34.1
Eric Debief May 23, 2024, 8:50 a.m. UTC | #2
Hi Lizhi,

Thanks for your reply.

Stream mode : I was told it was supported since the 6.8 or even since
the 6.7 kernel. I'll check my sources.
I now understand why this was not done.
On our side, we use the XDMA in stream mode as you suppose. At the
moment, it works fine with the patched genuine xdma driver.
I totally agree with your suggestion.
Let me correct this. It will take me a few weeks. Tell me if it is ok for you.

Best regards,
Eric.

Le mer. 22 mai 2024 à 18:59, Lizhi Hou <lizhi.hou@amd.com> a écrit :
>
> Hi Eric,
>
>
> Current xdma driver supports only Memory Mapped (writeback polling is
> off) channel. And it is tested with iommu on. With this configuration,
> the hardware should not generate any writeback.
>
> So, what you are working on is  something like "add stream channel support".
>
> To support stream channel, I would suggest to add "bool stream_mode" in
> struct xdma_chan and set it based on identifer register in
> xdma_alloc_channels().
>
> The C2H writeback allocations and other stream specific operation should
> be based on stream_mode flag.
>
> At last, the wb buf should be per descriptor instead of per channel. And
> the interrupt handler routine should look at the length and EOP in wb
> buf. With stream mode, the C2H desc may partially filled up due to an
> end of packet.
>
>
> Thanks,
>
> Lizhi
>
> On 5/22/24 05:12, Eric Debief wrote:
> > Hi,
> >
> > We had a "DMAR Error  NO PASID" error reported in the kernel's log
> > when the IOMMU was enabled.
> >
> > This is due to the missing WriteBack area for the C2H stream.
> > Below my patch.
> > One point : I didn't compile it within the latest kernel's sources'
> > tree as it is an extract of our backport of the XDMA support.
> > Feel free to contact me on any issue with this.
> >
> > Hope this helps,
> > Eric.
> >
> > Below my patch (corrected).
> >
> >  From b8d71851e6a146dcb448b01a671f455afb09ae90 Mon Sep 17 00:00:00 2001
> > From: Eric DEBIEF <debief@digigram.com>
> > Date: Wed, 22 May 2024 12:33:06 +0200
> > Subject: FIX: DMAR Error with IO_MMU enabled.
> >
> > C2H write-back area was not allocated and set.
> > This leads to the DMAR Error.
> >
> > Add the Writeback structure, allocate and set it as
> > the descriptors's Src field.
> > Done for all preps functions.
> >
> > Signed-off-by: Eric DEBIEF <debief@digigram.com>
> > ---
> >   drivers/dma/xilinx/xdma.c | 44 +++++++++++++++++++++++++++++++++++----
> >   1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> > index 74d4a953b50f..9ae615165cb6 100644
> > --- a/drivers/dma/xilinx/xdma.c
> > +++ b/drivers/dma/xilinx/xdma.c
> > @@ -51,6 +51,20 @@ struct xdma_desc_block {
> >       dma_addr_t    dma_addr;
> >   };
> >
> > +/**
> > + * struct xdma_c2h_write_back  - Write back block , written by the XDMA.
> > + * @magic_status_bit : magic (0x52B4) once written
> > + * @length: effective transfer length (in bytes)
> > + * @PADDING to be aligned on 32 bytes
> > + * @associated dma address
> > + */
> > +struct xdma_c2h_write_back {
> > +    __le32 magic_status_bit;
> > +    __le32 length;
> > +    u32 padding_1[6];
> > +    dma_addr_t dma_addr;
> > +};
> > +
> >   /**
> >    * struct xdma_chan - Driver specific DMA channel structure
> >    * @vchan: Virtual channel
> > @@ -61,6 +75,8 @@ struct xdma_desc_block {
> >    * @dir: Transferring direction of the channel
> >    * @cfg: Transferring config of the channel
> >    * @irq: IRQ assigned to the channel
> > + * @write_back : C2H meta data write back
> > +
> >    */
> >   struct xdma_chan {
> >       struct virt_dma_chan        vchan;
> > @@ -73,6 +89,7 @@ struct xdma_chan {
> >       u32                irq;
> >       struct completion        last_interrupt;
> >       bool                stop_requested;
> > +    struct xdma_c2h_write_back *write_back;
> >   };
> >
> >   /**
> > @@ -628,7 +645,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct
> > scatterlist *sgl,
> >           src = &addr;
> >           dst = &dev_addr;
> >       } else {
> > -        dev_addr = xdma_chan->cfg.src_addr;
> > +        dev_addr = xdma_chan->cfg.src_addr ?
> > +            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
> >           src = &dev_addr;
> >           dst = &addr;
> >       }
> > @@ -705,7 +723,8 @@ xdma_prep_dma_cyclic(struct dma_chan *chan,
> > dma_addr_t address,
> >           src = &addr;
> >           dst = &dev_addr;
> >       } else {
> > -        dev_addr = xdma_chan->cfg.src_addr;
> > +        dev_addr = xdma_chan->cfg.src_addr ?
> > +            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
> >           src = &dev_addr;
> >           dst = &addr;
> >       }
> > @@ -803,6 +822,9 @@ static void xdma_free_chan_resources(struct dma_chan *chan)
> >       struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> >
> >       vchan_free_chan_resources(&xdma_chan->vchan);
> > +    dma_pool_free(xdma_chan->desc_pool,
> > +                xdma_chan->write_back,
> > +               xdma_chan->write_back->dma_addr);
> >       dma_pool_destroy(xdma_chan->desc_pool);
> >       xdma_chan->desc_pool = NULL;
> >   }
> > @@ -816,6 +838,7 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
> >       struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> >       struct xdma_device *xdev = xdma_chan->xdev_hdl;
> >       struct device *dev = xdev->dma_dev.dev;
> > +    dma_addr_t write_back_addr;
> >
> >       while (dev && !dev_is_pci(dev))
> >           dev = dev->parent;
> > @@ -824,13 +847,26 @@ static int xdma_alloc_chan_resources(struct
> > dma_chan *chan)
> >           return -EINVAL;
> >       }
> >
> > -    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
> > XDMA_DESC_BLOCK_SIZE,
> > -                           XDMA_DESC_BLOCK_ALIGN, XDMA_DESC_BLOCK_BOUNDARY);
> > +    //Allocate the pool WITH the H2C write back
> > +    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
> > +                            dev,
> > +                            XDMA_DESC_BLOCK_SIZE +
> > +                                sizeof(struct xdma_c2h_write_back),
> > +                            XDMA_DESC_BLOCK_ALIGN,
> > +                            XDMA_DESC_BLOCK_BOUNDARY);
> >       if (!xdma_chan->desc_pool) {
> >           xdma_err(xdev, "unable to allocate descriptor pool");
> >           return -ENOMEM;
> >       }
> >
> > +    /* Allocate the C2H write back out of the pool*/
> > +    xdma_chan->write_back = dma_pool_alloc(xdma_chan->desc_pool,
> > GFP_NOWAIT, &write_back_addr);
> > +    if (!xdma_chan->write_back) {
> > +        xdma_err(xdev, "unable to allocate C2H write back block");
> > +        return -ENOMEM;
> > +    }
> > +    xdma_chan->write_back->dma_addr = write_back_addr;
> > +
> >       return 0;
> >   }
> >
> > --
> > 2.34.1
diff mbox series

Patch

diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 74d4a953b50f..9ae615165cb6 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -51,6 +51,20 @@  struct xdma_desc_block {
     dma_addr_t    dma_addr;
 };

+/**
+ * struct xdma_c2h_write_back  - Write back block , written by the XDMA.
+ * @magic_status_bit : magic (0x52B4) once written
+ * @length: effective transfer length (in bytes)
+ * @PADDING to be aligned on 32 bytes
+ * @associated dma address
+ */
+struct xdma_c2h_write_back {
+    __le32 magic_status_bit;
+    __le32 length;
+    u32 padding_1[6];
+    dma_addr_t dma_addr;
+};
+
 /**
  * struct xdma_chan - Driver specific DMA channel structure
  * @vchan: Virtual channel
@@ -61,6 +75,8 @@  struct xdma_desc_block {
  * @dir: Transferring direction of the channel
  * @cfg: Transferring config of the channel
  * @irq: IRQ assigned to the channel
+ * @write_back : C2H meta data write back
+
  */
 struct xdma_chan {
     struct virt_dma_chan        vchan;
@@ -73,6 +89,7 @@  struct xdma_chan {
     u32                irq;
     struct completion        last_interrupt;
     bool                stop_requested;
+    struct xdma_c2h_write_back *write_back;
 };

 /**
@@ -628,7 +645,8 @@  xdma_prep_device_sg(struct dma_chan *chan, struct
scatterlist *sgl,
         src = &addr;
         dst = &dev_addr;
     } else {
-        dev_addr = xdma_chan->cfg.src_addr;
+        dev_addr = xdma_chan->cfg.src_addr ?
+            xdma_chan->cfg.src_addr : xdma_chan->write_back->dma_addr;
         src = &dev_addr;
         dst = &addr;