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

Vivek Kumar Gautam
2021-03-12T14:06:59Z

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]