| Message ID | 20250920203851.2205115-23-ajones@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/riscv: Add irqbypass support | expand |
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; >
On Wed, Sep 24, 2025 at 11:25:59AM +0800, Nutty.Liu wrote: > 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 ? The short answer is that the current implementation is doing the right thing. The long answer is that riscv_iommu_iodir_update() and how it's called from riscv_iommu_attach_paging_domain() could use some cleanup. A more logical interface would be that new_dc would be completely written, which means any fields left zero when creating new_dc will result in zeros being written -- it doesn't do that right now. Also, rather than passing DC_TC_V through new_dc->ta (as PC_TA_V, even though DC_TC_PDTV = 0), we should probably just set it directly in new_dc->tc. We can clean this up separately though, probably as work for adding SVA support. > Otherwise, > Reviewed-by: Nutty Liu <nutty.liu@hotmail.com> Thanks, drew > > 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;
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); /* 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;