Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault
From: Jean-Philippe Brucker <hidden>
Date: 2021-03-03 20:56:08
Also in:
linux-iommu, lkml, virtualization
On Fri, Jan 15, 2021 at 05:43:42PM +0530, Vivek Gautam wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Cc: Joerg Roedel <joro@8bytes.org> Cc: Will Deacon <redacted> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Jean-Philippe Brucker <redacted> Cc: Eric Auger <eric.auger@redhat.com> Cc: Alex Williamson <redacted> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jacob Pan <redacted> Cc: Liu Yi L <redacted> Cc: Lorenzo Pieralisi <redacted> Cc: Shameerali Kolothum Thodi <redacted> --- 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. 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.
+
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 :) Thanks, Jean
__le64 address; + __le32 pasid; + __u8 reserved3[4]; }; #endif -- 2.17.1
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel