Messages in this thread | | | Subject | Re: [PATCH RFC v1 10/11] uapi/virtio-iommu: Add a new request type to send page response | From | Vivek Kumar Gautam <> | Date | Thu, 30 Sep 2021 14:54:05 +0530 |
| |
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 >>
| |