Re: [PATCH v7 3/5] virtio-iommu: Call iommu notifier for attach/detach

[Auger Eric]

Hi Bharat,

On 3/17/20 8:10 AM, Bharat Bhushan wrote:
> Hi Jean,
> 
> On Mon, Mar 16, 2020 at 3:41 PM Jean-Philippe Brucker
> <address@hidden> wrote:
>>
>> Hi Bharat,
>>
>> Could you Cc me on your next posting?  Unfortunately I don't have much
>> hardware for testing this at the moment, but I might be able to help a
>> little on the review.
>>
>> On Mon, Mar 16, 2020 at 02:40:00PM +0530, Bharat Bhushan wrote:
>>>>>>> First issue is: your guest can use 4K page and your host can use 64KB
>>>>>>> pages. In that case VFIO_DMA_MAP will fail with -EINVAL. We must devise
>>>>>>> a way to pass the host settings to the VIRTIO-IOMMU device.
>>>>>>>
>>>>>>> Even with 64KB pages, it did not work for me. I have obviously not the
>>>>>>> storm of VFIO_DMA_MAP failures but I have some, most probably due to
>>>>>>> some wrong notifications somewhere. I will try to investigate on my 
>>>>>>> side.
>>>>>>>
>>>>>>> Did you test with VFIO on your side?
>>>>>>
>>>>>> I did not tried with different page sizes, only tested with 4K page size.
>>>>>>
>>>>>> Yes it works, I tested with two n/w device assigned to VM, both 
>>>>>> interfaces works
>>>>>>
>>>>>> First I will try with 64k page size.
>>>>>
>>>>> 64K page size does not work for me as well,
>>>>>
>>>>> I think we are not passing correct page_size_mask here
>>>>> (config.page_size_mask is set to TARGET_PAGE_MASK ( which is
>>>>> 0xfffffffffffff000))
>>>> I guess you mean with guest using 4K and host using 64K.
>>>>>
>>>>> We need to set this correctly as per host page size, correct?
>>>> Yes that's correct. We need to put in place a control path to retrieve
>>>> the page settings on host through VFIO to inform the virtio-iommu device.
>>>>
>>>> Besides this issue, did you try with 64kB on host and guest?
>>>
>>> I tried Followings
>>>   - 4k host and 4k guest  - it works with v7 version
>>>   - 64k host and 64k guest - it does not work with v7
>>>     hard-coded config.page_size_mask to 0xffffffffffff0000 and it works
>>
>> You might get this from the iova_pgsize bitmap returned by
>> VFIO_IOMMU_GET_INFO. The virtio config.page_size_mask is global so there
>> is the usual problem of aggregating consistent properties, but I'm
>> guessing using the host page size as a granule here is safe enough.
>>
>> If it is a problem, we can add a PROBE property for page size mask,
>> allowing to define per-endpoint page masks. I have kernel patches
>> somewhere to do just that.
> 
> I do not see we need page size mask per endpoint.
the physical devices can be protected with different physical IOMMUs and
they may have different page size support
> 
> While I am trying to understand what "page-size-mask" guest will work with
> 
> - 4K page size host and 4k page size guest
>   config.page_size_mask = 0xffffffffffff000 will work
> 
> - 64K page size host and 64k page size guest
>   config.page_size_mask = 0xfffffffffff0000 will work
I guess not all the pages sizes should be exposed by the virtio-iommu
device, only 4K and 64K

If host supports 4K we should expose 4K and bigger
If host supports 64K we should expose page sizes of 64KB and bigger

The guest will be forced to use what is exposed and that should work.

What is missing is a way to retrieve the host supported page size
bitmask. I can try to help you on that if you want to.

Maybe we should first try to upstream vhost support and then VFIO?

Thanks

Eric
> 
> - 64K page size host and 4k page size guest
>    1) config.page_size_mask = 0xffffffffffff000 will also not work as
> VFIO in host expect iova and size to be aligned to 64k (PAGE_SIZE in
> host)
>    2) config.page_size_mask = 0xfffffffffff0000 will not work, iova
> initialization (in guest) expect minimum page-size supported by h/w to
> be equal to 4k (PAGE_SIZE in guest)
>        Should we look to relax this in iova allocation code?
> 
> Thanks
> -Bharat
> 
> 
>>
>> Thanks,
>> Jean
>