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

[Peter Xu <peterx@redhat.com>]

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/qemu-devel@nongnu.org/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 <yan.y.zhao@intel.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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?

[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... :)

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

Thanks,

-- 
Peter Xu