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

[Jean-Philippe Brucker]

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
>