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 <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Jean-Philippe Brucker <[email protected]>
> Cc: Eric Auger <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Kevin Tian <[email protected]>
> Cc: Jacob Pan <[email protected]>
> Cc: Liu Yi L <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Shameerali Kolothum Thodi <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> Cc: Joerg Roedel <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Michael S. Tsirkin <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Jean-Philippe Brucker <[email protected]>
>> Cc: Eric Auger <[email protected]>
>> Cc: Alex Williamson <[email protected]>
>> Cc: Kevin Tian <[email protected]>
>> Cc: Jacob Pan <[email protected]>
>> Cc: Liu Yi L <[email protected]>
>> Cc: Lorenzo Pieralisi <[email protected]>
>> Cc: Shameerali Kolothum Thodi <[email protected]>
>> ---
>> 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
>>
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/[email protected]/
Thanks,
Jean
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/[email protected]/
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
>