On 3/3/21 10:55 PM, Jean-Philippe Brucker wrote:
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, will amend it.
+ + /* 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()
Right, thanks for catching.
+ + 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.
Yea, this isn't right. The asid should be assigned to the one that we are allocating. I think this is getting over-written when iommu_psdtable_prepare() calls into arm_smmu_prepare_cd() where the correct asid value is assigned. I will remove this.
+ + pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;Parent function can set this
Sure.
+ /* 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.
Regarding this Eric pointed out [1] in my other patch about the scalability of the approach where we keep adding flags in 'iommu_nesting_info' corresponding to the arm-smmu-v3 capabilities. I guess the same goes to these flags in virtio. May be the 'iommu_nesting_info' can have a bitmap with the caps for vendor specific features, and here we can add the related flags?
+ + 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())
Right.
+ pst_cfg->fmt = PASID_TABLE_ARM_SMMU_V3;Already set above
Right.
+ + ret = viommu_setup_pgtable(vdev, vdomain); + if (ret) { + dev_err(vdev->dev, "could not install page tables\n"); + goto err_free_psdtable; + } + + /* Add arch-specific configuration */ + ret = viommu_config_pst(pst_cfg, (void *)&req, fmt); + if (ret) + goto err_free_ops; + + vdev_for_each_id(i, eid, vdev) { + req.endpoint = cpu_to_le32(eid); + ret = viommu_send_req_sync(viommu, &req, sizeof(req)); + if (ret) + goto err_free_ops; + } + } else { + /* TODO: otherwise, check for compatibility with vdev. */ + return -ENOSYS; + } + + dev_dbg(vdev->dev, "uses PASID table format 0x%x\n", fmt); + + return 0; + +err_free_ops: + if (vdomain->mm.ops) + viommu_teardown_pgtable(vdomain); +err_free_psdtable: + iommu_psdtable_free(tbl, &tbl->cfg); + + return ret; +} + static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) { int ret = 0; @@ -928,6 +1213,17 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) if (vdev->vdomain) vdev->vdomain->nr_endpoints--;+ ret = viommu_attach_pasid_table(vdev, vdomain);+ if (ret) { + /* + * No PASID support, too bad. Perhaps we can bind a single set + * of page tables? + */ + ret = viommu_setup_pgtable(vdev, vdomain);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.
Yea, will move it to the next patch. Thanks & regards Vivek
Thanks, Jean+ if (ret) + dev_err(vdev->dev, "could not install tables\n"); + } + if (!vdomain->mm.ops) { /* If we couldn't bind any table, use the mapping tree */ ret = viommu_simple_attach(vdomain, vdev); @@ -948,6 +1244,10 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova, u32 flags; struct virtio_iommu_req_map map; struct viommu_domain *vdomain = to_viommu_domain(domain); + struct io_pgtable_ops *ops = vdomain->mm.ops; + + if (ops) + return ops->map(ops, iova, paddr, size, prot, gfp);flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |(prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) | @@ -986,6 +1286,10 @@ static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t unmapped; struct virtio_iommu_req_unmap unmap; struct viommu_domain *vdomain = to_viommu_domain(domain); + struct io_pgtable_ops *ops = vdomain->mm.ops; + + if (ops) + return ops->unmap(ops, iova, size, gather);unmapped = viommu_del_mappings(vdomain, iova, size);if (unmapped < size) @@ -1014,6 +1318,10 @@ static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain, struct viommu_mapping *mapping; struct interval_tree_node *node; struct viommu_domain *vdomain = to_viommu_domain(domain); + struct io_pgtable_ops *ops = vdomain->mm.ops; + + if (ops) + return ops->iova_to_phys(ops, iova);spin_lock_irqsave(&vdomain->mappings_lock, flags);node = interval_tree_iter_first(&vdomain->mappings, iova, iova); @@ -1264,7 +1572,12 @@ static int viommu_probe(struct virtio_device *vdev) struct virtio_iommu_config, probe_size, &viommu->probe_size);+ viommu->has_table = virtio_has_feature(vdev, VIRTIO_IOMMU_F_ATTACH_TABLE);viommu->has_map = virtio_has_feature(vdev, VIRTIO_IOMMU_F_MAP_UNMAP); + if (!viommu->has_table && !viommu->has_map) { + ret = -EINVAL; + goto err_free_vqs; + }viommu->geometry = (struct iommu_domain_geometry) {.aperture_start = input_start, @@ -1356,6 +1669,7 @@ static unsigned int features[] = { VIRTIO_IOMMU_F_DOMAIN_RANGE, VIRTIO_IOMMU_F_PROBE, VIRTIO_IOMMU_F_MMIO, + VIRTIO_IOMMU_F_ATTACH_TABLE, };static struct virtio_device_id id_table[] = {-- 2.17.1
