Messages in this thread | | | Date | Wed, 2 Aug 2023 21:18:35 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH 01/11] RISC-V: drivers/iommu: Add RISC-V IOMMU - Ziommu support. |
| |
On Wed, Jul 19, 2023 at 12:33:45PM -0700, Tomasz Jeznach wrote:
> +static int riscv_iommu_domain_finalize(struct riscv_iommu_domain *domain, > + struct riscv_iommu_device *iommu) > +{
Do not introduce this finalize pattern into new drivers. We are trying to get rid of it. I don't see anything here that suggest you need it.
Do all of this when you allocate the domain.
> + struct iommu_domain_geometry *geometry; > + > + /* Domain assigned to another iommu */ > + if (domain->iommu && domain->iommu != iommu) > + return -EINVAL; > + /* Domain already initialized */ > + else if (domain->iommu) > + return 0;
These tests are not good, the domain should be able to be associated with as many iommu instances as it likes.
> +static int riscv_iommu_attach_dev(struct iommu_domain *iommu_domain, struct device *dev) > +{ > + struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain); > + struct riscv_iommu_endpoint *ep = dev_iommu_priv_get(dev); > + int ret; > + > + /* PSCID not valid */ > + if ((int)domain->pscid < 0) > + return -ENOMEM; > + > + mutex_lock(&domain->lock); > + mutex_lock(&ep->lock); > + > + if (!list_empty(&ep->domain)) { > + dev_warn(dev, "endpoint already attached to a domain. dropping\n");
This is legitimate, it means the driver has to replace the domain, and drivers have to implement this.
> +/* > + * Common I/O MMU driver probe/teardown > + */ > + > +static const struct iommu_domain_ops riscv_iommu_domain_ops = { > + .free = riscv_iommu_domain_free, > + .attach_dev = riscv_iommu_attach_dev, > + .map_pages = riscv_iommu_map_pages, > + .unmap_pages = riscv_iommu_unmap_pages, > + .iova_to_phys = riscv_iommu_iova_to_phys, > + .iotlb_sync = riscv_iommu_iotlb_sync, > + .iotlb_sync_map = riscv_iommu_iotlb_sync_map, > + .flush_iotlb_all = riscv_iommu_flush_iotlb_all, > +};
Please split the ops by domain type, eg identity, paging, sva, etc.
> +int riscv_iommu_init(struct riscv_iommu_device *iommu) > +{ > + struct device *dev = iommu->dev; > + u32 fctl = 0; > + int ret; > + > + iommu->eps = RB_ROOT; > + > + fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL); > + > +#ifdef CONFIG_CPU_BIG_ENDIAN > + if (!(cap & RISCV_IOMMU_CAP_END)) { > + dev_err(dev, "IOMMU doesn't support Big Endian\n");
Why not?
> + return -EIO; > + } else if (!(fctl & RISCV_IOMMU_FCTL_BE)) { > + fctl |= FIELD_PREP(RISCV_IOMMU_FCTL_BE, 1); > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, fctl); > + } > +#endif > + > + /* Clear any pending interrupt flag. */ > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_IPSR, > + RISCV_IOMMU_IPSR_CIP | > + RISCV_IOMMU_IPSR_FIP | > + RISCV_IOMMU_IPSR_PMIP | RISCV_IOMMU_IPSR_PIP); > + spin_lock_init(&iommu->cq_lock); > + mutex_init(&iommu->eps_mutex); > + > + ret = riscv_iommu_enable(iommu, RISCV_IOMMU_DDTP_MODE_BARE); > + > + if (ret) { > + dev_err(dev, "cannot enable iommu device (%d)\n", ret); > + goto fail; > + } > + > + ret = iommu_device_register(&iommu->iommu, &riscv_iommu_ops, dev); > + if (ret) { > + dev_err(dev, "cannot register iommu interface (%d)\n", ret); > + iommu_device_sysfs_remove(&iommu->iommu); > + goto fail; > + }
The calls to iommu_device_sysfs_add() are missing, this is mandatory..
Jason
| |