Enhancing Device Identification in RISC-V IOMMU Using Memory Attributes

[PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Jason Chien 11 months, 2 weeks ago

This commit adds the BDF to the memory attributes for DMA operations.

Signed-off-by: Jason Chien <jason.chien@sifive.com>
---
 include/hw/pci/pci_device.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index add208edfa..968f1ba3e9 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
                                      void *buf, dma_addr_t len,
                                      DMADirection dir, MemTxAttrs attrs)
 {
+    attrs.unspecified = 0;
+    attrs.requester_id = pci_requester_id(dev);
     return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
                          dir, attrs);
 }
@@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
                                                uint##_bits##_t *val, \
                                                MemTxAttrs attrs) \
     { \
+        attrs.unspecified = 0; \
+        attrs.requester_id = pci_requester_id(dev); \
         return ld##_l##_dma(pci_get_address_space(dev), addr, val, attrs); \
     } \
     static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
@@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
                                                uint##_bits##_t val, \
                                                MemTxAttrs attrs) \
     { \
+        attrs.unspecified = 0; \
+        attrs.requester_id = pci_requester_id(dev); \
         return st##_s##_dma(pci_get_address_space(dev), addr, val, attrs); \
     }
 
@@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
 static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
                                 dma_addr_t *plen, DMADirection dir)
 {
-    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
-                          MEMTXATTRS_UNSPECIFIED);
+    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
+    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir, attrs);
 }
 
 static inline void pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t len,
-- 
2.43.2

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Michael S. Tsirkin 10 months ago

On Sun, Mar 02, 2025 at 05:12:07PM +0800, Jason Chien wrote:
> This commit adds the BDF to the memory attributes for DMA operations.
> 
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---
>  include/hw/pci/pci_device.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index add208edfa..968f1ba3e9 100644
> --- a/include/hw/pci/pci_device.h
> +++ b/include/hw/pci/pci_device.h
> @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
>                                       void *buf, dma_addr_t len,
>                                       DMADirection dir, MemTxAttrs attrs)
>  {
> +    attrs.unspecified = 0;
> +    attrs.requester_id = pci_requester_id(dev);
>      return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
>                           dir, attrs);
>  }
> @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
>                                                 uint##_bits##_t *val, \
>                                                 MemTxAttrs attrs) \
>      { \
> +        attrs.unspecified = 0; \
> +        attrs.requester_id = pci_requester_id(dev); \
>          return ld##_l##_dma(pci_get_address_space(dev), addr, val, attrs); \
>      } \
>      static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
> @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
>                                                 uint##_bits##_t val, \
>                                                 MemTxAttrs attrs) \
>      { \
> +        attrs.unspecified = 0; \
> +        attrs.requester_id = pci_requester_id(dev); \
>          return st##_s##_dma(pci_get_address_space(dev), addr, val, attrs); \
>      }
>  
> @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
>  static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
>                                  dma_addr_t *plen, DMADirection dir)
>  {
> -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> -                          MEMTXATTRS_UNSPECIFIED);
> +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
> +    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir, attrs);
>  }


Map is the only issue  - bdf can technically change between map and
unmap.
The use in hw/net/net_tx_pkt.c is fine as it's under BQL.
I don't know about the use in megasas though.
I think it is probably fine as it seems to deal with commands
and I think any driver would flush these if changing BDF.
Cc megasas maintainers just to make sure though.

Also, adding a code comment here can't hurt.


>  static inline void pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t len,
> -- 
> 2.43.2

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Jason Chien 9 months, 3 weeks ago

Adding requester_id here does not break anything, since pci_dma_map()
passes MEMTXATTRS_UNSPECIFIED to dma_memory_map() and requester_id is
unused.

I'll add the below for the comment:
Attach BDF here for use during subsequent IOMMU translation.

Michael S. Tsirkin <mst@redhat.com> 於 2025年4月14日 週一 下午11:28寫道：

