Re: [RFC PATCH v4 04/10] iommu/riscv: use data structure instead of individual values
From: Gong Shuai
Date: Sun Sep 20 2026 - 01:32:13 EST
Hi Fangyu,
On 9/20/2026 11:08 AM, fangyu.yu@xxxxxxxxxxxxxxxxx wrote:
Hi Fangyu,
On 9/15/2026 11:28 AM, fangyu.yu@xxxxxxxxxxxxxxxxx wrote:
From: Zong Li <zong.li@xxxxxxxxxx>
The parameter will be increased when we need to set up more
bit fields in the device context. Use a data structure to
wrap them up.
Signed-off-by: Zong Li <zong.li@xxxxxxxxxx>
Signed-off-by: Fangyu Yu <fangyu.yu@xxxxxxxxxxxxxxxxx>
---
drivers/iommu/riscv/iommu.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
index a6c8307e82ea..c5b6214ee672 100644
--- a/drivers/iommu/riscv/iommu.c
+++ b/drivers/iommu/riscv/iommu.c
@@ -1165,7 +1165,7 @@ static void riscv_iommu_iodir_iotinval(struct riscv_iommu_device *iommu,
* 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;
@@ -1204,10 +1204,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;
According to SPEC 3.1.3.3, the "V bit" does not exist in DC.ta.
This should be a convention from when the iodir_update() function
did not yet use `struct riscv_iommu_dc` as a parameter, using bit 0
of DC.ta to indicate that the DC is valid. Now that iodir_update()
receives the complete dc, it can directly access DC.tc.V, so this
convention should no longer be necessary and will cause confusion.
Hi Shuai:
Thanks for the clarification.
My understanding is that `ta` here is used as a
combined concept, not only for `DC.ta`, but also
for `PC.ta` in this flow.
The `V` bit is only initialized when `ta` is first
set up, so the current usage still follows the
original design, without changing most of the
existing logic.
The original code also has comments describing
this behavior.
I got your point.
When fsc and ta were passed as separate raw parameters, treating
them as a combined concept was arguably reasonable, since the same
bit layout is shared between DC and PC in this path. But now that
iodir_update() takes a struct riscv_iommu_dc, I think it would be
better to keep the semantics explicit.
If PDT/PC support is added later, we can introduce a separate
struct riscv_iommu_pc parameter for the PC context, instead of
trying to make one ta/fsc cover both cases.
Thanks,Yeah, exactly, they share the same layout, so logically it's fine.
Fangyu
- 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);
`RISCV_IOMMU_PC_*` should be replaced with `RISCV_IOMMU_DC_*` for
readability. There are several other similar places in this file
that are unrelated to Process Context or PASID but use the *PC* macros.
My understanding is similar to the `ta` case: `fsc`
likely refers to both `dc.fsc` and `pc.fsc` in this
path, since some of their fields share the same bit
layout.
If we want to rename these for readability, I think
that should be done in a separate patch set, rather
than as part of this one.
Agree that it should be done in a separate patch.
Thanks,
Shuai
Thanks,
Shuai
/* Update device context, write TC.V as the last step. */
dma_wmb();
WRITE_ONCE(dc->tc, tc);
@@ -1288,22 +1288,22 @@ static int riscv_iommu_attach_paging_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 pt_iommu_riscv_64_hw_info pt_info;
- u64 fsc, ta;
+ struct riscv_iommu_dc dc = {0};
pt_iommu_riscv_64_hw_info(&domain->riscvpt, &pt_info);
if (!riscv_iommu_pt_supported(iommu, pt_info.fsc_iosatp_mode))
return -ENODEV;
- fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, pt_info.fsc_iosatp_mode) |
+ dc.fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, pt_info.fsc_iosatp_mode) |
FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, pt_info.ppn);
- ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
+ 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;
@@ -1378,9 +1378,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;
@@ -1400,8 +1403,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;