| Message ID | 20210423095147.27922-11-vivek.gautam@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/virtio: vSVA support with Arm | expand |
On Fri, Apr 23, 2021 at 03:21:46PM +0530, Vivek Gautam wrote: > Once the page faults are handled, the response has to be sent to > virtio-iommu backend, from where it can be sent to the host to > prepare the response to a generated io page fault by the device. > Add a new virt-queue request type to handle this. > > Signed-off-by: Vivek Gautam <vivek.gautam@arm.com> > --- > include/uapi/linux/virtio_iommu.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h > index c12d9b6a7243..1b174b98663a 100644 > --- a/include/uapi/linux/virtio_iommu.h > +++ b/include/uapi/linux/virtio_iommu.h > @@ -48,6 +48,7 @@ struct virtio_iommu_config { > #define VIRTIO_IOMMU_T_PROBE 0x05 > #define VIRTIO_IOMMU_T_ATTACH_TABLE 0x06 > #define VIRTIO_IOMMU_T_INVALIDATE 0x07 > +#define VIRTIO_IOMMU_T_PAGE_RESP 0x08 > > /* Status types */ > #define VIRTIO_IOMMU_S_OK 0x00 > @@ -70,6 +71,23 @@ struct virtio_iommu_req_tail { > __u8 reserved[3]; > }; > > +struct virtio_iommu_req_page_resp { > + struct virtio_iommu_req_head head; > + __le32 domain; I don't think we need this field, since the fault report doesn't come with a domain. > + __le32 endpoint; > +#define VIRTIO_IOMMU_PAGE_RESP_PASID_VALID (1 << 0) To be consistent with the rest of the document this would be VIRTIO_IOMMU_PAGE_RESP_F_PASID_VALID > + __le32 flags; > + __le32 pasid; > + __le32 grpid; > +#define VIRTIO_IOMMU_PAGE_RESP_SUCCESS (0x0) > +#define VIRTIO_IOMMU_PAGE_RESP_INVALID (0x1) > +#define VIRTIO_IOMMU_PAGE_RESP_FAILURE (0x2) > + __le16 resp_code; > + __u8 pasid_valid; This field isn't needed since there already is VIRTIO_IOMMU_PAGE_RESP_PASID_VALID > + __u8 reserved[9]; > + struct virtio_iommu_req_tail tail; > +}; I'd align the size of the struct to 16 bytes, but I don't think that's strictly necessary. Thanks, Jean > + > struct virtio_iommu_req_attach { > struct virtio_iommu_req_head head; > __le32 domain; > -- > 2.17.1 >
Hi Jean, On 9/21/21 9:46 PM, Jean-Philippe Brucker wrote: > On Fri, Apr 23, 2021 at 03:21:46PM +0530, Vivek Gautam wrote: >> Once the page faults are handled, the response has to be sent to >> virtio-iommu backend, from where it can be sent to the host to >> prepare the response to a generated io page fault by the device. >> Add a new virt-queue request type to handle this. >> >> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com> >> --- >> include/uapi/linux/virtio_iommu.h | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h >> index c12d9b6a7243..1b174b98663a 100644 >> --- a/include/uapi/linux/virtio_iommu.h >> +++ b/include/uapi/linux/virtio_iommu.h >> @@ -48,6 +48,7 @@ struct virtio_iommu_config { >> #define VIRTIO_IOMMU_T_PROBE 0x05 >> #define VIRTIO_IOMMU_T_ATTACH_TABLE 0x06 >> #define VIRTIO_IOMMU_T_INVALIDATE 0x07 >> +#define VIRTIO_IOMMU_T_PAGE_RESP 0x08 >> >> /* Status types */ >> #define VIRTIO_IOMMU_S_OK 0x00 >> @@ -70,6 +71,23 @@ struct virtio_iommu_req_tail { >> __u8 reserved[3]; >> }; >> >> +struct virtio_iommu_req_page_resp { >> + struct virtio_iommu_req_head head; >> + __le32 domain; > > I don't think we need this field, since the fault report doesn't come with > a domain. But here we are sending the response which would be consumed by the vfio ultimately. In kvmtool, I am consuming this "virtio_iommu_req_page_resp" request in the virtio/iommu driver, extracting the domain from it, and using that to call the respective "page_response" ops from "vfio_iommu_ops" in the vfio/core driver. Is this incorrect way of passing on the page-response back to the host kernel? But I think this will have to be worked out with the /dev/iommu framework. > >> + __le32 endpoint; >> +#define VIRTIO_IOMMU_PAGE_RESP_PASID_VALID (1 << 0) > > To be consistent with the rest of the document this would be > VIRTIO_IOMMU_PAGE_RESP_F_PASID_VALID Sure, will update this. > >> + __le32 flags; >> + __le32 pasid; >> + __le32 grpid; >> +#define VIRTIO_IOMMU_PAGE_RESP_SUCCESS (0x0) >> +#define VIRTIO_IOMMU_PAGE_RESP_INVALID (0x1) >> +#define VIRTIO_IOMMU_PAGE_RESP_FAILURE (0x2) >> + __le16 resp_code; >> + __u8 pasid_valid; > > This field isn't needed since there already is > VIRTIO_IOMMU_PAGE_RESP_PASID_VALID Yes, sure will remove this field. > >> + __u8 reserved[9]; >> + struct virtio_iommu_req_tail tail; >> +}; > > I'd align the size of the struct to 16 bytes, but I don't think that's > strictly necessary. Will fix this. Thanks a lot for reviewing. Best regards Vivek > > Thanks, > Jean > >> + >> struct virtio_iommu_req_attach { >> struct virtio_iommu_req_head head; >> __le32 domain; >> -- >> 2.17.1 >>
On Thu, Sep 30, 2021 at 02:54:05PM +0530, Vivek Kumar Gautam wrote: > > > +struct virtio_iommu_req_page_resp { > > > + struct virtio_iommu_req_head head; > > > + __le32 domain; > > > > I don't think we need this field, since the fault report doesn't come with > > a domain. > > But here we are sending the response which would be consumed by the vfio > ultimately. In kvmtool, I am consuming this "virtio_iommu_req_page_resp" > request in the virtio/iommu driver, extracting the domain from it, and using > that to call the respective "page_response" ops from "vfio_iommu_ops" in the > vfio/core driver. > > Is this incorrect way of passing on the page-response back to the host > kernel? That works for the host userspace-kernel interface because the device is always attached to a VFIO container. For virtio-iommu the domain info is redundant. The endpoint information needs to be kept through the whole response path in order to target the right endpoint in the end. In addition the guest could enable PRI without attaching the endpoint to a domain, or fail to disable PRI before detaching the endpoint. Sure it's weird, but the host can still inject the recoverable page fault in this case, and the guest answers with "invalid" status but no domain. We could mandate domains for recoverable faults but that forces a synchronization against attach/detach and I think it needlessly deviates from other IOMMUs. Thanks, Jean
diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h index c12d9b6a7243..1b174b98663a 100644 --- a/include/uapi/linux/virtio_iommu.h +++ b/include/uapi/linux/virtio_iommu.h @@ -48,6 +48,7 @@ struct virtio_iommu_config { #define VIRTIO_IOMMU_T_PROBE 0x05 #define VIRTIO_IOMMU_T_ATTACH_TABLE 0x06 #define VIRTIO_IOMMU_T_INVALIDATE 0x07 +#define VIRTIO_IOMMU_T_PAGE_RESP 0x08 /* Status types */ #define VIRTIO_IOMMU_S_OK 0x00 @@ -70,6 +71,23 @@ struct virtio_iommu_req_tail { __u8 reserved[3]; }; +struct virtio_iommu_req_page_resp { + struct virtio_iommu_req_head head; + __le32 domain; + __le32 endpoint; +#define VIRTIO_IOMMU_PAGE_RESP_PASID_VALID (1 << 0) + __le32 flags; + __le32 pasid; + __le32 grpid; +#define VIRTIO_IOMMU_PAGE_RESP_SUCCESS (0x0) +#define VIRTIO_IOMMU_PAGE_RESP_INVALID (0x1) +#define VIRTIO_IOMMU_PAGE_RESP_FAILURE (0x2) + __le16 resp_code; + __u8 pasid_valid; + __u8 reserved[9]; + struct virtio_iommu_req_tail tail; +}; + struct virtio_iommu_req_attach { struct virtio_iommu_req_head head; __le32 domain;
Once the page faults are handled, the response has to be sent to virtio-iommu backend, from where it can be sent to the host to prepare the response to a generated io page fault by the device. Add a new virt-queue request type to handle this. Signed-off-by: Vivek Gautam <vivek.gautam@arm.com> --- include/uapi/linux/virtio_iommu.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)