Messages in this thread | | | Subject | Re: [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information | From | Vivek Kumar Gautam <> | Date | Thu, 30 Sep 2021 10:26:35 +0530 |
| |
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
| |