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

[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 <vivek.gau...@arm.com>
Cc: Joerg Roedel <j...@8bytes.org>
Cc: Will Deacon <will.dea...@arm.com>
Cc: Michael S. Tsirkin <m...@redhat.com>
Cc: Robin Murphy <robin.mur...@arm.com>
Cc: Jean-Philippe Brucker <jean-phili...@linaro.org>
Cc: Eric Auger <eric.au...@redhat.com>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: Kevin Tian <kevin.t...@intel.com>
Cc: Jacob Pan <jacob.jun....@linux.intel.com>
Cc: Liu Yi L <yi.l....@intel.com>
Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Cc: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
---
  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_PERMISSION                5
+#define VIRTIO_IOMMU_FAULT_R_ACCESS            6
+#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 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.


+
  struct virtio_iommu_fault {
        __u8                                    reason;
-       __u8                                    reserved[3];
+       __le16                                  flt_type;
+       __u8                                    reserved;
        __le32                                  flags;
        __le32                                  endpoint;
        __u8                                    reserved2[4];

Why not replace reserved2 with the pasid?  It fits perfectly :)

Sure, will do it.

Thanks & regards
Vivek


Thanks,
Jean

        __le64                                  address;
+       __le32                                  pasid;
+       __u8                                    reserved3[4];
  };
  
  #endif
--
2.17.1