Enhancing Device Identification in RISC-V IOMMU Using Memory Attributes

[PATCH 3/3] hw/riscv/riscv_iommu: Remove the "bus" property

Posted by Jason Chien 11 months, 2 weeks ago

This property was originally intended to set the bus number for non-root
endpoints. However, since the PCIe bus number is assigned and modified
at runtime, setting this property before software execution is incorrect.
Additionally, the property incorrectly assumes that all endpoints share
the same bus, whereas no such restriction exists.

With the IOMMU now retrieving the latest device IDs from memory attributes,
there is no longer a need to set or update device IDs.

Signed-off-by: Jason Chien <jason.chien@sifive.com>
---
 hw/riscv/riscv-iommu.c | 7 -------
 hw/riscv/riscv-iommu.h | 1 -
 2 files changed, 8 deletions(-)

diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index b72ce8e6d0..1ca85b95ac 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -1197,9 +1197,6 @@ static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid)
 {
     RISCVIOMMUSpace *as;
 
-    /* FIXME: PCIe bus remapping for attached endpoints. */
-    devid |= s->bus << 8;
-
     QLIST_FOREACH(as, &s->spaces, list) {
         if (as->devid == devid) {
             break;
@@ -2261,9 +2258,6 @@ static MemTxResult riscv_iommu_trap_write(void *opaque, hwaddr addr,
         return MEMTX_ACCESS_ERROR;
     }
 
-    /* FIXME: PCIe bus remapping for attached endpoints. */
-    devid |= s->bus << 8;
-
     ctx = riscv_iommu_ctx(s, devid, 0, &ref);
     if (ctx == NULL) {
         res = MEMTX_ACCESS_ERROR;
@@ -2498,7 +2492,6 @@ void riscv_iommu_reset(RISCVIOMMUState *s)
 static const Property riscv_iommu_properties[] = {
     DEFINE_PROP_UINT32("version", RISCVIOMMUState, version,
         RISCV_IOMMU_SPEC_DOT_VER),
-    DEFINE_PROP_UINT32("bus", RISCVIOMMUState, bus, 0x0),
     DEFINE_PROP_UINT32("ioatc-limit", RISCVIOMMUState, iot_limit,
         LIMIT_CACHE_IOT),
     DEFINE_PROP_BOOL("intremap", RISCVIOMMUState, enable_msi, TRUE),
diff --git a/hw/riscv/riscv-iommu.h b/hw/riscv/riscv-iommu.h
index a31aa62144..655c0e71a8 100644
--- a/hw/riscv/riscv-iommu.h
+++ b/hw/riscv/riscv-iommu.h
@@ -34,7 +34,6 @@ struct RISCVIOMMUState {
     /*< public >*/
     uint32_t version;     /* Reported interface version number */
     uint32_t pid_bits;    /* process identifier width */
-    uint32_t bus;         /* PCI bus mapping for non-root endpoints */
 
     uint64_t cap;         /* IOMMU supported capabilities */
     uint64_t fctl;        /* IOMMU enabled features */
-- 
2.43.2

Re: [PATCH 3/3] hw/riscv/riscv_iommu: Remove the "bus" property

Posted by Alistair Francis 10 months, 1 week ago

On Sun, Mar 2, 2025 at 7:13 PM Jason Chien <jason.chien@sifive.com> wrote:
>
> This property was originally intended to set the bus number for non-root
> endpoints. However, since the PCIe bus number is assigned and modified
> at runtime, setting this property before software execution is incorrect.
> Additionally, the property incorrectly assumes that all endpoints share
> the same bus, whereas no such restriction exists.
>
> With the IOMMU now retrieving the latest device IDs from memory attributes,
> there is no longer a need to set or update device IDs.
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/riscv-iommu.c | 7 -------
>  hw/riscv/riscv-iommu.h | 1 -
>  2 files changed, 8 deletions(-)
>
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index b72ce8e6d0..1ca85b95ac 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -1197,9 +1197,6 @@ static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid)
>  {
>      RISCVIOMMUSpace *as;
>
> -    /* FIXME: PCIe bus remapping for attached endpoints. */
> -    devid |= s->bus << 8;
> -
>      QLIST_FOREACH(as, &s->spaces, list) {
>          if (as->devid == devid) {
>              break;
> @@ -2261,9 +2258,6 @@ static MemTxResult riscv_iommu_trap_write(void *opaque, hwaddr addr,
>          return MEMTX_ACCESS_ERROR;
>      }
>
> -    /* FIXME: PCIe bus remapping for attached endpoints. */
> -    devid |= s->bus << 8;
> -
>      ctx = riscv_iommu_ctx(s, devid, 0, &ref);
>      if (ctx == NULL) {
>          res = MEMTX_ACCESS_ERROR;
> @@ -2498,7 +2492,6 @@ void riscv_iommu_reset(RISCVIOMMUState *s)
>  static const Property riscv_iommu_properties[] = {
>      DEFINE_PROP_UINT32("version", RISCVIOMMUState, version,
>          RISCV_IOMMU_SPEC_DOT_VER),
> -    DEFINE_PROP_UINT32("bus", RISCVIOMMUState, bus, 0x0),
>      DEFINE_PROP_UINT32("ioatc-limit", RISCVIOMMUState, iot_limit,
>          LIMIT_CACHE_IOT),
>      DEFINE_PROP_BOOL("intremap", RISCVIOMMUState, enable_msi, TRUE),
> diff --git a/hw/riscv/riscv-iommu.h b/hw/riscv/riscv-iommu.h
> index a31aa62144..655c0e71a8 100644
> --- a/hw/riscv/riscv-iommu.h
> +++ b/hw/riscv/riscv-iommu.h
> @@ -34,7 +34,6 @@ struct RISCVIOMMUState {
>      /*< public >*/
>      uint32_t version;     /* Reported interface version number */
>      uint32_t pid_bits;    /* process identifier width */
> -    uint32_t bus;         /* PCI bus mapping for non-root endpoints */
>
>      uint64_t cap;         /* IOMMU supported capabilities */
>      uint64_t fctl;        /* IOMMU enabled features */
> --
> 2.43.2
>
>

Re: [PATCH 3/3] hw/riscv/riscv_iommu: Remove the "bus" property

Posted by Daniel Henrique Barboza 11 months, 1 week ago


On 3/2/25 6:12 AM, Jason Chien wrote:
> This property was originally intended to set the bus number for non-root
> endpoints. However, since the PCIe bus number is assigned and modified
> at runtime, setting this property before software execution is incorrect.
> Additionally, the property incorrectly assumes that all endpoints share
> the same bus, whereas no such restriction exists.
> 
> With the IOMMU now retrieving the latest device IDs from memory attributes,
> there is no longer a need to set or update device IDs.
> 
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/riscv/riscv-iommu.c | 7 -------
>   hw/riscv/riscv-iommu.h | 1 -
>   2 files changed, 8 deletions(-)
> 
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index b72ce8e6d0..1ca85b95ac 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -1197,9 +1197,6 @@ static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid)
>   {
>       RISCVIOMMUSpace *as;
>   
> -    /* FIXME: PCIe bus remapping for attached endpoints. */
> -    devid |= s->bus << 8;
> -
>       QLIST_FOREACH(as, &s->spaces, list) {
>           if (as->devid == devid) {
>               break;
> @@ -2261,9 +2258,6 @@ static MemTxResult riscv_iommu_trap_write(void *opaque, hwaddr addr,
>           return MEMTX_ACCESS_ERROR;
>       }
>   
> -    /* FIXME: PCIe bus remapping for attached endpoints. */
> -    devid |= s->bus << 8;
> -
>       ctx = riscv_iommu_ctx(s, devid, 0, &ref);
>       if (ctx == NULL) {
>           res = MEMTX_ACCESS_ERROR;
> @@ -2498,7 +2492,6 @@ void riscv_iommu_reset(RISCVIOMMUState *s)
>   static const Property riscv_iommu_properties[] = {
>       DEFINE_PROP_UINT32("version", RISCVIOMMUState, version,
>           RISCV_IOMMU_SPEC_DOT_VER),
> -    DEFINE_PROP_UINT32("bus", RISCVIOMMUState, bus, 0x0),
>       DEFINE_PROP_UINT32("ioatc-limit", RISCVIOMMUState, iot_limit,
>           LIMIT_CACHE_IOT),
>       DEFINE_PROP_BOOL("intremap", RISCVIOMMUState, enable_msi, TRUE),
> diff --git a/hw/riscv/riscv-iommu.h b/hw/riscv/riscv-iommu.h
> index a31aa62144..655c0e71a8 100644
> --- a/hw/riscv/riscv-iommu.h
> +++ b/hw/riscv/riscv-iommu.h
> @@ -34,7 +34,6 @@ struct RISCVIOMMUState {
>       /*< public >*/
>       uint32_t version;     /* Reported interface version number */
>       uint32_t pid_bits;    /* process identifier width */
> -    uint32_t bus;         /* PCI bus mapping for non-root endpoints */
>   
>       uint64_t cap;         /* IOMMU supported capabilities */
>       uint64_t fctl;        /* IOMMU enabled features */