On Fri, Jan 15, 2021 at 05:43:40PM +0530, Vivek Gautam wrote:
[...]

+static int viommu_setup_pgtable(struct viommu_endpoint *vdev,
+				struct viommu_domain *vdomain)
+{
+	int ret, id;
+	u32 asid;
+	enum io_pgtable_fmt fmt;
+	struct io_pgtable_ops *ops = NULL;
+	struct viommu_dev *viommu = vdev->viommu;
+	struct virtio_iommu_probe_table_format *desc = vdev->pgtf;
+	struct iommu_pasid_table *tbl = vdomain->pasid_tbl;
+	struct iommu_vendor_psdtable_cfg *pst_cfg;
+	struct arm_smmu_cfg_info *cfgi;
+	struct io_pgtable_cfg cfg = {
+		.iommu_dev	= viommu->dev->parent,
+		.tlb		= &viommu_flush_ops,
+		.pgsize_bitmap	= vdev->pgsize_mask ? vdev->pgsize_mask :
+				  vdomain->domain.pgsize_bitmap,
+		.ias		= (vdev->input_end ? ilog2(vdev->input_end) :
+				   ilog2(vdomain->domain.geometry.aperture_end)) + 1,
+		.oas		= vdev->output_bits,
+	};
+
+	if (!desc)
+		return -EINVAL;
+
+	if (!vdev->output_bits)
+		return -ENODEV;
+
+	switch (le16_to_cpu(desc->format)) {
+	case VIRTIO_IOMMU_FOMRAT_PGTF_ARM_LPAE:
+		fmt = ARM_64_LPAE_S1;
+		break;
+	default:
+		dev_err(vdev->dev, "unsupported page table format 0x%x\n",
+			le16_to_cpu(desc->format));
+		return -EINVAL;
+	}
+
+	if (vdomain->mm.ops) {
+		/*
+		 * TODO: attach additional endpoint to the domain. Check that
+		 * the config is sane.
+		 */
+		return -EEXIST;
+	}
+
+	vdomain->mm.domain = vdomain;
+	ops = alloc_io_pgtable_ops(fmt, &cfg, &vdomain->mm);
+	if (!ops)
+		return -ENOMEM;
+
+	pst_cfg = &tbl->cfg;
+	cfgi = &pst_cfg->vendor.cfg;
+	id = ida_simple_get(&asid_ida, 1, 1 << desc->asid_bits, GFP_KERNEL);
+	if (id < 0) {
+		ret = id;
+		goto err_free_pgtable;
+	}
+
+	asid = id;
+	ret = iommu_psdtable_prepare(tbl, pst_cfg, &cfg, asid);
+	if (ret)
+		goto err_free_asid;
+
+	/*
+	 * Strange to setup an op here?
+	 * cd-lib is the actual user of sync op, and therefore the platform
+	 * drivers should assign this sync/maintenance ops as per need.
+	 */
+	tbl->ops->sync = viommu_flush_pasid;

But this function deals with page tables, not pasid tables. As said on an
earlier patch, the TLB flush ops should probably be passed during table
registration - those ops are global so should really be const.

+
+	/* Right now only PASID 0 supported ?? */
+	ret = iommu_psdtable_write(tbl, pst_cfg, 0, &cfgi->s1_cfg->cd);
+	if (ret)
+		goto err_free_asid;
+
+	vdomain->mm.ops = ops;
+	dev_dbg(vdev->dev, "using page table format 0x%x\n", fmt);
+
+	return 0;
+
+err_free_asid:
+	ida_simple_remove(&asid_ida, asid);
+err_free_pgtable:
+	free_io_pgtable_ops(ops);
+	return ret;
+}
+
+static int viommu_config_arm_pst(struct iommu_vendor_psdtable_cfg *pst_cfg,
+				 struct virtio_iommu_req_attach_pst_arm *req)
+{
+	struct arm_smmu_s1_cfg *s1_cfg = pst_cfg->vendor.cfg.s1_cfg;
+
+	if (!s1_cfg)
+		return -ENODEV;
+
+	req->format	= cpu_to_le16(VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3);
+	req->s1fmt	= s1_cfg->s1fmt;
+	req->s1dss	= VIRTIO_IOMMU_PSTF_ARM_SMMU_V3_DSS_0;
+	req->s1contextptr = cpu_to_le64(pst_cfg->base);
+	req->s1cdmax	= cpu_to_le32(s1_cfg->s1cdmax);
+
+	return 0;
+}
+
+static int viommu_config_pst(struct iommu_vendor_psdtable_cfg *pst_cfg,
+			     void *req, enum pasid_table_fmt fmt)
+{
+	int ret;
+
+	switch (fmt) {
+	case PASID_TABLE_ARM_SMMU_V3:
+		ret = viommu_config_arm_pst(pst_cfg, req);
+		break;
+	default:
+		ret = -EINVAL;
+		WARN_ON(1);
+	}
+
+	return ret;
+}
+
+static int viommu_prepare_arm_pst(struct viommu_endpoint *vdev,
+				  struct iommu_vendor_psdtable_cfg *pst_cfg)
+{
+	struct virtio_iommu_probe_table_format *pgtf = vdev->pgtf;
+	struct arm_smmu_cfg_info *cfgi = &pst_cfg->vendor.cfg;
+	struct arm_smmu_s1_cfg *cfg;
+
+	/* Some sanity checks */
+	if (pgtf->asid_bits != 8 && pgtf->asid_bits != 16)
+		return -EINVAL;

No need for this, next patch cheks asid size in viommu_config_arm_pgt()

+
+	cfg = devm_kzalloc(pst_cfg->iommu_dev, sizeof(cfg), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	cfgi->s1_cfg = cfg;
+	cfg->s1cdmax = vdev->pasid_bits;
+	cfg->cd.asid = pgtf->asid_bits;

That doesn't look right, cfg->cd.asid takes the ASID value of context 0
but here we're writing a limit. viommu_setup_pgtable() probably needs to
set this field to the allocated ASID, since viommu_teardown_pgtable() uses
it.

+
+	pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;

Parent function can set this

+	/* XXX HACK: set feature bit ARM_SMMU_FEAT_2_LVL_CDTAB */
+	pst_cfg->vendor.cfg.feat_flag |= (1 << 1);

Oh right, this flag is missing. I'll add

  #define VIRTIO_IOMMU_PST_ARM_SMMU3_F_CD2L (1ULL << 1)

to the spec.

+
+	return 0;
+}
+
+static int viommu_prepare_pst(struct viommu_endpoint *vdev,
+			      struct iommu_vendor_psdtable_cfg *pst_cfg,
+			      enum pasid_table_fmt fmt)
+{
+	int ret;
+
+	switch (fmt) {
+	case PASID_TABLE_ARM_SMMU_V3:
+		ret = viommu_prepare_arm_pst(vdev, pst_cfg);
+		break;
+	default:
+		dev_err(vdev->dev, "unsupported PASID table format 0x%x\n", fmt);
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int viommu_attach_pasid_table(struct viommu_endpoint *vdev,
+				     struct viommu_domain *vdomain)
+{
+	int ret;
+	int i, eid;
+	enum pasid_table_fmt fmt = -1;
+	struct virtio_iommu_probe_table_format *desc = vdev->pstf;
+	struct virtio_iommu_req_attach_table req = {
+		.head.type	= VIRTIO_IOMMU_T_ATTACH_TABLE,
+		.domain		= cpu_to_le32(vdomain->id),
+	};
+	struct viommu_dev *viommu = vdev->viommu;
+	struct iommu_pasid_table *tbl;
+	struct iommu_vendor_psdtable_cfg *pst_cfg;
+
+	if (!viommu->has_table)
+		return 0;
+
+	if (!desc)
+		return -ENODEV;
+
+	/* Prepare PASID tables configuration */
+	switch (le16_to_cpu(desc->format)) {
+	case VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3:
+		fmt = PASID_TABLE_ARM_SMMU_V3;
+		break;
+	default:
+		dev_err(vdev->dev, "unsupported PASID table format 0x%x\n",
+			le16_to_cpu(desc->format));
+		return 0;
+	}
+
+	if (!tbl) {
+		tbl = iommu_register_pasid_table(fmt, viommu->dev->parent, vdomain);
+		if (!tbl)
+			return -ENOMEM;
+
+		vdomain->pasid_tbl = tbl;
+		pst_cfg = &tbl->cfg;
+
+		pst_cfg->iommu_dev = viommu->dev->parent;
+
+		/* Prepare PASID tables info to allocate a new table */
+		ret = viommu_prepare_pst(vdev, pst_cfg, fmt);
+		if (ret)
+			return ret;
+
+		ret = iommu_psdtable_alloc(tbl, pst_cfg);
+		if (ret)
+			return ret;
+
+		pst_cfg->iommu_dev = viommu->dev->parent;

Already set by iommu_register_pasid_table() (and needed for DMA
allocations in iommu_psdtable_alloc())

+		pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;

Already set above

This cannot work at the moment because viommu_setup_pgtable() writes to
the non-existing pasid table. Probably best to leave this call for next
patch.

Thanks,
Jean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel