Re: [PATCH RFC v1 03/11] iommu/virtio: Handle incoming page faults

2021-10-11 Thread Vivek Kumar Gautam

Hi Jean,


On 10/11/21 2:46 PM, Jean-Philippe Brucker wrote:

Hi Vivek,

On Mon, Oct 11, 2021 at 01:41:15PM +0530, Vivek Gautam wrote:

+ list_for_each_entry(ep, >endpoints, list) {
+ if (ep->eid == endpoint) {
+ vdev = ep->vdev;


I have a question here though -
Is endpoint-ID unique across all the endpoints available per 'viommu_dev' or
per 'viommu_domain'?
If it is per 'viommu_domain' then the above list is also incorrect.
As you pointed to in the patch [1] -
[PATCH RFC v1 02/11] iommu/virtio: Maintain a list of endpoints served
by viommu_dev
I am planning to add endpoint ID into a static global xarray in
viommu_probe_device() as below:

 vdev_for_each_id(i, eid, vdev) {
 ret = xa_insert(_ep_ids, eid, vdev, GFP_KERNEL);
 if (ret)
 goto err_free_dev;
 }

and replace the above list traversal as below:

 xa_lock_irqsave(_ep_ids, flags);
 xa_for_each(_ep_ids, eid, vdev) {
 if (eid == endpoint) {
 ret =
iommu_report_device_fault(vdev->dev, _evt);
 if (ret)
 dev_err(vdev->dev, "Couldn't
handle page request\n");
 }
 }
 xa_unlock_irqrestore(_ep_ids, flags);

But using a global xarray would also be incorrect if the endpointsID are global
across 'viommu_domain'.

I need to find the correct 'viommu_endpoint' to call iommu_report_device_fault()
with the correct device.


The endpoint IDs are only unique across viommu_dev, so a global xarray
wouldn't work but one in viommu_dev would. In vdomain it doesn't work
either because we can't get to the domain from the fault handler without
first finding the endpoint


Thanks. That's easy then. Will have a xarray in viommu_dev and iterate 
over it from the fault handler.


Best regards
Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information

2021-09-30 Thread Vivek Kumar Gautam




On 9/30/21 2:19 PM, Jean-Philippe Brucker wrote:

On Thu, Sep 30, 2021 at 10:26:35AM +0530, Vivek Kumar Gautam wrote:

I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
seems more than enough.


Right, I will update this to 16-bits field. It won't be okay to update the
iommu uAPI now, right?


Since there is no UAPI transport for the fault request/response at the
moment, it should be possible to update it.


Righty! I will add a patch for that too then.



Thanks & regards

Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 08/11] iommu/arm-smmu-v3: Implement shared context alloc and free ops

2021-09-30 Thread Vivek Kumar Gautam

Hi Jean,


On 9/21/21 9:37 PM, Jean-Philippe Brucker wrote:

On Fri, Apr 23, 2021 at 03:21:44PM +0530, Vivek Gautam wrote:

Implementing the alloc_shared_cd and free_shared_cd in cd-lib, and
start using them for arm-smmu-v3-sva implementation.

Signed-off-by: Vivek Gautam 
---
  .../arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c  | 71 
  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 83 ---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 14 
  4 files changed, 73 insertions(+), 96 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
index 537b7c784d40..b87829796596 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
@@ -285,16 +285,14 @@ static bool arm_smmu_free_asid(struct xarray *xa, void 
*cookie_cd)
   * descriptor is using it, try to replace it.
   */
  static struct arm_smmu_ctx_desc *
-arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
+arm_smmu_share_asid(struct iommu_pasid_table *tbl, struct mm_struct *mm,
+   struct xarray *xa, u16 asid, u32 asid_bits)


xa and asid_bits could be stored in some arch-specific section of the
iommu_pasid_table struct. Other table drivers wouldn't need those
arguments.


Okay, will move them to a separate structure section.



More a comment for the parent series: it may be clearer to give a
different prefix to functions in this file (arm_smmu_cd_, pst_arm_?).
Reading this patch I'm a little confused by what belongs in the IOMMU
driver and what is done by this library. (I also keep reading 'tbl' as
'tlb'. Maybe we could make it 'table' since that doesn't take a lot more
space)


Yea, this may be confusing. I will fix these namings in my next version.




  {
int ret;
u32 new_asid;
struct arm_smmu_ctx_desc *cd;
-   struct arm_smmu_device *smmu;
-   struct arm_smmu_domain *smmu_domain;
-   struct iommu_pasid_table *tbl;
  
-	cd = xa_load(_smmu_asid_xa, asid);

+   cd = xa_load(xa, asid);
if (!cd)
return NULL;
  
@@ -306,12 +304,8 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)

return cd;
}
  
-	smmu_domain = container_of(cd, struct arm_smmu_domain, s1_cfg.cd);

-   smmu = smmu_domain->smmu;
-   tbl = smmu_domain->tbl;
-
-   ret = xa_alloc(_smmu_asid_xa, _asid, cd,
-  XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL);
+   ret = xa_alloc(xa, _asid, cd, XA_LIMIT(1, (1 << asid_bits) - 1),
+  GFP_KERNEL);
if (ret)
return ERR_PTR(-ENOSPC);
/*
@@ -325,48 +319,52 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 * be some overlap between use of both ASIDs, until we invalidate the
 * TLB.
 */
-   ret = iommu_psdtable_write(tbl, >cfg, 0, cd);
+   ret = arm_smmu_write_ctx_desc(>cfg, 0, cd);
if (ret)
return ERR_PTR(-ENOSYS);
  
  	/* Invalidate TLB entries previously associated with that context */

-   iommu_psdtable_flush_tlb(tbl, smmu_domain, asid);
+   iommu_psdtable_flush_tlb(tbl, tbl->cookie, asid);
  
-	xa_erase(_smmu_asid_xa, asid);

+   xa_erase(xa, asid);
return NULL;
  }
  
-struct arm_smmu_ctx_desc *

-arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm)
+static struct iommu_psdtable_mmu_notifier *
+arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm,
+struct xarray *xa, u32 asid_bits)
  {
u16 asid;
int err = 0;
u64 tcr, par, reg;
struct arm_smmu_ctx_desc *cd;
struct arm_smmu_ctx_desc *ret = NULL;
+   struct iommu_psdtable_mmu_notifier *pst_mn;
  
  	asid = arm64_mm_context_get(mm);

if (!asid)
return ERR_PTR(-ESRCH);
  
+	pst_mn = kzalloc(sizeof(*pst_mn), GFP_KERNEL);

+   if (!pst_mn) {
+   err = -ENOMEM;
+   goto out_put_context;
+   }
+
cd = kzalloc(sizeof(*cd), GFP_KERNEL);
if (!cd) {
err = -ENOMEM;
-   goto out_put_context;
+   goto out_free_mn;
}
  
  	refcount_set(>refs, 1);
  
-	mutex_lock(_smmu_asid_lock);

-   ret = arm_smmu_share_asid(mm, asid);
+   ret = arm_smmu_share_asid(tbl, mm, xa, asid, asid_bits);
if (ret) {
-   mutex_unlock(_smmu_asid_lock);
goto out_free_cd;
}
  
-	err = xa_insert(_smmu_asid_xa, asid, cd, GFP_KERNEL);

-   mutex_unlock(_smmu_asid_lock);
-
+   err = xa_insert(xa, asid, cd, GFP_KERNEL);
if (err)
goto out_free_asid;
  
@@ -406,21 +404,26 @@ arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm)

cd->asid = asid;
cd->mm = mm;
  
-	return cd;

+   

Re: [PATCH RFC v1 10/11] uapi/virtio-iommu: Add a new request type to send page response

2021-09-30 Thread Vivek Kumar Gautam

Hi Jean,


On 9/21/21 9:46 PM, Jean-Philippe Brucker wrote:

On Fri, Apr 23, 2021 at 03:21:46PM +0530, Vivek Gautam wrote:

Once the page faults are handled, the response has to be sent to
virtio-iommu backend, from where it can be sent to the host to
prepare the response to a generated io page fault by the device.
Add a new virt-queue request type to handle this.

Signed-off-by: Vivek Gautam 
---
  include/uapi/linux/virtio_iommu.h | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/include/uapi/linux/virtio_iommu.h 
b/include/uapi/linux/virtio_iommu.h
index c12d9b6a7243..1b174b98663a 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -48,6 +48,7 @@ struct virtio_iommu_config {
  #define VIRTIO_IOMMU_T_PROBE  0x05
  #define VIRTIO_IOMMU_T_ATTACH_TABLE   0x06
  #define VIRTIO_IOMMU_T_INVALIDATE 0x07
+#define VIRTIO_IOMMU_T_PAGE_RESP   0x08
  
  /* Status types */

  #define VIRTIO_IOMMU_S_OK 0x00
@@ -70,6 +71,23 @@ struct virtio_iommu_req_tail {
__u8reserved[3];
  };
  
+struct virtio_iommu_req_page_resp {

+   struct virtio_iommu_req_headhead;
+   __le32  domain;


I don't think we need this field, since the fault report doesn't come with
a domain.


But here we are sending the response which would be consumed by the vfio 
ultimately. In kvmtool, I am consuming this "virtio_iommu_req_page_resp" 
request in the virtio/iommu driver, extracting the domain from it, and 
using that to call the respective "page_response" ops from 
"vfio_iommu_ops" in the vfio/core driver.


Is this incorrect way of passing on the page-response back to the host 
kernel?


But I think this will have to be worked out with the /dev/iommu framework.




+   __le32  endpoint;
+#define VIRTIO_IOMMU_PAGE_RESP_PASID_VALID (1 << 0)


To be consistent with the rest of the document this would be
VIRTIO_IOMMU_PAGE_RESP_F_PASID_VALID


Sure, will update this.




+   __le32  flags;
+   __le32  pasid;
+   __le32  grpid;
+#define VIRTIO_IOMMU_PAGE_RESP_SUCCESS (0x0)
+#define VIRTIO_IOMMU_PAGE_RESP_INVALID (0x1)
+#define VIRTIO_IOMMU_PAGE_RESP_FAILURE (0x2)
+   __le16  resp_code;
+   __u8pasid_valid;


This field isn't needed since there already is
VIRTIO_IOMMU_PAGE_RESP_PASID_VALID


Yes, sure will remove this field.




+   __u8reserved[9];
+   struct virtio_iommu_req_tailtail;
+};


I'd align the size of the struct to 16 bytes, but I don't think that's
strictly necessary.


Will fix this. Thanks a lot for reviewing.

Best regards
Vivek



Thanks,
Jean


+
  struct virtio_iommu_req_attach {
struct virtio_iommu_req_headhead;
__le32  domain;
--
2.17.1


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 02/11] iommu/virtio: Maintain a list of endpoints served by viommu_dev

2021-09-30 Thread Vivek Kumar Gautam




On 9/21/21 9:29 PM, Jean-Philippe Brucker wrote:

On Fri, Apr 23, 2021 at 03:21:38PM +0530, Vivek Gautam wrote:

Keeping a record of list of endpoints that are served by the virtio-iommu
device would help in redirecting the requests of page faults to the
correct endpoint device to handle such requests.

Signed-off-by: Vivek Gautam 
---
  drivers/iommu/virtio-iommu.c | 21 +
  1 file changed, 21 insertions(+)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 50039070e2aa..c970f386f031 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -48,6 +48,7 @@ struct viommu_dev {
spinlock_t  request_lock;
struct list_headrequests;
void*evts;
+   struct list_headendpoints;


As we're going to search by ID, an xarray or rb_tree would be more
appropriate than a list


Sure, I will update this with a rb_tree.



  
  	/* Device configuration */

struct iommu_domain_geometrygeometry;
@@ -115,6 +116,12 @@ struct viommu_endpoint {
void*pgtf;
  };
  
+struct viommu_ep_entry {

+   u32 eid;
+   struct viommu_endpoint  *vdev;
+   struct list_headlist;
+};


No need for a separate struct, I think you can just add the list head and
id into viommu_endpoint.


Yea right. I will update it.




+
  struct viommu_request {
struct list_headlist;
void*writeback;
@@ -573,6 +580,7 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, 
struct device *dev)
size_t probe_len;
struct virtio_iommu_req_probe *probe;
struct virtio_iommu_probe_property *prop;
+   struct viommu_ep_entry *ep;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
  
@@ -640,6 +648,18 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)

prop = (void *)probe->properties + cur;
type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
}
+   if (ret)
+   goto out_free;
+
+   ep = kzalloc(sizeof(*ep), GFP_KERNEL);
+   if (!ep) {
+   ret = -ENOMEM;
+   goto out_free;
+   }
+   ep->eid = probe->endpoint;
+   ep->vdev = vdev;
+
+   list_add(>list, >endpoints);


This should be in viommu_probe_device() (viommu_probe_endpoint() is only
called if F_PROBE is negotiated). I think we need a lock for this
list/xarray


Sure, will fix this, and add the needed locking around.

Thanks & regards
Vivek



Thanks,
Jean

  
  out_free:

kfree(probe);
@@ -1649,6 +1669,7 @@ static int viommu_probe(struct virtio_device *vdev)
viommu->dev = dev;
viommu->vdev = vdev;
INIT_LIST_HEAD(>requests);
+   INIT_LIST_HEAD(>endpoints);
  
  	ret = viommu_init_vqs(viommu);

if (ret)
--
2.17.1


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information

2021-09-29 Thread Vivek Kumar Gautam

Hi Jean,


On 9/21/21 9:28 PM, Jean-Philippe Brucker wrote:

Hi Vivek,

Thanks a lot for your work on this


Thanks a lot for taking a look at it. I hope that most of the changes in 
this series and the nested page table series [1] are independent of the 
presently on-going /dev/iommu proposal, and can be separately reviewed.


Please find my comments inline below.



On Fri, Apr 23, 2021 at 03:21:37PM +0530, Vivek Gautam wrote:

Add fault information for group-id and necessary flags for page
request faults that can be handled by page fault handler in
virtio-iommu driver.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  include/uapi/linux/virtio_iommu.h | 13 +
  1 file changed, 13 insertions(+)

