Re: [PATCH 01/11] RISC-V: drivers/iommu: Add RISC-V IOMMU - Ziommu support.

[Jason Gunthorpe <jgg@nvidia.com>]

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