Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Auger Eric]

Hi Peter,

On 2/18/21 6:48 PM, Peter Xu wrote:
> On Thu, Feb 18, 2021 at 06:18:22PM +0100, Auger Eric wrote:
>> Hi Peter,
>>
>> On 2/18/21 5:42 PM, Peter Xu wrote:
>>> Eric,
>>>
>>> On Thu, Feb 18, 2021 at 03:16:50PM +0100, Eric Auger wrote:
>>>> @@ -164,12 +166,27 @@ static void 
>>>> virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start,
>>>>  
>>>>      event.type = IOMMU_NOTIFIER_UNMAP;
>>>>      event.entry.target_as = &address_space_memory;
>>>> -    event.entry.addr_mask = virt_end - virt_start;
>>>> -    event.entry.iova = virt_start;
>>>>      event.entry.perm = IOMMU_NONE;
>>>>      event.entry.translated_addr = 0;
>>>> +    event.entry.addr_mask = mask;
>>>> +    event.entry.iova = virt_start;
>>>>  
>>>> -    memory_region_notify_iommu(mr, 0, event);
>>>> +    if (mask == UINT64_MAX) {
>>>> +        memory_region_notify_iommu(mr, 0, event);
>>>> +    }
>>>> +
>>>> +    size = mask + 1;
>>>> +
>>>> +    while (size) {
>>>> +        uint8_t highest_bit = 64 - clz64(size) - 1;
>>>
>>> I'm not sure fetching highest bit would work right. E.g., with start=0x11000
>>> and size=0x11000 (then we need to unmap 0x11000-0x22000), current code will
>>> first try to invalidate range (0x11000, 0x10000), that seems still invalid
>>> since 0x11000 is not aligned to 0x10000 page mask.
>>
>> Hum I thought aligning the size was sufficient. Where is it checked exactly?
> 
> I don't remember all the context either.. :)
> 
> Firstly - It makes sense to do that since hardware does it, and emulation code
> would make sense to follow that.>
> There's some more info where I looked into the src of when vt-d got introduced
> with the similar change:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> 
> I'm not 100% certain anything will break if we don't use page mask but
> arbitrary length as vhost did in iotlb msg.  However what Yan Zhao reported is
> definitely worse than that since we (vt-d) used to unmap outside the range of
> the range just for page mask alignment.

OK. I read again the get_naturally_aligned_size() code and indeed it
matches the need while taking into account the largest page size bigger
than the start @ alignment. This is a common pattern anyway. I am not
sure either this would break with vhost and vfio notifications but
better to use that function too in virtio-iommu and smmu (I sent a
similar patch for smmu).

Thank you for reminding me of that piece of code!

Eric
> 
> Thanks,
>