Re: [Qemu-devel] [Qemu-arm] [PATCH v4 0/5] virtio-iommu: VFIO integration

[Auger Eric <eric.auger@redhat.com>]

Hi Bharat,

On 10/10/2017 08:42, Bharat Bhushan wrote:
> Hi Alex, Eric,
> 
>> -----Original Message-----
>> From: Qemu-devel [mailto:qemu-devel-
>> bounces+bharat.bhushan=nxp.com@nongnu.org] On Behalf Of Bharat
>> Bhushan
>> Sent: Friday, October 06, 2017 9:16 AM
>> To: Auger Eric <eric.auger@redhat.com>; Linu Cherian
>> <linuc.decode@gmail.com>
>> Cc: peter.maydell@linaro.org; kevin.tian@intel.com; mst@redhat.com;
>> marc.zyngier@arm.com; tn@semihalf.com; will.deacon@arm.com;
>> drjones@redhat.com; qemu-devel@nongnu.org;
>> alex.williamson@redhat.com; qemu-arm@nongnu.org;
>> linu.cherian@cavium.com; eric.auger.pro@gmail.com;
>> robin.murphy@arm.com; christoffer.dall@linaro.org;
>> bharatb.yadav@gmail.com
>> Subject: Re: [Qemu-devel] [Qemu-arm] [PATCH v4 0/5] virtio-iommu: VFIO
>> integration
>>
>>
>>
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>>
>>>>>> However you should be allowed to map 1 sg element of 5 pages and
>>>>>> then notify the host about this event I think. Still looking at the code...
>>>>>>
>>>>>> I still can't reproduce the issue at the moment. What kind of
>>>>>> device are you assigning?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Eric
>>>>>>>
>>>>>>> Atleast vfio_get_vaddr called from vfio_iommu_map_notify in Qemu
>>>>>>> expects the map size to be a power of 2.
>>>>
>>>> Actually I missed the most important here ;-)
>>>>>>>
>>>>>>>  if (len & iotlb->addr_mask) {
>>>> This check looks suspiscious to me. In our case the len is not
>>>> modified by the previous translation and it fails, I don't see why.
>>>> It should be valid to be able to notify 5 granules.
>>>
>>> So after discussion with Alex, looks the way we notify the host
>>> currently is wrong. we set the addr_mask to the mapping/unmapping size
>>> -1 whereas this should be a page mask instead (granule size or block size?).
>>> So if the guest maps 5 x 4kB pages we should send 5 notifications for
>>> each page and not a single one. It is unclear to me if we can notify
>>> with hugepage/block page size mask. Peter may confirm/infirm this. in
>>> vsmmuv3 code I notify by granule or block size.
> 
> My understanding is that host provides supported page sizes (page_size_mask), and Size of each notification to host should be exactly best fit of supported page-size and/or multiples of supported page-sizes.
> So if guest maps 20K size (single request), and supported page size is 4K, so we can still send one 20K size request.
> Not sure I get it, why multiples of supported page-size cannot be provided in one notification to host.
I think the IOTLB API was originally devised to manage only granule or
block ^2 sizes. We might change this in the future but for the moment,
with respect to this series,  I would simply recommend to stick to the
existing API limitation and concurrently we can work on relaxing this
constraint on another series.

Thanks

Eric
> 
> Thanks
> -Bharat
> 
>>>
>>> Bharat, please can you add this to your TODO list?
>>>
>>> Linu, thanks a lot for the time you spent debugging this issue.
>>> Curiously on my side, it is really seldom hit but it is ...
>>
>> Thanks Linu and Eric, I added this to my todo list.
>> While I am still not able to reproduce the issue.  I tried with e1000 and now
>> try with ixgbe device. May I know which device can be used to reproduce this
>> issue?
>>
>> Thanks
>> -Bharat
>>
>>>
>>> Thanks!
>>>
>>> Eric
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>>>>         error_report("iommu has granularity incompatible with target
>> AS");
>>>>>>>         return false;
>>>>>>>     }
>>>>>>>
>>>>>>> Just trying to understand how this is not hitting in your case.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>