diff --git a/include/uapi/linux/virtio_iommu.h 
b/include/uapi/linux/virtio_iommu.h
index f8bf927a0689..accc3318ce46 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -307,14 +307,27 @@ struct virtio_iommu_req_invalidate {
  #define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV  1
  #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ 2
  
+#define VIRTIO_IOMMU_FAULT_PRQ_F_PASID_VALID		(1 << 0)

+#define VIRTIO_IOMMU_FAULT_PRQ_F_LAST_PAGE (1 << 1)
+#define VIRTIO_IOMMU_FAULT_PRQ_F_PRIV_DATA (1 << 2)
+#define VIRTIO_IOMMU_FAULT_PRQ_F_NEEDS_PASID   (1 << 3)


I don't think this one is necessary here. The NEEDS_PASID flags added by
commit 970471914c67 ("iommu: Allow page responses without PASID") mainly
helps Linux keep track of things internally. It does tell the fault
handler whether to reply with PASID or not, but we don't need that here.
The virtio-iommu driver knows whether a PASID is required by looking at
the "PRG Response PASID Required" bit in the PCIe capability. For non-PCIe
faults (e.g. SMMU stall), I'm guessing we'll need a PROBE property to
declare that the endpoint supports recoverable faults anyway, so "PASID
required in response" can go through there as well.


Sure, I will remove this flag, and rather read the PCIe cap to find out 
if PASID is required or not. After this series, I will follow up with 
the non-PCIe fault handling.





+
+#define VIRTIO_IOMMU_FAULT_UNREC_F_PASID_VALID (1 << 0)
+#define VIRTIO_IOMMU_FAULT_UNREC_F_ADDR_VALID  (1 << 1)
+#define VIRTIO_IOMMU_FAULT_UNREC_F_FETCH_ADDR_VALID(1 << 2)
+
  struct virtio_iommu_fault {
__u8reason;
__u8reserved[3];
__le16  flt_type;
__u8reserved2[2];
+   /* flags is actually permission flags */


It's also used for declaring validity of fields.
VIRTIO_IOMMU_FAULT_F_ADDRESS already tells whether the address field is
valid, so all the other flags introduced by this patch can go in here.


Sure, will remove pr_evt_flags field, and move all the flags to 'flags'.




__le32  flags;
+   /* flags for PASID and Page request handling info */
+   __le32  pr_evt_flags;
__le32  endpoint;
__le32  pasid;
+   __le32  grpid;


I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
seems more than enough.


Right, I will update this to 16-bits field. It won't be okay to update 
the iommu uAPI now, right?




New fields must be appended at the end of the struct, because old drivers
will expect to find the 'endpoint' field at this offset. You could remove
'reserved3' while adding 'grpid', to keep the struct layout.


Sure, will update this.




__u8reserved3[4];
__le64  address;
__u8reserved4[8];



So the base structure, currently in the spec, looks like this:

struct virtio_iommu_fault {
u8   reason;
u8   reserved[3];
le32 flags;
le32 endpoint;
le32 reserved1;
le64 address;
};

#define VIRTIO_IOMMU_FAULT_F_READ   (1 << 0)
#define VIRTIO_IOMMU_FAULT_F_WRITE  (1 << 1)
#define VIRTIO_IOMMU_FAULT_F_ADDRESS(1 << 8)

The extended struct could be:

struct virtio_iommu_fault {
u8   reason;
u8   reserved[3];
le32 flags;
le32 endpoint;
le32 pasid;
le64 address;
/* Page 

Re: [PATCH v15 00/12] SMMUv3 Nested Stage Setup (IOMMU part)

2021-04-21 Thread Vivek Kumar Gautam

Hi Eric,

On 4/11/21 4:42 PM, Eric Auger wrote:

SMMUv3 Nested Stage Setup (IOMMU part)



[snip]



Eric Auger (12):
   iommu: Introduce attach/detach_pasid_table API
   iommu: Introduce bind/unbind_guest_msi
   iommu/smmuv3: Allow s1 and s2 configs to coexist
   iommu/smmuv3: Get prepared for nested stage support
   iommu/smmuv3: Implement attach/detach_pasid_table
   iommu/smmuv3: Allow stage 1 invalidation with unmanaged ASIDs
   iommu/smmuv3: Implement cache_invalidate
   dma-iommu: Implement NESTED_MSI cookie
   iommu/smmuv3: Nested mode single MSI doorbell per domain enforcement
   iommu/smmuv3: Enforce incompatibility between nested mode and HW MSI
 regions
   iommu/smmuv3: Implement bind/unbind_guest_msi
   iommu/smmuv3: report additional recoverable faults


[snip]

I noticed that the patch[1]:
[PATCH v13 15/15] iommu/smmuv3: Add PASID cache invalidation per PASID
has been dropped in the v14 and v15 of
 this series.

Is this planned to be part of any future series, or did I miss a
discussion about dropping the patch? :-)


[1]
https://patchwork.kernel.org/project/kvm/patch/20201118112151.25412-16-eric.au...@redhat.com/


Best regards
Vivek
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault

2021-04-06 Thread Vivek Kumar Gautam




On 3/29/21 9:53 PM, Jean-Philippe Brucker wrote:

On Fri, Mar 12, 2021 at 06:39:05PM +0530, Vivek Kumar Gautam wrote:

To complete the page request we would also need to send the response back to
the host from virtio backend when handling page request. So the virtio
command should also be accompanied with a vfio api to send the page request
response back to the host. Isn't it?
This is where the host smmuv3 can send PRI_RESP command to the device to
complete the page fault.


It looks like Eric already has this in the VFIO series:
https://lore.kernel.org/linux-iommu/20210223210625.604517-14-eric.au...@redhat.com/


Right, I have taken this change to work on getting the vSVA with 
virtio-iommu.
For this I am adding a new request for virtio-iomm - 
VIRTIO_IOMMU_T_PAGE_RESP, and related struct virtio_iommu_req_page_resp 
that would contain information such as, pasid, grpid, response_code, 
flags, and endpoint. This is inline with struct iommu_page_response.

I will post out the patches for this soon.

Thanks & regards
Vivek



Thanks,
Jean


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available

2021-03-12 Thread Vivek Kumar Gautam




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= _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, , >mm);
+   if (!ops)
+   return -ENOMEM;
+
+   pst_cfg = >cfg;
+   cfgi = _cfg->vendor.cfg;
+   id = ida_simple_get(_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, , 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, >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(_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 = _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 = 

Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault

2021-03-12 Thread Vivek Kumar Gautam




On 3/3/21 10:55 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:42PM +0530, Vivek Gautam wrote:

Fault type information can tell about a page request fault or
an unreceoverable fault, and further additions to fault reasons
and the related PASID information can help in handling faults
efficiently.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Michael S. Tsirkin 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  drivers/iommu/virtio-iommu.c  | 27 +--
  include/uapi/linux/virtio_iommu.h | 13 -
  2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 9cc3d35125e9..10ef9e98214a 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -652,9 +652,16 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
char *reason_str;
  
  	u8 reason	= fault->reason;

+   u16 type= fault->flt_type;
u32 flags   = le32_to_cpu(fault->flags);
u32 endpoint= le32_to_cpu(fault->endpoint);
u64 address = le64_to_cpu(fault->address);
+   u32 pasid   = le32_to_cpu(fault->pasid);
+
+   if (type == VIRTIO_IOMMU_FAULT_F_PAGE_REQ) {
+   dev_info(viommu->dev, "Page request fault - unhandled\n");
+   return 0;
+   }
  
  	switch (reason) {

case VIRTIO_IOMMU_FAULT_R_DOMAIN:
@@ -663,6 +670,21 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
case VIRTIO_IOMMU_FAULT_R_MAPPING:
reason_str = "page";
break;
+   case VIRTIO_IOMMU_FAULT_R_WALK_EABT:
+   reason_str = "page walk external abort";
+   break;
+   case VIRTIO_IOMMU_FAULT_R_PTE_FETCH:
+   reason_str = "pte fetch";
+   break;
+   case VIRTIO_IOMMU_FAULT_R_PERMISSION:
+   reason_str = "permission";
+   break;
+   case VIRTIO_IOMMU_FAULT_R_ACCESS:
+   reason_str = "access";
+   break;
+   case VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS:
+   reason_str = "output address";
+   break;
case VIRTIO_IOMMU_FAULT_R_UNKNOWN:
default:
reason_str = "unknown";
@@ -671,8 +693,9 @@ static int viommu_fault_handler(struct viommu_dev *viommu,
  
  	/* TODO: find EP by ID and report_iommu_fault */

if (flags & VIRTIO_IOMMU_FAULT_F_ADDRESS)
-   dev_err_ratelimited(viommu->dev, "%s fault from EP %u at %#llx 
[%s%s%s]\n",
-   reason_str, endpoint, address,
+   dev_err_ratelimited(viommu->dev,
+   "%s fault from EP %u PASID %u at %#llx 
[%s%s%s]\n",
+   reason_str, endpoint, pasid, address,
flags & VIRTIO_IOMMU_FAULT_F_READ ? "R" : 
"",
flags & VIRTIO_IOMMU_FAULT_F_WRITE ? "W" : 
"",
flags & VIRTIO_IOMMU_FAULT_F_EXEC ? "X" : 
"");
diff --git a/include/uapi/linux/virtio_iommu.h 
b/include/uapi/linux/virtio_iommu.h
index 608c8d642e1f..a537d82777f7 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -290,19 +290,30 @@ struct virtio_iommu_req_invalidate {
  #define VIRTIO_IOMMU_FAULT_R_UNKNOWN  0
  #define VIRTIO_IOMMU_FAULT_R_DOMAIN   1
  #define VIRTIO_IOMMU_FAULT_R_MAPPING  2
+#define VIRTIO_IOMMU_FAULT_R_WALK_EABT 3
+#define VIRTIO_IOMMU_FAULT_R_PTE_FETCH 4
+#define VIRTIO_IOMMU_FAULT_R_PERMISSION5
+#define VIRTIO_IOMMU_FAULT_R_ACCESS6
+#define VIRTIO_IOMMU_FAULT_R_OOR_ADDRESS   7
  
  #define VIRTIO_IOMMU_FAULT_F_READ		(1 << 0)

  #define VIRTIO_IOMMU_FAULT_F_WRITE(1 << 1)
  #define VIRTIO_IOMMU_FAULT_F_EXEC (1 << 2)
  #define VIRTIO_IOMMU_FAULT_F_ADDRESS  (1 << 8)
  
+#define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV	1

+#define VIRTIO_IOMMU_FAULT_F_PAGE_REQ  2


Currently all reported faults are unrecoverable, so to be consistent
DMA_UNRECOV should be 0. But I'd prefer having just a new "page request"
flag in the flags field, instead of the flt_type field.


Yea, looking at what I am currently trying as well - handle page-request 
and leave all other faults as unrecoverable - I will add the page 
request flag in the structure.




For page requests we'll also need a 16-bit fault ID field to store the PRI
"page request group index" or the stall "stag". "last" and "privileged"
flags as well, to match the PRI page request. And a new command to
complete a page fault.


Right, will add the fields as suggested.
To complete the page request we would also need to send the response 
back to the host from virtio backend 

Re: [PATCH RFC v1 09/15] iommu/virtio: Update table format probing header

2021-03-12 Thread Vivek Kumar Gautam




On 3/3/21 10:51 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:36PM +0530, Vivek Gautam wrote:

Add info about asid_bits and additional flags to table format
probing header.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Michael S. Tsirkin 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  include/uapi/linux/virtio_iommu.h | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_iommu.h 
b/include/uapi/linux/virtio_iommu.h
index 43821e33e7af..8a0624bab4b2 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -169,7 +169,10 @@ struct virtio_iommu_probe_pasid_size {
  struct virtio_iommu_probe_table_format {
struct virtio_iommu_probe_property  head;
__le16  format;
-   __u8reserved[2];
+   __le16  asid_bits;
+
+   __le32  flags;


This struct should only contain the head and format fields. asid and flags
should go in a specialized structure - virtio_iommu_probe_pgt_arm64 in the
latest spec draft, where I dropped the asid_bits field in favor of an
"ASID16" flag.


Right, will take care of this looking at the spec draft.

Best regards
Vivek



Thanks,
Jean


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 08/15] iommu: Add asid_bits to arm smmu-v3 stage1 table info

2021-03-12 Thread Vivek Kumar Gautam




On 3/3/21 10:48 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:35PM +0530, Vivek Gautam wrote:

aisd_bits data is required to prepare stage-1 tables for arm-smmu-v3.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  include/uapi/linux/iommu.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 082d758dd016..96abbfc7c643 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -357,7 +357,7 @@ struct iommu_pasid_smmuv3 {
__u32   version;
__u8s1fmt;
__u8s1dss;
-   __u8padding[2];
+   __u16   asid_bits;


Is this used anywhere?  This struct is passed from host userspace to host
kernel to attach the PASID table, so I don't think it needs an asid_bits
field.


Yea, must have missed removing it from the WIP work. Will remove it.

Thanks
Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 06/15] iommu/virtio: Add headers for table format probing

2021-03-12 Thread Vivek Kumar Gautam




On 3/3/21 10:47 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:33PM +0530, Vivek Gautam wrote:

From: Jean-Philippe Brucker 

Add required UAPI defines for probing table format for underlying
iommu hardware. The device may provide information about hardware
tables and additional capabilities for each device.
This allows guest to correctly fabricate stage-1 page tables.

Signed-off-by: Jean-Philippe Brucker 
[Vivek: Use a single "struct virtio_iommu_probe_table_format" rather
 than separate structures for page table and pasid table format.


Makes sense. I've integrated that into the spec draft, added more precise
documentation and modified some of the definitions.

The current draft is here:
https://jpbrucker.net/virtio-iommu/spec/virtio-iommu-v0.13.pdf
Posted on the list here
https://lists.oasis-open.org/archives/virtio-dev/202102/msg00012.html


Thanks, I took an initial look, will review it this week.




Also update commit message.]
Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Michael S. Tsirkin 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  include/uapi/linux/virtio_iommu.h | 44 ++-
  1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_iommu.h 
b/include/uapi/linux/virtio_iommu.h
index 237e36a280cb..43821e33e7af 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -2,7 +2,7 @@
  /*
   * Virtio-iommu definition v0.12
   *
- * Copyright (C) 2019 Arm Ltd.
+ * Copyright (C) 2019-2021 Arm Ltd.


Not strictly necessary. But if you're modifying this comment please also
remove the "v0.12" above


Sure, let me keep the copyright year unchanged until we finalize the 
changes in draft spec.





   */
  #ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
  #define _UAPI_LINUX_VIRTIO_IOMMU_H
@@ -111,6 +111,12 @@ struct virtio_iommu_req_unmap {
  
  #define VIRTIO_IOMMU_PROBE_T_NONE		0

  #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
+#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK2
+#define VIRTIO_IOMMU_PROBE_T_INPUT_RANGE   3
+#define VIRTIO_IOMMU_PROBE_T_OUTPUT_SIZE   4
+#define VIRTIO_IOMMU_PROBE_T_PASID_SIZE5
+#define VIRTIO_IOMMU_PROBE_T_PAGE_TABLE_FMT6
+#define VIRTIO_IOMMU_PROBE_T_PASID_TABLE_FMT   7


Since there is a single struct we can have a single
VIRTIO_IOMMU_PROBE_T_TABLE_FORMAT.


Right, that would make sense.



  
  #define VIRTIO_IOMMU_PROBE_T_MASK		0xfff
  
@@ -130,6 +136,42 @@ struct virtio_iommu_probe_resv_mem {

__le64  end;
  };
  
+struct virtio_iommu_probe_page_size_mask {

+   struct virtio_iommu_probe_property  head;
+   __u8reserved[4];
+   __le64  mask;
+};
+
+struct virtio_iommu_probe_input_range {
+   struct virtio_iommu_probe_property  head;
+   __u8reserved[4];
+   __le64  start;
+   __le64  end;
+};
+
+struct virtio_iommu_probe_output_size {
+   struct virtio_iommu_probe_property  head;
+   __u8bits;
+   __u8reserved[3];
+};
+
+struct virtio_iommu_probe_pasid_size {
+   struct virtio_iommu_probe_property  head;
+   __u8bits;
+   __u8reserved[3];
+};
+
+/* Arm LPAE page table format */
+#define VIRTIO_IOMMU_FOMRAT_PGTF_ARM_LPAE  1


s/FOMRAT/FORMAT


Sure.




+/* Arm smmu-v3 type PASID table format */
+#define VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3   2


These should be with the Arm-specific definitions patches 11 and 14


Right, will add these definitions with Arm specific patches.

Best regards
Vivek

[snip]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 05/15] iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib

2021-03-12 Thread Vivek Kumar Gautam




On 3/3/21 10:45 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:32PM +0530, Vivek Gautam wrote:

Te change allows different consumers of arm-smmu-v3-cd-lib to set
their respective sync op for pasid entries.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c | 1 -
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c| 7 +++
  2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
index ec37476c8d09..acaa09acecdd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
@@ -265,7 +265,6 @@ struct iommu_vendor_psdtable_ops arm_cd_table_ops = {
.free= arm_smmu_free_cd_tables,
.prepare = arm_smmu_prepare_cd,
.write   = arm_smmu_write_ctx_desc,
-   .sync= arm_smmu_sync_cd,
  };
  
  struct iommu_pasid_table *arm_smmu_register_cd_table(struct device *dev,

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 2f86c6ac42b6..0c644be22b4b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1869,6 +1869,13 @@ static int arm_smmu_domain_finalise_s1(struct 
arm_smmu_domain *smmu_domain,
if (ret)
goto out_free_cd_tables;
  
+	/*

+* 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 = arm_smmu_sync_cd;
+


Modifying a static struct from here doesn't feel right. I think the
interface should be roughly similar to io-pgtable since the principle is
the same. So the sync() op should be separate from arm_cd_table_ops since
it's a callback into the driver. Maybe pass it to
iommu_register_pasid_table().


Sure, will take care of this.

Thanks & regards
Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 02/15] iommu: Add a simple PASID table library

2021-03-12 Thread Vivek Kumar Gautam

Hi Jean,


On 3/3/21 10:41 PM, Jean-Philippe Brucker wrote:

Hi Vivek,

Thanks again for working on this. I have a few comments but it looks
sensible overall.


Thanks a lot for reviewing the patch-series. Please find my responses 
inline below.




Regarding the overall design, I was initially assigning page directories
instead of whole PASID tables, which would simplify the driver and host
implementation. A major complication, however, is SMMUv3 accesses PASID
tables using a guest-physical address, so there is a messy negotiation
needed between host and guest when the host needs to allocate PASID
tables. Plus vSMMU needs PASID table assignment, so that's what the host
driver will implement.


By assigning the page directories, you mean setting up just the stage-1 
page table ops, and passing that information to the host using ATTACH_TABLE?
Right now when using kvmtool, the struct iommu_pasid_table_config is 
populated with the correct information, and this whole memory is mapped 
between host and guest by creating a mem bank using 
kvm__for_each_mem_bank().
Did I get you or did I fail terribly in understanding the point you are 
making here?

If it helps, I will publish my kvmtool branch.



On Fri, Jan 15, 2021 at 05:43:29PM +0530, Vivek Gautam wrote:

Add a small API in iommu subsystem to handle PASID table allocation
requests from different consumer drivers, such as a paravirtualized
iommu driver. The API provides ops for allocating and freeing PASID
table, writing to it and managing the table caches.

This library also provides for registering a vendor API that attaches
to these ops. The vendor APIs would eventually perform arch level
implementations for these PASID tables.


Although Arm might be the only vendor to ever use this, I think the
abstraction makes sense and isn't too invasive. Even if we called directly
into the SMMU driver from the virtio one, we'd still need patch 3 and
separate TLB invalidations ops.


Right, the idea was to make users of iommu-pasid-table - virtio-iommu or 
the arm-smmu-v3 - consistent. I also noticed that the whole process of 
allocating the pasid tables (or cd tables) and populating them with 
stage-1 page tables in viommu is also in-line with how things are in 
arm-smmu-v3 or atleast that's how the design can be in general - 
allocate pasid_table, and program stage-1 information into it, and then 
pass it across to host.





Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  drivers/iommu/iommu-pasid-table.h | 134 ++
  1 file changed, 134 insertions(+)
  create mode 100644 drivers/iommu/iommu-pasid-table.h

diff --git a/drivers/iommu/iommu-pasid-table.h 
b/drivers/iommu/iommu-pasid-table.h
new file mode 100644
index ..bd4f57656f67
--- /dev/null
+++ b/drivers/iommu/iommu-pasid-table.h
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PASID table management for the IOMMU
+ *
+ * Copyright (C) 2021 Arm Ltd.
+ */
+#ifndef __IOMMU_PASID_TABLE_H
+#define __IOMMU_PASID_TABLE_H
+
+#include 
+
+#include "arm/arm-smmu-v3/arm-smmu-v3.h"
+
+enum pasid_table_fmt {
+   PASID_TABLE_ARM_SMMU_V3,
+   PASID_TABLE_NUM_FMTS,
+};
+
+/**
+ * struct arm_smmu_cfg_info - arm-smmu-v3 specific configuration data
+ *
+ * @s1_cfg: arm-smmu-v3 stage1 config data
+ * @feat_flag: features supported by arm-smmu-v3 implementation
+ */
+struct arm_smmu_cfg_info {
+   struct arm_smmu_s1_cfg  *s1_cfg;
+   u32 feat_flag;
+};
+
+/**
+ * struct iommu_vendor_psdtable_cfg - Configuration data for PASID tables
+ *
+ * @iommu_dev: device performing the DMA table walks
+ * @fmt: The PASID table format
+ * @base: DMA address of the allocated table, set by the vendor driver
+ * @cfg: arm-smmu-v3 specific config data
+ */
+struct iommu_vendor_psdtable_cfg {
+   struct device   *iommu_dev;
+   enum pasid_table_fmtfmt;
+   dma_addr_t  base;
+   union {
+   struct arm_smmu_cfg_infocfg;


For the union to be extensible, that field should be called "arm" or
something like that.


Sure. Will amend this.

Thanks,
Vivek



Thanks,
Jean


+   } vendor;
+};
+
+struct iommu_vendor_psdtable_ops;
+
+/**
+ * struct iommu_pasid_table - describes a set of PASID tables
+ *
+ * @cookie: An opaque token provided by the IOMMU driver and passed back to any
+ * callback routine.
+ * @cfg: A copy of the PASID table configuration
+ * @ops: The PASID table operations in use for this set of page tables
+ */
+struct iommu_pasid_table {
+   void*cookie;
+   struct iommu_vendor_psdtable_cfgcfg;
+   struct iommu_vendor_psdtable_ops*ops;
+};
+
+#define pasid_table_cfg_to_table(pst_cfg) \
+   container_of((pst_cfg), struct 

Re: [PATCH RFC v1 04/15] iommu/arm-smmu-v3: Update CD base address info for user-space

2021-03-12 Thread Vivek Kumar Gautam

Hi Jean,

On 3/3/21 10:44 PM, Jean-Philippe Brucker wrote:

On Fri, Jan 15, 2021 at 05:43:31PM +0530, Vivek Gautam wrote:

Update base address information in vendor pasid table info to pass that
to user-space for stage1 table management.

Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
index 8a7187534706..ec37476c8d09 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c
@@ -55,6 +55,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct 
iommu_vendor_psdtable_cfg *pst_cfg,
if (arm_smmu_alloc_cd_leaf_table(dev, l1_desc))
return NULL;
  
+		if (s1cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)

+   pst_cfg->base = l1_desc->l2ptr_dma;
+


This isn't the right place, because this path allocates second-level
tables for two-level tables. I don't think we need pst_cfg->base at all,
because for both linear and two-level tables, the base pointer is in
cdcfg->cdtab_dma, which can be read directly.


Sure, will remove this.



Thanks,
Jean


l1ptr = cdcfg->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
/* An invalid L1CD can be cached */
@@ -211,6 +214,9 @@ static int arm_smmu_alloc_cd_tables(struct 
iommu_vendor_psdtable_cfg *pst_cfg)
goto err_free_l1;
}
  
+	if (s1cfg->s1fmt == STRTAB_STE_0_S1FMT_64K_L2)

+   pst_cfg->base = cdcfg->cdtab_dma;
+
return 0;
  
  err_free_l1:

--
2.17.1


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 12/15] iommu/virtio: Add support for INVALIDATE request

2021-03-03 Thread Vivek Kumar Gautam

Hi Jacob, Kevin,


On 3/4/21 11:28 AM, Tian, Kevin wrote:

From: Jacob Pan 
Sent: Thursday, March 4, 2021 2:29 AM

Hi Vivek,

On Fri, 15 Jan 2021 17:43:39 +0530, Vivek Gautam 
wrote:


From: Jean-Philippe Brucker 

Add support for tlb invalidation ops that can send invalidation
requests to back-end virtio-iommu when stage-1 page tables are
supported.


Just curious if it possible to reuse the iommu uapi for invalidation and others.
When we started out designing the iommu uapi, the intention was to support
both emulated and virtio iommu.


IIUC this patch is about the protocol between virtio-iommu frontend and backend.
After the virtio-iommu backend receives invalidation ops, it then needs to
forward the request to the host IOMMU driver through the existing iommu
uapi that you referred to, as a emulated VT-d or SMMU would do.


Thanks a lot for looking at the patch.

Yes this patch is to provide the front-end virtio interface for 
invalidation requests during map/unmap and when flushing the pasid 
tables when virtio-iommu requested pasid table (in other words cd tables 
for arm-smmu-v3) from the iommu-pasid-table library.
The kvmtool back-end virtio driver forwards these requests to vfio 
driver which then makes use of iommu uapi to finally request host iommu 
driver for handling these invalidations.


Regards
Vivek



Thanks
Kevin




Signed-off-by: Jean-Philippe Brucker 
[Vivek: Refactoring the iommu_flush_ops, and adding only one pasid sync
 op that's needed with current iommu-pasid-table infrastructure.
Also updating uapi defines as required by latest changes]
Signed-off-by: Vivek Gautam 
Cc: Joerg Roedel 
Cc: Will Deacon 
Cc: Michael S. Tsirkin 
Cc: Robin Murphy 
Cc: Jean-Philippe Brucker 
Cc: Eric Auger 
Cc: Alex Williamson 
Cc: Kevin Tian 
Cc: Jacob Pan 
Cc: Liu Yi L 
Cc: Lorenzo Pieralisi 
Cc: Shameerali Kolothum Thodi 
---
  drivers/iommu/virtio-iommu.c | 95



  1 file changed, 95 insertions(+)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index ae5dfd3f8269..004ea94e3731 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -63,6 +64,8 @@ struct viommu_mapping {
  };

  struct viommu_mm {
+   int pasid;
+   u64 archid;
struct io_pgtable_ops   *ops;
struct viommu_domain*domain;
  };
@@ -692,6 +695,98 @@ static void viommu_event_handler(struct

virtqueue

*vq) virtqueue_kick(vq);
  }

+/* PASID and pgtable APIs */
+
+static void __viommu_flush_pasid_tlb_all(struct viommu_domain

*vdomain,

+int pasid, u64 arch_id, int
type) +{
+   struct virtio_iommu_req_invalidate req = {
+   .head.type  = VIRTIO_IOMMU_T_INVALIDATE,
+   .inv_gran   =
cpu_to_le32(VIRTIO_IOMMU_INVAL_G_PASID),
+   .flags  =
cpu_to_le32(VIRTIO_IOMMU_INVAL_F_PASID),
+   .inv_type   = cpu_to_le32(type),
+
+   .domain = cpu_to_le32(vdomain->id),
+   .pasid  = cpu_to_le32(pasid),
+   .archid = cpu_to_le64(arch_id),
+   };
+
+   if (viommu_send_req_sync(vdomain->viommu, , sizeof(req)))
+   pr_debug("could not send invalidate request\n");
+}
+
+static void viommu_flush_tlb_add(struct iommu_iotlb_gather *gather,
+unsigned long iova, size_t granule,
+void *cookie)
+{
+   struct viommu_mm *viommu_mm = cookie;
+   struct viommu_domain *vdomain = viommu_mm->domain;
+   struct iommu_domain *domain = >domain;
+
+   iommu_iotlb_gather_add_page(domain, gather, iova, granule);
+}
+
+static void viommu_flush_tlb_walk(unsigned long iova, size_t size,
+ size_t granule, void *cookie)
+{
+   struct viommu_mm *viommu_mm = cookie;
+   struct viommu_domain *vdomain = viommu_mm->domain;
+   struct virtio_iommu_req_invalidate req = {
+   .head.type  = VIRTIO_IOMMU_T_INVALIDATE,
+   .inv_gran   = cpu_to_le32(VIRTIO_IOMMU_INVAL_G_VA),
+   .inv_type   =

cpu_to_le32(VIRTIO_IOMMU_INV_T_IOTLB),

+   .flags  =
cpu_to_le32(VIRTIO_IOMMU_INVAL_F_ARCHID), +
+   .domain = cpu_to_le32(vdomain->id),
+   .pasid  = cpu_to_le32(viommu_mm->pasid),
+   .archid = cpu_to_le64(viommu_mm->archid),
+   .virt_start = cpu_to_le64(iova),
+   .nr_pages   = cpu_to_le64(size / granule),
+   .granule= ilog2(granule),
+   };
+
+   if (viommu_add_req(vdomain->viommu, , sizeof(req)))
+   pr_debug("could not add invalidate request\n");
+}
+
+static void viommu_flush_tlb_all(void 

Re: [PATCH 2/2] iommu: arm-smmu-v3: Report domain nesting info reuqired for stage1

2021-03-03 Thread Vivek Kumar Gautam

Hi Eric,


On 2/12/21 11:43 PM, Auger Eric wrote:

Hi Vivek,

On 2/12/21 11:58 AM, Vivek Gautam wrote:

Update nested domain information required for stage1 page table.


s/reuqired/required in the commit title


My bad! Will correct it.



Signed-off-by: Vivek Gautam 
---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index c11dd3940583..728018921fae 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2555,6 +2555,7 @@ static int arm_smmu_domain_nesting_info(struct 
arm_smmu_domain *smmu_domain,
void *data)
  {
struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
+   struct arm_smmu_device *smmu = smmu_domain->smmu;
unsigned int size;
  
  	if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)

@@ -2571,9 +2572,20 @@ static int arm_smmu_domain_nesting_info(struct 
arm_smmu_domain *smmu_domain,
return 0;
}
  
-	/* report an empty iommu_nesting_info for now */

-   memset(info, 0x0, size);
+   /* Update the nesting info as required for stage1 page tables */
+   info->addr_width = smmu->ias;
+   info->format = IOMMU_PASID_FORMAT_ARM_SMMU_V3;
+   info->features = IOMMU_NESTING_FEAT_BIND_PGTBL |

I understood IOMMU_NESTING_FEAT_BIND_PGTBL advertises the requirement to
bind tables per PASID, ie. passing iommu_gpasid_bind_data.
In ARM case I guess you plan to use attach/detach_pasid_table API with
iommu_pasid_table_config struct. So I understood we should add a new
feature here.


Right, the idea is to let vfio know that we support pasid table binding, and
 I thought we could use the same flag. But clearly that's not the case.
 I will add a new feature.


+IOMMU_NESTING_FEAT_PAGE_RESP |
+IOMMU_NESTING_FEAT_CACHE_INVLD;
+   info->pasid_bits = smmu->ssid_bits;
+   info->vendor.smmuv3.asid_bits = smmu->asid_bits;
+   info->vendor.smmuv3.pgtbl_fmt = ARM_64_LPAE_S1;
+   memset(>padding, 0x0, 12);
+   memset(>vendor.smmuv3.padding, 0x0, 9);
+
info->argsz = size;
+

spurious new line


Sure, will correct this.


return 0;
  }
  





Thanks
Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 1/2] iommu: Report domain nesting info for arm-smmu-v3

2021-03-03 Thread Vivek Kumar Gautam

Hi Eric,


On 2/12/21 11:43 PM, Auger Eric wrote:

Hi Vivek,
On 2/12/21 11:58 AM, Vivek Gautam wrote:

Add a vendor specific structure for domain nesting info for
arm smmu-v3, and necessary info fields required to populate
stage1 page tables.

Signed-off-by: Vivek Gautam 
---
  include/uapi/linux/iommu.h | 31 +--
  1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 4d3d988fa353..5f059bcf7720 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -323,7 +323,8 @@ struct iommu_gpasid_bind_data {
  #define IOMMU_GPASID_BIND_VERSION_1   1
__u32 version;
  #define IOMMU_PASID_FORMAT_INTEL_VTD  1
-#define IOMMU_PASID_FORMAT_LAST2
+#define IOMMU_PASID_FORMAT_ARM_SMMU_V3 2
+#define IOMMU_PASID_FORMAT_LAST3
__u32 format;
__u32 addr_width;
  #define IOMMU_SVA_GPASID_VAL  (1 << 0) /* guest PASID valid */
@@ -409,6 +410,21 @@ struct iommu_nesting_info_vtd {
__u64   ecap_reg;
  };
  
+/*

+ * struct iommu_nesting_info_arm_smmuv3 - Arm SMMU-v3 nesting info.
+ */
+struct iommu_nesting_info_arm_smmuv3 {
+   __u32   flags;
+   __u16   asid_bits;
+
+   /* Arm LPAE page table format as per kernel */
+#define ARM_PGTBL_32_LPAE_S1   (0x0)
+#define ARM_PGTBL_64_LPAE_S1   (0x2)


Thanks for reviewing and I am terribly sorry for coming to it with delay.


Shouldn't it be a bitfield instead as both can be supported (the actual
driver only supports 64b table format though). Does it match matches
IDR0.TTF?


Yes, it should be a bitfield rather, and it doesn't match with IDR0.TTF. 
This is

 to hint the stage1 table allocations from viommu.
 Please see viommu_setup_pgtable() in the patch at [1].


+   __u8pgtbl_fmt;

So I understand this API is supposed to allow VFIO to expose those info
early enough to the userspace to help configuring the viommu and avoid
errors later on. I wonder how far we want to go on this path. What about
those other caps that impact the STE/CD validity. There may be others...

SMMU_IDR0.CD2L (support of 2 stage CD)
SMMU_IDR0.TTENDIAN (endianness)
SMMU_IDR0.HTTU (if 0 forbids HA/HD setting in the CD)
SMMU_IDR3.STT (impacts T0SZ)


Right. The idea was to start with a minimal set of configuration.

But as you rightly pointed out we need a scalable solution to this problem

for arm-smmu-v3. I am now thinking if we could even use the nesting_info

for arm. We don't want to end up adding flags for all the feature bits.

Let me know if you have any suggestions.

Best regards
Vivek

[1] 
https://lore.kernel.org/linux-arm-kernel/20210115121342.15093-14-vivek.gau...@arm.com/


[snip]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info

2021-02-12 Thread Vivek Kumar Gautam

Hi Eric,


On 2/12/21 3:48 PM, Vivek Kumar Gautam wrote:

Hi Eric,


On 2/12/21 3:27 PM, Auger Eric wrote:

Hi Vivek, Yi,

On 2/12/21 8:14 AM, Vivek Gautam wrote:

Hi Yi,


On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L  wrote:


Hi Eric,


From: Auger Eric 
Sent: Tuesday, January 19, 2021 6:03 PM

Hi Yi, Vivek,


[...]

I see. I think there needs a change in the code there. Should also
expect
a nesting_info returned instead of an int anymore. @Eric, how
about your
opinion?

 domain = iommu_get_domain_for_dev(>pdev->dev);
 ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,

);

 if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
 /*
  * No need go futher as no page request service support.
  */
 return 0;
 }

Sure I think it is "just" a matter of synchro between the 2 series.
Yi,


exactly.


do you have plans to respin part of
[PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
or would you allow me to embed this patch in my series.


My v7 hasn’t touch the prq change yet. So I think it's better for
you to
embed it to  your series. ^_^>>


Can you please let me know if you have an updated series of these
patches? It will help me to work with virtio-iommu/arm side changes.


As per the previous discussion, I plan to take those 2 patches in my
SMMUv3 nested stage series:

[PATCH v7 01/16] iommu: Report domain nesting info
[PATCH v7 02/16] iommu/smmu: Report empty domain nesting info

we need to upgrade both since we do not want to report an empty nesting
info anymore, for arm.


Absolutely. Let me send the couple of patches that I have been using,
that add arm configuration.


Posted the couple of patches that I have been using -

https://lore.kernel.org/linux-iommu/20210212105859.8445-1-vivek.gau...@arm.com/T/#t


Thanks & regards
Vivek



Best regards
Vivek



Thanks

Eric


Thanks & regards
Vivek




IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info

2021-02-12 Thread Vivek Kumar Gautam

Hi Eric,


On 2/12/21 3:27 PM, Auger Eric wrote:

Hi Vivek, Yi,

On 2/12/21 8:14 AM, Vivek Gautam wrote:

Hi Yi,


On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L  wrote:


Hi Eric,


From: Auger Eric 
Sent: Tuesday, January 19, 2021 6:03 PM

Hi Yi, Vivek,


[...]

I see. I think there needs a change in the code there. Should also expect
a nesting_info returned instead of an int anymore. @Eric, how about your
opinion?

 domain = iommu_get_domain_for_dev(>pdev->dev);
 ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,

);

 if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
 /*
  * No need go futher as no page request service support.
  */
 return 0;
 }

Sure I think it is "just" a matter of synchro between the 2 series. Yi,


exactly.


do you have plans to respin part of
[PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
or would you allow me to embed this patch in my series.


My v7 hasn’t touch the prq change yet. So I think it's better for you to
embed it to  your series. ^_^>>


Can you please let me know if you have an updated series of these
patches? It will help me to work with virtio-iommu/arm side changes.


As per the previous discussion, I plan to take those 2 patches in my
SMMUv3 nested stage series:

[PATCH v7 01/16] iommu: Report domain nesting info
[PATCH v7 02/16] iommu/smmu: Report empty domain nesting info

we need to upgrade both since we do not want to report an empty nesting
info anymore, for arm.


Absolutely. Let me send the couple of patches that I have been using,
that add arm configuration.

Best regards
Vivek



Thanks

Eric


Thanks & regards
Vivek




IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm

2021-01-25 Thread Vivek Kumar Gautam

Hi Shameer,


On 1/22/21 9:19 PM, Shameerali Kolothum Thodi wrote:

Hi Vivek,


-Original Message-
From: Vivek Kumar Gautam [mailto:vivek.gau...@arm.com]
Sent: 21 January 2021 17:34
To: Auger Eric ; linux-ker...@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org;
virtualizat...@lists.linux-foundation.org
Cc: j...@8bytes.org; will.dea...@arm.com; m...@redhat.com;
robin.mur...@arm.com; jean-phili...@linaro.org;
alex.william...@redhat.com; kevin.t...@intel.com;
jacob.jun@linux.intel.com; yi.l@intel.com; lorenzo.pieral...@arm.com;
Shameerali Kolothum Thodi 
Subject: Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with
Arm

Hi Eric,


On 1/19/21 2:33 PM, Auger Eric wrote:

Hi Vivek,

On 1/15/21 1:13 PM, Vivek Gautam wrote:

This patch-series aims at enabling Nested stage translation in guests
using virtio-iommu as the paravirtualized iommu. The backend is
supported with Arm SMMU-v3 that provides nested stage-1 and stage-2

translation.


This series derives its purpose from various efforts happening to add
support for Shared Virtual Addressing (SVA) in host and guest. On
Arm, most of the support for SVA has already landed. The support for
nested stage translation and fault reporting to guest has been proposed [1].
The related changes required in VFIO [2] framework have also been put
forward.

This series proposes changes in virtio-iommu to program PASID tables
and related stage-1 page tables. A simple iommu-pasid-table library
is added for this purpose that interacts with vendor drivers to
allocate and populate PASID tables.
In Arm SMMUv3 we propose to pull the Context Descriptor (CD)
management code out of the arm-smmu-v3 driver and add that as a glue
vendor layer to support allocating CD tables, and populating them with right

values.

These CD tables are essentially the PASID tables and contain stage-1
page table configurations too.
A request to setup these CD tables come from virtio-iommu driver
using the iommu-pasid-table library when running on Arm. The
virtio-iommu then pass these PASID tables to the host using the right
virtio backend and support in VMM.

For testing we have added necessary support in kvmtool. The changes
in kvmtool are based on virtio-iommu development branch by
Jean-Philippe Brucker [3].

The tested kernel branch contains following in the order bottom to
top on the git hash -
a) v5.11-rc3
b) arm-smmu-v3 [1] and vfio [2] changes from Eric to add nested page
 table support for Arm.
c) Smmu test engine patches from Jean-Philippe's branch [4]
d) This series
e) Domain nesting info patches [5][6][7].
f) Changes to add arm-smmu-v3 specific nesting info (to be sent to
 the list).

