linux-kernel - Re: [PATCH RFC v1 10/11] uapi/virtio-iommu: Add a new request type to send page response

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d40ea85b-3612-10b3-0add-40d07e6d9ca5@arm.com>
Date:   Thu, 30 Sep 2021 14:54:05 +0530
From:   Vivek Kumar Gautam <vivek.gautam@....com>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux-foundation.org,
        virtualization@...ts.linux-foundation.org, joro@...tes.org,
        will.deacon@....com, mst@...hat.com, robin.murphy@....com,
        eric.auger@...hat.com, kevin.tian@...el.com,
        jacob.jun.pan@...ux.intel.com, yi.l.liu@...el.com,
        Lorenzo.Pieralisi@....com, shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH RFC v1 10/11] uapi/virtio-iommu: Add a new request type to
 send page response

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@....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
>>