diff mbox series

[RFC,v1,01/11] uapi/virtio-iommu: Add page request grp-id and flags information

Message ID 20210423095147.27922-2-vivek.gautam@arm.com (mailing list archive)
State New, archived
Headers show
Series iommu/virtio: vSVA support with Arm | expand

Commit Message

Vivek Kumar Gautam April 23, 2021, 9:51 a.m. UTC 
Add fault information for group-id and necessary flags for page
request faults that can be handled by page fault handler in
virtio-iommu driver.

Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
---
 include/uapi/linux/virtio_iommu.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Jean-Philippe Brucker Sept. 21, 2021, 3:58 p.m. UTC | #1 
Hi Vivek,

Thanks a lot for your work on this

On Fri, Apr 23, 2021 at 03:21:37PM +0530, Vivek Gautam wrote:
> Add fault information for group-id and necessary flags for page
> request faults that can be handled by page fault handler in
> virtio-iommu driver.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  include/uapi/linux/virtio_iommu.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index f8bf927a0689..accc3318ce46 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -307,14 +307,27 @@ struct virtio_iommu_req_invalidate {
>  #define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV	1
>  #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ		2
>  
> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PASID_VALID		(1 << 0)
> +#define VIRTIO_IOMMU_FAULT_PRQ_F_LAST_PAGE		(1 << 1)
> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PRIV_DATA		(1 << 2)
> +#define VIRTIO_IOMMU_FAULT_PRQ_F_NEEDS_PASID		(1 << 3)

I don't think this one is necessary here. The NEEDS_PASID flags added by
commit 970471914c67 ("iommu: Allow page responses without PASID") mainly
helps Linux keep track of things internally. It does tell the fault
handler whether to reply with PASID or not, but we don't need that here.
The virtio-iommu driver knows whether a PASID is required by looking at
the "PRG Response PASID Required" bit in the PCIe capability. For non-PCIe
faults (e.g. SMMU stall), I'm guessing we'll need a PROBE property to
declare that the endpoint supports recoverable faults anyway, so "PASID
required in response" can go through there as well.

> +
> +#define VIRTIO_IOMMU_FAULT_UNREC_F_PASID_VALID		(1 << 0)
> +#define VIRTIO_IOMMU_FAULT_UNREC_F_ADDR_VALID		(1 << 1)
> +#define VIRTIO_IOMMU_FAULT_UNREC_F_FETCH_ADDR_VALID	(1 << 2)
> +
>  struct virtio_iommu_fault {
>  	__u8					reason;
>  	__u8					reserved[3];
>  	__le16					flt_type;
>  	__u8					reserved2[2];
> +	/* flags is actually permission flags */

It's also used for declaring validity of fields.
VIRTIO_IOMMU_FAULT_F_ADDRESS already tells whether the address field is
valid, so all the other flags introduced by this patch can go in here.

>  	__le32					flags;
> +	/* flags for PASID and Page request handling info */
> +	__le32					pr_evt_flags;
>  	__le32					endpoint;
>  	__le32					pasid;
> +	__le32					grpid;

I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
seems more than enough.

New fields must be appended at the end of the struct, because old drivers
will expect to find the 'endpoint' field at this offset. You could remove
'reserved3' while adding 'grpid', to keep the struct layout.

>  	__u8					reserved3[4];
>  	__le64					address;
>  	__u8					reserved4[8];


So the base structure, currently in the spec, looks like this:

	struct virtio_iommu_fault {
		u8   reason;
		u8   reserved[3];
		le32 flags;
		le32 endpoint;
		le32 reserved1;
		le64 address;
	};

	#define VIRTIO_IOMMU_FAULT_F_READ	(1 << 0)
	#define VIRTIO_IOMMU_FAULT_F_WRITE	(1 << 1)
	#define VIRTIO_IOMMU_FAULT_F_ADDRESS	(1 << 8)

The extended struct could be:

	struct virtio_iommu_fault {
		u8   reason;
		u8   reserved[3];
		le32 flags;
		le32 endpoint;
		le32 pasid;
		le64 address;
		/* Page request group ID */
		le16 group_id;
		u8   reserved1[6];
		/* For VT-d private data */
		le64 private_data[2];
	};

	#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_PRIVILEGED	(1 << 3)
	/* Last fault in group */
	#define VIRTIO_IOMMU_FAULT_F_LAST	(1 << 4)
	/* Fault is a recoverable page request and requires a response */
	#define VIRTIO_IOMMU_FAULT_F_PAGE_REQ	(1 << 5)

	/* address field is valid */
	#define VIRTIO_IOMMU_FAULT_F_ADDRESS	(1 << 8)
	/* pasid field is valid */
	#define VIRTIO_IOMMU_FAULT_F_PASID	(1 << 9)
	/* group_id field is valid */
	#define VIRTIO_IOMMU_FAULT_F_GROUP_ID	(1 << 10)
	/* private data field is valid */
	#define VIRTIO_IOMMU_FAULT_F_PRIV_DATA	(1 << 11)

Thanks,
Jean
Vivek Kumar Gautam Sept. 30, 2021, 4:56 a.m. UTC | #2 
Hi Jean,


On 9/21/21 9:28 PM, Jean-Philippe Brucker wrote:
> Hi Vivek,
> 
> Thanks a lot for your work on this

Thanks a lot for taking a look at it. I hope that most of the changes in 
this series and the nested page table series [1] are independent of the 
presently on-going /dev/iommu proposal, and can be separately reviewed.

Please find my comments inline below.

> 
> On Fri, Apr 23, 2021 at 03:21:37PM +0530, Vivek Gautam wrote:
>> Add fault information for group-id and necessary flags for page
>> request faults that can be handled by page fault handler in
>> virtio-iommu driver.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   include/uapi/linux/virtio_iommu.h | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
>> index f8bf927a0689..accc3318ce46 100644
>> --- a/include/uapi/linux/virtio_iommu.h
>> +++ b/include/uapi/linux/virtio_iommu.h
>> @@ -307,14 +307,27 @@ struct virtio_iommu_req_invalidate {
>>   #define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV	1
>>   #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ		2
>>   
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PASID_VALID		(1 << 0)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_LAST_PAGE		(1 << 1)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_PRIV_DATA		(1 << 2)
>> +#define VIRTIO_IOMMU_FAULT_PRQ_F_NEEDS_PASID		(1 << 3)
> 
> I don't think this one is necessary here. The NEEDS_PASID flags added by
> commit 970471914c67 ("iommu: Allow page responses without PASID") mainly
> helps Linux keep track of things internally. It does tell the fault
> handler whether to reply with PASID or not, but we don't need that here.
> The virtio-iommu driver knows whether a PASID is required by looking at
> the "PRG Response PASID Required" bit in the PCIe capability. For non-PCIe
> faults (e.g. SMMU stall), I'm guessing we'll need a PROBE property to
> declare that the endpoint supports recoverable faults anyway, so "PASID
> required in response" can go through there as well.

Sure, I will remove this flag, and rather read the PCIe cap to find out 
if PASID is required or not. After this series, I will follow up with 
the non-PCIe fault handling.

> 
>> +
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_PASID_VALID		(1 << 0)
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_ADDR_VALID		(1 << 1)
>> +#define VIRTIO_IOMMU_FAULT_UNREC_F_FETCH_ADDR_VALID	(1 << 2)
>> +
>>   struct virtio_iommu_fault {
>>   	__u8					reason;
>>   	__u8					reserved[3];
>>   	__le16					flt_type;
>>   	__u8					reserved2[2];
>> +	/* flags is actually permission flags */
> 
> It's also used for declaring validity of fields.
> VIRTIO_IOMMU_FAULT_F_ADDRESS already tells whether the address field is
> valid, so all the other flags introduced by this patch can go in here.

Sure, will remove pr_evt_flags field, and move all the flags to 'flags'.

> 
>>   	__le32					flags;
>> +	/* flags for PASID and Page request handling info */
>> +	__le32					pr_evt_flags;
>>   	__le32					endpoint;
>>   	__le32					pasid;
>> +	__le32					grpid;
> 
> I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
> PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
> is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
> seems more than enough.

Right, I will update this to 16-bits field. It won't be okay to update 
the iommu uAPI now, right?

> 
> New fields must be appended at the end of the struct, because old drivers
> will expect to find the 'endpoint' field at this offset. You could remove
> 'reserved3' while adding 'grpid', to keep the struct layout.

Sure, will update this.

> 
>>   	__u8					reserved3[4];
>>   	__le64					address;
>>   	__u8					reserved4[8];
> 
> 
> So the base structure, currently in the spec, looks like this:
> 
> 	struct virtio_iommu_fault {
> 		u8   reason;
> 		u8   reserved[3];
> 		le32 flags;
> 		le32 endpoint;
> 		le32 reserved1;
> 		le64 address;
> 	};
> 
> 	#define VIRTIO_IOMMU_FAULT_F_READ	(1 << 0)
> 	#define VIRTIO_IOMMU_FAULT_F_WRITE	(1 << 1)
> 	#define VIRTIO_IOMMU_FAULT_F_ADDRESS	(1 << 8)
> 
> The extended struct could be:
> 
> 	struct virtio_iommu_fault {
> 		u8   reason;
> 		u8   reserved[3];
> 		le32 flags;
> 		le32 endpoint;
> 		le32 pasid;
> 		le64 address;
> 		/* Page request group ID */
> 		le16 group_id;
> 		u8   reserved1[6];
> 		/* For VT-d private data */
> 		le64 private_data[2];
> 	};
> 
> 	#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_PRIVILEGED	(1 << 3)
> 	/* Last fault in group */
> 	#define VIRTIO_IOMMU_FAULT_F_LAST	(1 << 4)
> 	/* Fault is a recoverable page request and requires a response */
> 	#define VIRTIO_IOMMU_FAULT_F_PAGE_REQ	(1 << 5)
> 
> 	/* address field is valid */
> 	#define VIRTIO_IOMMU_FAULT_F_ADDRESS	(1 << 8)
> 	/* pasid field is valid */
> 	#define VIRTIO_IOMMU_FAULT_F_PASID	(1 << 9)
> 	/* group_id field is valid */
> 	#define VIRTIO_IOMMU_FAULT_F_GROUP_ID	(1 << 10)
> 	/* private data field is valid */
> 	#define VIRTIO_IOMMU_FAULT_F_PRIV_DATA	(1 << 11)

Thanks Jean for summarizing it here. I will update the patch.

Best regards
Vivek
Jean-Philippe Brucker Sept. 30, 2021, 8:49 a.m. UTC | #3 
On Thu, Sep 30, 2021 at 10:26:35AM +0530, Vivek Kumar Gautam wrote:
> > I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
> > PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
> > is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
> > seems more than enough.
> 
> Right, I will update this to 16-bits field. It won't be okay to update the
> iommu uAPI now, right?

Since there is no UAPI transport for the fault request/response at the
moment, it should be possible to update it.

Thanks,
Jean
Vivek Kumar Gautam Sept. 30, 2021, 10:57 a.m. UTC | #4 
On 9/30/21 2:19 PM, Jean-Philippe Brucker wrote:
> On Thu, Sep 30, 2021 at 10:26:35AM +0530, Vivek Kumar Gautam wrote:
>>> I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful.
>>> PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid
>>> is the endpoint, 16-bit means 64k in-flight faults per endpoint, which
>>> seems more than enough.
>>
>> Right, I will update this to 16-bits field. It won't be okay to update the
>> iommu uAPI now, right?
> 
> Since there is no UAPI transport for the fault request/response at the
> moment, it should be possible to update it.

Righty! I will add a patch for that too then.



Thanks & regards

Vivek
diff mbox series

Patch

diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index f8bf927a0689..accc3318ce46 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -307,14 +307,27 @@  struct virtio_iommu_req_invalidate {
 #define VIRTIO_IOMMU_FAULT_F_DMA_UNRECOV	1
 #define VIRTIO_IOMMU_FAULT_F_PAGE_REQ		2
 
+#define VIRTIO_IOMMU_FAULT_PRQ_F_PASID_VALID		(1 << 0)
+#define VIRTIO_IOMMU_FAULT_PRQ_F_LAST_PAGE		(1 << 1)
+#define VIRTIO_IOMMU_FAULT_PRQ_F_PRIV_DATA		(1 << 2)
+#define VIRTIO_IOMMU_FAULT_PRQ_F_NEEDS_PASID		(1 << 3)
+
+#define VIRTIO_IOMMU_FAULT_UNREC_F_PASID_VALID		(1 << 0)
+#define VIRTIO_IOMMU_FAULT_UNREC_F_ADDR_VALID		(1 << 1)
+#define VIRTIO_IOMMU_FAULT_UNREC_F_FETCH_ADDR_VALID	(1 << 2)
+
 struct virtio_iommu_fault {
 	__u8					reason;
 	__u8					reserved[3];
 	__le16					flt_type;
 	__u8					reserved2[2];
+	/* flags is actually permission flags */
 	__le32					flags;
+	/* flags for PASID and Page request handling info */
+	__le32					pr_evt_flags;
 	__le32					endpoint;
 	__le32					pasid;
+	__le32					grpid;
 	__u8					reserved3[4];
 	__le64					address;
 	__u8					reserved4[8];