This kernel is tested on Neoverse reference software stack with Fixed
virtual platform. Public version of the software stack and FVP is
available here[8][9].

A big thanks to Jean-Philippe for his contributions towards this work
and for his valuable guidance.

[1]
https://lore.kernel.org/linux-iommu/20201118112151.25412-1-eric.auger
@redhat.com/T/ [2]


https://lore.kernel.org/kvmarm/20201116110030.32335-12-eric.auger@red

hat.com/T/ [3]
https://jpbrucker.net/git/kvmtool/log/?h=virtio-iommu/devel
[4] https://jpbrucker.net/git/linux/log/?h=sva/smmute
[5]
https://lore.kernel.org/kvm/1599734733-6431-2-git-send-email-yi.l.liu
@intel.com/ [6]
https://lore.kernel.org/kvm/1599734733-6431-3-git-send-email-yi.l.liu
@intel.com/ [7]
https://lore.kernel.org/kvm/1599734733-6431-4-git-send-email-yi.l.liu
@intel.com/ [8]
https://developer.arm.com/tools-and-software/open-source-software/arm
-platforms-software/arm-ecosystem-fvps
[9]
https://git.linaro.org/landing-teams/working/arm/arm-reference-platfo
rms.git/about/docs/rdn1edge/user-guide.rst


