Re: [RFC PATCH v2 03/18] iommu/riscv: Use data structure instead of individual values

Nutty.Liu
2025-09-24T03:26:13Z

On 9/21/2025 4:38 AM, Andrew Jones wrote:
> From: Zong Li <zong.li@sifive.com>
>
> The parameter will be increased when we need to set up more fields
> in the device context. Use a data structure to wrap them up.
>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> drivers/iommu/riscv/iommu.c | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> index 901d02529a26..a44c67a848fa 100644
> --- a/drivers/iommu/riscv/iommu.c
> +++ b/drivers/iommu/riscv/iommu.c
> @@ -988,7 +988,7 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
> * interim translation faults.
> */
> static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
> - struct device *dev, u64 fsc, u64 ta)
> + struct device *dev, struct riscv_iommu_dc *new_dc)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct riscv_iommu_dc *dc;
> @@ -1022,10 +1022,10 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
> for (i = 0; i < fwspec->num_ids; i++) {
> dc = riscv_iommu_get_dc(iommu, fwspec->ids[i]);
> tc = READ_ONCE(dc->tc);
> - tc |= ta & RISCV_IOMMU_DC_TC_V;
> + tc |= new_dc->ta & RISCV_IOMMU_DC_TC_V;
>
> - WRITE_ONCE(dc->fsc, fsc);
> - WRITE_ONCE(dc->ta, ta & RISCV_IOMMU_PC_TA_PSCID);
> + WRITE_ONCE(dc->fsc, new_dc->fsc);
> + WRITE_ONCE(dc->ta, new_dc->ta & RISCV_IOMMU_PC_TA_PSCID);
Seems it will override all other fields in 'TA' except for the field of
'PSCID'.
Should the other fields remain unchanged ?
Otherwise,
Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>

Thanks,
Nutty
> /* Update device context, write TC.V as the last step. */
> dma_wmb();
> WRITE_ONCE(dc->tc, tc);
> @@ -1304,20 +1304,20 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
> struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain);
> struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> - u64 fsc, ta;
> + struct riscv_iommu_dc dc = {0};
>
> if (!riscv_iommu_pt_supported(iommu, domain->pgd_mode))
> return -ENODEV;
>
> - fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, domain->pgd_mode) |
> - FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, virt_to_pfn(domain->pgd_root));
> - ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
> - RISCV_IOMMU_PC_TA_V;
> + dc.fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, domain->pgd_mode) |
> + FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, virt_to_pfn(domain->pgd_root));
> + dc.ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
> + RISCV_IOMMU_PC_TA_V;
>
> if (riscv_iommu_bond_link(domain, dev))
> return -ENOMEM;
>
> - riscv_iommu_iodir_update(iommu, dev, fsc, ta);
> + riscv_iommu_iodir_update(iommu, dev, &dc);
> riscv_iommu_bond_unlink(info->domain, dev);
> info->domain = domain;
>
> @@ -1408,9 +1408,12 @@ static int riscv_iommu_attach_blocking_domain(struct iommu_domain *iommu_domain,
> {
> struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> + struct riscv_iommu_dc dc = {0};
> +
> + dc.fsc = RISCV_IOMMU_FSC_BARE;
>
> /* Make device context invalid, translation requests will fault w/ #258 */
> - riscv_iommu_iodir_update(iommu, dev, RISCV_IOMMU_FSC_BARE, 0);
> + riscv_iommu_iodir_update(iommu, dev, &dc);
> riscv_iommu_bond_unlink(info->domain, dev);
> info->domain = NULL;
>
> @@ -1429,8 +1432,12 @@ static int riscv_iommu_attach_identity_domain(struct iommu_domain *iommu_domain,
> {
> struct riscv_iommu_device *iommu = dev_to_iommu(dev);
> struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> + struct riscv_iommu_dc dc = {0};
> +
> + dc.fsc = RISCV_IOMMU_FSC_BARE;
> + dc.ta = RISCV_IOMMU_PC_TA_V;
>
> - riscv_iommu_iodir_update(iommu, dev, RISCV_IOMMU_FSC_BARE, RISCV_IOMMU_PC_TA_V);
> + riscv_iommu_iodir_update(iommu, dev, &dc);
> riscv_iommu_bond_unlink(info->domain, dev);
> info->domain = NULL;
>