Re: [PATCH RFC v1 06/15] iommu/virtio: Add headers for table format probing

[Vivek Kumar Gautam <vivek.gautam@arm.com>]

On 3/3/21 10:47 PM, Jean-Philippe Brucker wrote:
> On Fri, Jan 15, 2021 at 05:43:33PM +0530, Vivek Gautam wrote:
>> From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>>
>> Add required UAPI defines for probing table format for underlying
>> iommu hardware. The device may provide information about hardware
>> tables and additional capabilities for each device.
>> This allows guest to correctly fabricate stage-1 page tables.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> [Vivek: Use a single "struct virtio_iommu_probe_table_format" rather
>>          than separate structures for page table and pasid table format.
> 
> Makes sense. I've integrated that into the spec draft, added more precise
> documentation and modified some of the definitions.
> 
> The current draft is here:
> https://jpbrucker.net/virtio-iommu/spec/virtio-iommu-v0.13.pdf
> Posted on the list here
> https://lists.oasis-open.org/archives/virtio-dev/202102/msg00012.html

Thanks, I took an initial look, will review it this week.

> 
>> 	Also update commit message.]
>> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Michael S. Tsirkin <mst@redhat.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 | 44 ++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
>> index 237e36a280cb..43821e33e7af 100644
>> --- a/include/uapi/linux/virtio_iommu.h
>> +++ b/include/uapi/linux/virtio_iommu.h
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Virtio-iommu definition v0.12
>>    *
>> - * Copyright (C) 2019 Arm Ltd.
>> + * Copyright (C) 2019-2021 Arm Ltd.
> 
> Not strictly necessary. But if you're modifying this comment please also
> remove the "v0.12" above

Sure, let me keep the copyright year unchanged until we finalize the 
changes in draft spec.

> 
>>    */
>>   #ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
>>   #define _UAPI_LINUX_VIRTIO_IOMMU_H
>> @@ -111,6 +111,12 @@ struct virtio_iommu_req_unmap {
>>   
>>   #define VIRTIO_IOMMU_PROBE_T_NONE		0
>>   #define VIRTIO_IOMMU_PROBE_T_RESV_MEM		1
>> +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK	2
>> +#define VIRTIO_IOMMU_PROBE_T_INPUT_RANGE	3
>> +#define VIRTIO_IOMMU_PROBE_T_OUTPUT_SIZE	4
>> +#define VIRTIO_IOMMU_PROBE_T_PASID_SIZE		5
>> +#define VIRTIO_IOMMU_PROBE_T_PAGE_TABLE_FMT	6
>> +#define VIRTIO_IOMMU_PROBE_T_PASID_TABLE_FMT	7
> 
> Since there is a single struct we can have a single
> VIRTIO_IOMMU_PROBE_T_TABLE_FORMAT.

Right, that would make sense.

> 
>>   
>>   #define VIRTIO_IOMMU_PROBE_T_MASK		0xfff
>>   
>> @@ -130,6 +136,42 @@ struct virtio_iommu_probe_resv_mem {
>>   	__le64					end;
>>   };
>>   
>> +struct virtio_iommu_probe_page_size_mask {
>> +	struct virtio_iommu_probe_property	head;
>> +	__u8					reserved[4];
>> +	__le64					mask;
>> +};
>> +
>> +struct virtio_iommu_probe_input_range {
>> +	struct virtio_iommu_probe_property	head;
>> +	__u8					reserved[4];
>> +	__le64					start;
>> +	__le64					end;
>> +};
>> +
>> +struct virtio_iommu_probe_output_size {
>> +	struct virtio_iommu_probe_property	head;
>> +	__u8					bits;
>> +	__u8					reserved[3];
>> +};
>> +
>> +struct virtio_iommu_probe_pasid_size {
>> +	struct virtio_iommu_probe_property	head;
>> +	__u8					bits;
>> +	__u8					reserved[3];
>> +};
>> +
>> +/* Arm LPAE page table format */
>> +#define VIRTIO_IOMMU_FOMRAT_PGTF_ARM_LPAE	1
> 
> s/FOMRAT/FORMAT

Sure.

> 
>> +/* Arm smmu-v3 type PASID table format */
>> +#define VIRTIO_IOMMU_FORMAT_PSTF_ARM_SMMU_V3	2
> 
> These should be with the Arm-specific definitions patches 11 and 14

Right, will add these definitions with Arm specific patches.

Best regards
Vivek

[snip]