Could you share a public branch where we could find all the kernel pieces.

Thank you in advance


Apologies for the delay. It took a bit of time to sort things out for a public
branch.
The branch is available in my github now. Please have a look.

https://github.com/vivek-arm/linux/tree/5.11-rc3-nested-pgtbl-arm-smmuv3-vi
rtio-iommu
 > Thanks for this. Do you have a corresponding kvmtool branch mentioned 

above as public?

Thanks for showing interest. I will publish the kvmtool branch asap. 
Though the current development is based on Jean's branch for 
virtio-iommu [1], I plan to rebase the changes to master soon.


Thanks & regards
Vivek

[1] https://jpbrucker.net/git/kvmtool/log/?h=virtio-iommu/devel


Thanks,
Shameer


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RFC v1 00/15] iommu/virtio: Nested stage support with Arm

2021-01-21 Thread Vivek Kumar Gautam

Hi Eric,


On 1/19/21 2:33 PM, Auger Eric wrote:

Hi Vivek,

On 1/15/21 1:13 PM, Vivek Gautam wrote:

This patch-series aims at enabling Nested stage translation in guests
using virtio-iommu as the paravirtualized iommu. The backend is supported
with Arm SMMU-v3 that provides nested stage-1 and stage-2 translation.

This series derives its purpose from various efforts happening to add
support for Shared Virtual Addressing (SVA) in host and guest. On Arm,
most of the support for SVA has already landed. The support for nested
stage translation and fault reporting to guest has been proposed [1].
The related changes required in VFIO [2] framework have also been put
forward.

