Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Auger Eric]

On 6/24/19 1:10 PM, Peter Xu wrote:
> On Mon, Jun 24, 2019 at 12:09:48PM +0200, Auger Eric wrote:
>> Hi Peter,
>>
>> On 6/24/19 11:18 AM, Peter Xu wrote:
>>> This is an replacement work of Yan Zhao's patch:
>>>
>>> https://www.mail-archive.com/address@hidden/msg625340.html
>>>
>>> vtd_address_space_unmap() will do proper page mask alignment to make
>>> sure each IOTLB message will have correct masks for notification
>>> messages (2^N-1), but sometimes it can be expanded to even supercede
>>> the registered range.  That could lead to unexpected UNMAP of already
>>> mapped regions in some other notifiers.
>>>
>>> Instead of doing mindless expension of the start address and address
>>> mask, we split the range into smaller ones and guarantee that each
>>> small range will have correct masks (2^N-1) and at the same time we
>>> should also try our best to generate as less IOTLB messages as
>>> possible.
>>>
>>> Reported-by: Yan Zhao <address@hidden>
>>> Signed-off-by: Peter Xu <address@hidden>
>>> ---
>>>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>>>  1 file changed, 41 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 719ce19ab3..de86f53b4e 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState 
>>> *s, PCIBus *bus, int devfn)
>>>      return vtd_dev_as;
>>>  }
>>>  
>>> +static uint64_t get_naturally_aligned_size(uint64_t start,
>>> +                                           uint64_t size, int gaw)
>>> +{
>>> +    uint64_t max_mask = 1ULL << gaw;
>>> +    uint64_t alignment = start ? start & -start : max_mask;
>>> +
>>> +    alignment = MIN(alignment, max_mask);
>>> +    size = MIN(size, max_mask);
>> this does not not prevent from invalidating beyond gaw if start != 0, right?
> 
> Yes.  But at the start of vtd_address_space_unmap(), we have:
> 
>     if (end > VTD_ADDRESS_SIZE(s->aw_bits) - 1) {
>         /*
>          * Don't need to unmap regions that is bigger than the whole
>          * VT-d supported address space size
>          */
>         end = VTD_ADDRESS_SIZE(s->aw_bits) - 1;
>     }
> 
> So we don't need to worry about (start+size) exceeding GAW?
Hum yes. Reviewed the previous patch with blinkers ...
> 
> [1]
> 
>>> +
>>> +    if (alignment <= size) {
>>> +        /* Increase the alignment of start */
>> I don't really get this comment
> 
> This comment comes from Paolo, but I'll try to explain - it tries to
> mean that this "alignment" will be used as an increasement to "start"
> variable, so finally variable "start" will align with larger mask
> size.
> 
> Better comments welcomed... :)
smallest page mask from @start or gaw?
> 
>>> +        return alignment;
>>> +    } else {
>>> +        /* Find the largest page mask from size */
>>> +        return 1ULL << (63 - clz64(size));
>>> +    }> +}
>>> +
>>>  /* Unmap the whole range in the notifier's scope. */
>>>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>>>  {
>>> -    IOMMUTLBEntry entry;
>>> -    hwaddr size;
>>> +    hwaddr size, remain;
>>>      hwaddr start = n->start;
>>>      hwaddr end = n->end;
>>>      IntelIOMMUState *s = as->iommu_state;
>>> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace 
>>> *as, IOMMUNotifier *n)
>>>      }
>>>  
>>>      assert(start <= end);
>>> -    size = end - start;
>>> +    size = remain = end - start + 1;
>>>  
>>> -    if (ctpop64(size) != 1) {
>>> -        /*
>>> -         * This size cannot format a correct mask. Let's enlarge it to
>>> -         * suite the minimum available mask.
>>> -         */
>>> -        int n = 64 - clz64(size);
>>> -        if (n > s->aw_bits) {
>>> -            /* should not happen, but in case it happens, limit it */
>>> -            n = s->aw_bits;
>>> -        }
>>> -        size = 1ULL << n;
>>> +    while (remain >= VTD_PAGE_SIZE) {
>> Can't we stop as soon as entry.iova exceeds gaw as well?
> 
> As explained at [1], I think we've already checked it.
OK

Thanks

Eric
> 
> Thanks,
>