Re: [RFC PATCH 5/6] hw/riscv/riscv-iommu: Acquire device IDs dynamically

[Daniel Henrique Barboza]

On 12/15/24 9:48 PM, Jason Chien wrote:
> When all memory transactions from a PCIe host write to the same IOMMU
> memory region, we need to distinguish the source device dynamically.
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---

I'm not sure whether this should be squashed in patch 4 but the code LGTM,
so either way:


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

>   hw/riscv/riscv-iommu.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index f5d53a36b2..e4b7008306 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -48,6 +48,7 @@ struct RISCVIOMMUSpace {
>       RISCVIOMMUState *iommu;     /* Managing IOMMU device state */
>       uint32_t devid;             /* Requester identifier, AKA device_id */
>       bool notifier;              /* IOMMU unmap notifier enabled */
> +    bool dynamic_devid;         /* Acquiring device_id dynamically */
>       QLIST_ENTRY(RISCVIOMMUSpace) list;
>   };
>   
> @@ -1184,7 +1185,8 @@ static void riscv_iommu_ctx_put(RISCVIOMMUState *s, void *ref)
>   }
>   
>   /* Find or allocate address space for a given device */
> -static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid)
> +static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid,
> +                                       bool dynamic)
>   {
>       RISCVIOMMUSpace *as;
>   
> @@ -1203,6 +1205,7 @@ static AddressSpace *riscv_iommu_space(RISCVIOMMUState *s, uint32_t devid)
>   
>           as->iommu = s;
>           as->devid = devid;
> +        as->dynamic_devid = dynamic;
>   
>           snprintf(name, sizeof(name), "riscv-iommu-%04x:%02x.%d-iova",
>               PCI_BUS_NUM(as->devid), PCI_SLOT(as->devid), PCI_FUNC(as->devid));
> @@ -2415,7 +2418,8 @@ static AddressSpace *riscv_iommu_find_as(PCIBus *bus, 
> void *opaque, int devfn)
>   
>       /* Find first matching IOMMU */
>       while (s != NULL && as == NULL) {
> -        as = riscv_iommu_space(s, PCI_BUILD_BDF(pci_bus_num(bus), devfn));
> +        as = riscv_iommu_space(s, PCI_BUILD_BDF(pci_bus_num(bus), devfn),
> +                               false);
>           s = s->iommus.le_next;
>       }
>   
> @@ -2438,11 +2442,10 @@ void riscv_iommu_pci_setup_iommu(RISCVIOMMUState *iommu, PCIBus *bus,
>           pci_setup_iommu(bus, &riscv_iommu_ops, iommu);
>       } else if (bus->iommu_ops && bus->iommu_ops->set_memory_region) {
>           /*
> -         * TODO:
>            * All memory transactions of this bus will be directed to this AS.
>            * We need to distinguish the source device dynamically.
>            */
> -        AddressSpace *as = riscv_iommu_space(iommu, 0);
> +        AddressSpace *as = riscv_iommu_space(iommu, 0, true);
>           pci_setup_iommu_downstream_mem(bus, as->root);
>       } else {
>           error_setg(errp, "can't register secondary IOMMU for PCI bus #%d",
> @@ -2453,6 +2456,12 @@ void riscv_iommu_pci_setup_iommu(RISCVIOMMUState *iommu, 
> PCIBus *bus,
>   static int riscv_iommu_memory_region_index(IOMMUMemoryRegion *iommu_mr,
>       MemTxAttrs attrs)
>   {
> +    RISCVIOMMUSpace *as = container_of(iommu_mr, RISCVIOMMUSpace, iova_mr);
> +
> +    if (as->dynamic_devid) {
> +        as->devid = attrs.requester_id;
> +    }
> +
>       return attrs.unspecified ? RISCV_IOMMU_NOPROCID : (int)attrs.pid;
>   }
>