This series proposes changes in virtio-iommu to program PASID tables
and related stage-1 page tables. A simple iommu-pasid-table library
is added for this purpose that interacts with vendor drivers to
allocate and populate PASID tables.
In Arm SMMUv3 we propose to pull the Context Descriptor (CD) management
code out of the arm-smmu-v3 driver and add that as a glue vendor layer
to support allocating CD tables, and populating them with right values.
These CD tables are essentially the PASID tables and contain stage-1
page table configurations too.
A request to setup these CD tables come from virtio-iommu driver using
the iommu-pasid-table library when running on Arm. The virtio-iommu
then pass these PASID tables to the host using the right virtio backend
and support in VMM.

For testing we have added necessary support in kvmtool. The changes in
kvmtool are based on virtio-iommu development branch by Jean-Philippe
Brucker [3].

The tested kernel branch contains following in the order bottom to top
on the git hash -
a) v5.11-rc3
b) arm-smmu-v3 [1] and vfio [2] changes from Eric to add nested page
table support for Arm.
c) Smmu test engine patches from Jean-Philippe's branch [4]
d) This series
e) Domain nesting info patches [5][6][7].
f) Changes to add arm-smmu-v3 specific nesting info (to be sent to
the list).

This kernel is tested on Neoverse reference software stack with
Fixed virtual platform. Public version of the software stack and
FVP is available here[8][9].

