Messages in this thread | | | Subject | Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available | From | Vivek Kumar Gautam <> | Date | Fri, 12 Mar 2021 18:59:17 +0530 |
| |
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 >>
| |