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.gautam@xxxxxxx>
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> Cc: Eric Auger <eric.auger@xxxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Cc: Liu Yi L <yi.l.liu@xxxxxxxxx>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