A big thanks to Jean-Philippe for his contributions towards this work
and for his valuable guidance.

[1] 
https://lore.kernel.org/linux-iommu/20201118112151.25412-1-eric.au...@redhat.com/T/
[2] 
https://lore.kernel.org/kvmarm/20201116110030.32335-12-eric.au...@redhat.com/T/
[3] https://jpbrucker.net/git/kvmtool/log/?h=virtio-iommu/devel
[4] https://jpbrucker.net/git/linux/log/?h=sva/smmute
[5] 
https://lore.kernel.org/kvm/1599734733-6431-2-git-send-email-yi.l@intel.com/
[6] 
https://lore.kernel.org/kvm/1599734733-6431-3-git-send-email-yi.l@intel.com/
[7] 
https://lore.kernel.org/kvm/1599734733-6431-4-git-send-email-yi.l@intel.com/
[8] 
https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps
[9] 
https://git.linaro.org/landing-teams/working/arm/arm-reference-platforms.git/about/docs/rdn1edge/user-guide.rst


Could you share a public branch where we could find all the kernel pieces.

Thank you in advance


Apologies for the delay. It took a bit of time to sort things out for a 
public branch.

The branch is available in my github now. Please have a look.

https://github.com/vivek-arm/linux/tree/5.11-rc3-nested-pgtbl-arm-smmuv3-virtio-iommu


