From: Vivek Kumar Gautam <vivek.gautam@arm.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: kevin.tian@intel.com, alex.williamson@redhat.com, mst@redhat.com,
will.deacon@arm.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
iommu@lists.linux-foundation.org, robin.murphy@arm.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available
Date: Fri, 12 Mar 2021 18:59:17 +0530 [thread overview]
Message-ID: <ee88590b-513e-7821-ab52-18d496ad90dc@arm.com> (raw)
In-Reply-To: <YD/GeIJA/ARevO7X@myrica>
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
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-03-12 13:29 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-15 12:13 [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 01/15] iommu/arm-smmu-v3: Create a Context Descriptor library Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 02/15] iommu: Add a simple PASID table library Vivek Gautam
2021-03-03 17:11 ` Jean-Philippe Brucker
2021-03-12 12:47 ` Vivek Kumar Gautam
2021-03-29 16:25 ` Jean-Philippe Brucker
2021-01-15 12:13 ` [PATCH RFC v1 03/15] iommu/arm-smmu-v3: Update drivers to work with iommu-pasid-table Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 04/15] iommu/arm-smmu-v3: Update CD base address info for user-space Vivek Gautam
2021-03-03 17:14 ` Jean-Philippe Brucker
2021-03-12 12:31 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 05/15] iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib Vivek Gautam
2021-03-03 17:15 ` Jean-Philippe Brucker
2021-03-12 12:49 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 06/15] iommu/virtio: Add headers for table format probing Vivek Gautam
2021-03-03 17:17 ` Jean-Philippe Brucker
2021-03-12 12:54 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 07/15] iommu/virtio: Add " Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 08/15] iommu: Add asid_bits to arm smmu-v3 stage1 table info Vivek Gautam
2021-03-03 17:18 ` Jean-Philippe Brucker
2021-03-12 12:57 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 09/15] iommu/virtio: Update table format probing header Vivek Gautam
2021-03-03 17:21 ` Jean-Philippe Brucker
2021-03-12 12:58 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 10/15] iommu/virtio: Prepare to add attach pasid table infrastructure Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 11/15] iommu/virtio: Add headers for binding pasid table in iommu Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 12/15] iommu/virtio: Add support for INVALIDATE request Vivek Gautam
2021-03-03 18:28 ` Jacob Pan
2021-03-04 5:58 ` Tian, Kevin
2021-03-04 6:16 ` Vivek Kumar Gautam
2021-01-15 12:13 ` [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available Vivek Gautam
2021-03-03 17:25 ` Jean-Philippe Brucker
2021-03-12 13:29 ` Vivek Kumar Gautam [this message]
2021-03-29 16:21 ` Jean-Philippe Brucker
2021-01-15 12:13 ` [PATCH RFC v1 14/15] iommu/virtio: Add support for Arm LPAE page table format Vivek Gautam
2021-01-15 12:13 ` [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault Vivek Gautam
2021-03-03 17:25 ` Jean-Philippe Brucker
2021-03-12 13:09 ` Vivek Kumar Gautam
2021-03-29 16:23 ` Jean-Philippe Brucker
2021-04-06 6:24 ` Vivek Kumar Gautam
2021-01-19 9:03 ` [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm Auger Eric
2021-01-21 17:34 ` Vivek Kumar Gautam
2021-01-22 15:49 ` Shameerali Kolothum Thodi
2021-01-25 12:55 ` Vivek Kumar Gautam
2021-01-25 8:43 ` Auger Eric
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ee88590b-513e-7821-ab52-18d496ad90dc@arm.com \
--to=vivek.gautam@arm.com \
--cc=alex.williamson@redhat.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jean-philippe@linaro.org \
--cc=kevin.tian@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=robin.murphy@arm.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).