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
> +
> + 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.
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
>
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
>>
On Fri, Mar 12, 2021 at 06:59:17PM +0530, Vivek Kumar Gautam wrote:
> > > + /* 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?
Something like that, but I'd keep separate arch-specific structs. Vt-d
reports the capability registers directly through iommu_nesting_info [2].
We could do the same for Arm, copy sanitized values of IDR0..5 into
struct iommu_nesting_info_arm_smmuv3.
I've avoided doing that for virtio-iommu because every field needs a
description in the spec. So where possible I used generic properties that
apply to any architecture, such as page, PASID and address size. What's
left is the minimum arch-specific information to get nested translation
going, leaving out a lot of properties such as big-endian and 32-bit,
which can be added later if needed. The Arm specific properties are split
into page table and pasid table information. Page table info should work
for both SMMUv2 and v3 (where they correspond to an SMMU_IDRx field that
constrains a context descriptor field.) I should move BTM in there since
it's supported by SMMUv2.
Thanks,
Jean
[2] https://lore.kernel.org/linux-iommu/[email protected]/