Thanks and regards
Vivek



Best Regards

Eric


Jean-Philippe Brucker (6):
   iommu/virtio: Add headers for table format probing
   iommu/virtio: Add table format probing
   iommu/virtio: Add headers for binding pasid table in iommu
   iommu/virtio: Add support for INVALIDATE request
   iommu/virtio: Attach Arm PASID tables when available
   iommu/virtio: Add support for Arm LPAE page table format

Vivek Gautam (9):
   iommu/arm-smmu-v3: Create a Context Descriptor library
   iommu: Add a simple PASID table library
   iommu/arm-smmu-v3: Update drivers to work with iommu-pasid-table
   iommu/arm-smmu-v3: Update CD base address info for user-space
   iommu/arm-smmu-v3: Set sync op from consumer driver of cd-lib
   iommu: Add asid_bits to arm smmu-v3 stage1 table info
   iommu/virtio: Update table format probing header
   iommu/virtio: Prepare to add attach pasid table infrastructure
   iommu/virtio: Update fault type and reason info for viommu fault

  drivers/iommu/arm/arm-smmu-v3/Makefile|   2 +-
  .../arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c  | 283 +++
  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  16 +-
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 268 +--
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   4 +-
  drivers/iommu/iommu-pasid-table.h | 140 
  drivers/iommu/virtio-iommu.c  | 692 +-
  include/uapi/linux/iommu.h|   2 +-
  include/uapi/linux/virtio_iommu.h | 158 +++-
  9 files changed, 1303 insertions(+), 262 deletions(-)
  create mode 100644