> On Sun, Mar 02, 2025 at 05:12:07PM +0800, Jason Chien wrote:
> > This commit adds the BDF to the memory attributes for DMA operations.
> >
> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
> > ---
> >  include/hw/pci/pci_device.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> > index add208edfa..968f1ba3e9 100644
> > --- a/include/hw/pci/pci_device.h
> > +++ b/include/hw/pci/pci_device.h
> > @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev,
> dma_addr_t addr,
> >                                       void *buf, dma_addr_t len,
> >                                       DMADirection dir, MemTxAttrs attrs)
> >  {
> > +    attrs.unspecified = 0;
> > +    attrs.requester_id = pci_requester_id(dev);
> >      return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
> >                           dir, attrs);
> >  }
> > @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
> *dev, dma_addr_t addr,
> >                                                 uint##_bits##_t *val, \
> >                                                 MemTxAttrs attrs) \
> >      { \
> > +        attrs.unspecified = 0; \
> > +        attrs.requester_id = pci_requester_id(dev); \
> >          return ld##_l##_dma(pci_get_address_space(dev), addr, val,
> attrs); \
> >      } \
> >      static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
> > @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
> *dev, dma_addr_t addr,
> >                                                 uint##_bits##_t val, \
> >                                                 MemTxAttrs attrs) \
> >      { \
> > +        attrs.unspecified = 0; \
> > +        attrs.requester_id = pci_requester_id(dev); \
> >          return st##_s##_dma(pci_get_address_space(dev), addr, val,
> attrs); \
> >      }
> >
> > @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
> >  static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
> >                                  dma_addr_t *plen, DMADirection dir)
> >  {
> > -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> > -                          MEMTXATTRS_UNSPECIFIED);
> > +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
> > +    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> attrs);
> >  }
>
>
> Map is the only issue  - bdf can technically change between map and
> unmap.
> The use in hw/net/net_tx_pkt.c is fine as it's under BQL.
> I don't know about the use in megasas though.
> I think it is probably fine as it seems to deal with commands
> and I think any driver would flush these if changing BDF.
> Cc megasas maintainers just to make sure though.
>
> Also, adding a code comment here can't hurt.
>
>
> >  static inline void pci_dma_unmap(PCIDevice *dev, void *buffer,
> dma_addr_t len,
> > --
> > 2.43.2
>
>

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Daniel Henrique Barboza 11 months, 1 week ago


On 3/2/25 6:12 AM, Jason Chien wrote:
> This commit adds the BDF to the memory attributes for DMA operations.
> 
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---

This looks sensible but I'll feel more comfortable if Michael/Marcel also
takes a look. Thanks,


Daniel

>   include/hw/pci/pci_device.h | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> index add208edfa..968f1ba3e9 100644
> --- a/include/hw/pci/pci_device.h
> +++ b/include/hw/pci/pci_device.h
> @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
>                                        void *buf, dma_addr_t len,
>                                        DMADirection dir, MemTxAttrs attrs)
>   {
> +    attrs.unspecified = 0;
> +    attrs.requester_id = pci_requester_id(dev);
>       return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
>                            dir, attrs);
>   }
> @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
>                                                  uint##_bits##_t *val, \
>                                                  MemTxAttrs attrs) \
>       { \
> +        attrs.unspecified = 0; \
> +        attrs.requester_id = pci_requester_id(dev); \
>           return ld##_l##_dma(pci_get_address_space(dev), addr, val, attrs); \
>       } \
>       static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
> @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
>                                                  uint##_bits##_t val, \
>                                                  MemTxAttrs attrs) \
>       { \
> +        attrs.unspecified = 0; \
> +        attrs.requester_id = pci_requester_id(dev); \
>           return st##_s##_dma(pci_get_address_space(dev), addr, val, attrs); \
>       }
>   
> @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
>   static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
>                                   dma_addr_t *plen, DMADirection dir)
>   {
> -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> -                          MEMTXATTRS_UNSPECIFIED);
> +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
> +    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir, attrs);
>   }
>   
>   static inline void pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t len,

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Jason Chien 11 months ago

Ping.

Michael/Marcel, would you mind taking a look? Thanks!


Jason

Daniel Henrique Barboza <dbarboza@ventanamicro.com> 於 2025年3月7日 週五 下午8:40寫道：

>
>
> On 3/2/25 6:12 AM, Jason Chien wrote:
> > This commit adds the BDF to the memory attributes for DMA operations.
> >
> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
> > ---
>
> This looks sensible but I'll feel more comfortable if Michael/Marcel also
> takes a look. Thanks,
>
>
> Daniel
>
> >   include/hw/pci/pci_device.h | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> > index add208edfa..968f1ba3e9 100644
> > --- a/include/hw/pci/pci_device.h
> > +++ b/include/hw/pci/pci_device.h
> > @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev,
> dma_addr_t addr,
> >                                        void *buf, dma_addr_t len,
> >                                        DMADirection dir, MemTxAttrs
> attrs)
> >   {
> > +    attrs.unspecified = 0;
> > +    attrs.requester_id = pci_requester_id(dev);
> >       return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
> >                            dir, attrs);
> >   }
> > @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
> *dev, dma_addr_t addr,
> >                                                  uint##_bits##_t *val, \
> >                                                  MemTxAttrs attrs) \
> >       { \
> > +        attrs.unspecified = 0; \
> > +        attrs.requester_id = pci_requester_id(dev); \
> >           return ld##_l##_dma(pci_get_address_space(dev), addr, val,
> attrs); \
> >       } \
> >       static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
> > @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
> *dev, dma_addr_t addr,
> >                                                  uint##_bits##_t val, \
> >                                                  MemTxAttrs attrs) \
> >       { \
> > +        attrs.unspecified = 0; \
> > +        attrs.requester_id = pci_requester_id(dev); \
> >           return st##_s##_dma(pci_get_address_space(dev), addr, val,
> attrs); \
> >       }
> >
> > @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
> >   static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
> >                                   dma_addr_t *plen, DMADirection dir)
> >   {
> > -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> > -                          MEMTXATTRS_UNSPECIFIED);
> > +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
> > +    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> attrs);
> >   }
> >
> >   static inline void pci_dma_unmap(PCIDevice *dev, void *buffer,
> dma_addr_t len,
>
>

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Jason Chien 10 months, 3 weeks ago

Ping

Jason Chien <jason.chien@sifive.com> 於 2025年3月13日 週四 上午12:59寫道：

> Ping.
>
> Michael/Marcel, would you mind taking a look? Thanks!
>
>
> Jason
>
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> 於 2025年3月7日 週五
> 下午8:40寫道：
>
>>
>>
>> On 3/2/25 6:12 AM, Jason Chien wrote:
>> > This commit adds the BDF to the memory attributes for DMA operations.
>> >
>> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
>> > ---
>>
>> This looks sensible but I'll feel more comfortable if Michael/Marcel also
>> takes a look. Thanks,
>>
>>
>> Daniel
>>
>> >   include/hw/pci/pci_device.h | 10 ++++++++--
>> >   1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
>> > index add208edfa..968f1ba3e9 100644
>> > --- a/include/hw/pci/pci_device.h
>> > +++ b/include/hw/pci/pci_device.h
>> > @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice
>> *dev, dma_addr_t addr,
>> >                                        void *buf, dma_addr_t len,
>> >                                        DMADirection dir, MemTxAttrs
>> attrs)
>> >   {
>> > +    attrs.unspecified = 0;
>> > +    attrs.requester_id = pci_requester_id(dev);
>> >       return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
>> >                            dir, attrs);
>> >   }
>> > @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
>> *dev, dma_addr_t addr,
>> >                                                  uint##_bits##_t *val, \
>> >                                                  MemTxAttrs attrs) \
>> >       { \
>> > +        attrs.unspecified = 0; \
>> > +        attrs.requester_id = pci_requester_id(dev); \
>> >           return ld##_l##_dma(pci_get_address_space(dev), addr, val,
>> attrs); \
>> >       } \
>> >       static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
>> > @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
>> *dev, dma_addr_t addr,
>> >                                                  uint##_bits##_t val, \
>> >                                                  MemTxAttrs attrs) \
>> >       { \
>> > +        attrs.unspecified = 0; \
>> > +        attrs.requester_id = pci_requester_id(dev); \
>> >           return st##_s##_dma(pci_get_address_space(dev), addr, val,
>> attrs); \
>> >       }
>> >
>> > @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
>> >   static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
>> >                                   dma_addr_t *plen, DMADirection dir)
>> >   {
>> > -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
>> > -                          MEMTXATTRS_UNSPECIFIED);
>> > +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
>> > +    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
>> attrs);
>> >   }
>> >
>> >   static inline void pci_dma_unmap(PCIDevice *dev, void *buffer,
>> dma_addr_t len,
>>
>>

Re: [PATCH 1/3] include/hw/pci: Attach BDF to Memory Attributes

Posted by Jason Chien 10 months ago

Ping.

Jason Chien <jason.chien@sifive.com> 於 2025年3月20日 週四 上午12:40寫道：

> Ping
>
> Jason Chien <jason.chien@sifive.com> 於 2025年3月13日 週四 上午12:59寫道：
>
>> Ping.
>>
>> Michael/Marcel, would you mind taking a look? Thanks!
>>
>>
>> Jason
>>
>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> 於 2025年3月7日 週五
>> 下午8:40寫道：
>>
>>>
>>>
>>> On 3/2/25 6:12 AM, Jason Chien wrote:
>>> > This commit adds the BDF to the memory attributes for DMA operations.
>>> >
>>> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
>>> > ---
>>>
>>> This looks sensible but I'll feel more comfortable if Michael/Marcel also
>>> takes a look. Thanks,
>>>
>>>
>>> Daniel
>>>
>>> >   include/hw/pci/pci_device.h | 10 ++++++++--
>>> >   1 file changed, 8 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
>>> > index add208edfa..968f1ba3e9 100644
>>> > --- a/include/hw/pci/pci_device.h
>>> > +++ b/include/hw/pci/pci_device.h
>>> > @@ -244,6 +244,8 @@ static inline MemTxResult pci_dma_rw(PCIDevice
>>> *dev, dma_addr_t addr,
>>> >                                        void *buf, dma_addr_t len,
>>> >                                        DMADirection dir, MemTxAttrs
>>> attrs)
>>> >   {
>>> > +    attrs.unspecified = 0;
>>> > +    attrs.requester_id = pci_requester_id(dev);
>>> >       return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
>>> >                            dir, attrs);
>>> >   }
>>> > @@ -292,6 +294,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
>>> *dev, dma_addr_t addr,
>>> >                                                  uint##_bits##_t *val,
>>> \
>>> >                                                  MemTxAttrs attrs) \
>>> >       { \
>>> > +        attrs.unspecified = 0; \
>>> > +        attrs.requester_id = pci_requester_id(dev); \
>>> >           return ld##_l##_dma(pci_get_address_space(dev), addr, val,
>>> attrs); \
>>> >       } \
>>> >       static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
>>> > @@ -299,6 +303,8 @@ static inline MemTxResult pci_dma_write(PCIDevice
>>> *dev, dma_addr_t addr,
>>> >                                                  uint##_bits##_t val, \
>>> >                                                  MemTxAttrs attrs) \
>>> >       { \
>>> > +        attrs.unspecified = 0; \
>>> > +        attrs.requester_id = pci_requester_id(dev); \
>>> >           return st##_s##_dma(pci_get_address_space(dev), addr, val,
>>> attrs); \
>>> >       }
>>> >
>>> > @@ -327,8 +333,8 @@ PCI_DMA_DEFINE_LDST(q_be, q_be, 64);
>>> >   static inline void *pci_dma_map(PCIDevice *dev, dma_addr_t addr,
>>> >                                   dma_addr_t *plen, DMADirection dir)
>>> >   {
>>> > -    return dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
>>> > -                          MEMTXATTRS_UNSPECIFIED);
>>> > +    MemTxAttrs attrs = {.requester_id = pci_requester_id(dev)};
>>> > +    return dma_memory_map(pci_get_address_space(dev), addr, plen,
>>> dir, attrs);
>>> >   }
>>> >
>>> >   static inline void pci_dma_unmap(PCIDevice *dev, void *buffer,
>>> dma_addr_t len,
>>>
>>>