Re: [Xen-devel] [PATCH 2/2] iommu: elide flushing for higher order map/unmap operations

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 03 December 2018 15:29
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Brian Woods <brian.woods@xxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [Xen-devel] [PATCH 2/2] iommu: elide flushing for higher
> order map/unmap operations
> 
> >>> On 03.12.18 at 16:18, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 03 December 2018 15:03
> >>
> >> >>> On 30.11.18 at 11:45, <paul.durrant@xxxxxxxxxx> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -631,11 +631,14 @@ static int __must_check
> iommu_flush_iotlb(struct
> >> domain *d, dfn_t dfn,
> >> >      return rc;
> >> >  }
> >> >
> >> > -static int __must_check iommu_flush_iotlb_pages(struct domain *d,
> >> > -                                                dfn_t dfn,
> >> > -                                                unsigned int
> >> page_count)
> >> > +static int __must_check iommu_flush_iotlb_pages(
> >> > +    struct domain *d, dfn_t dfn, unsigned int page_count,
> >> > +    enum iommu_flush_type flush_type)
> >>
> >> Is the re-flowing really needed?
> >>
> >
> > Yes. The enum is long and won't fit within 80 chars otherwise.
> 
> How about calling the parameter by a shorter name, e.g. ft?
>

Well I'm going to passing flags around now, so I may as well use an unsigned 
int.
 
> >> > @@ -674,9 +677,6 @@ static int __must_check dma_pte_clear_one(struct
> >> domain *domain, u64 addr)
> >> >      spin_unlock(&hd->arch.mapping_lock);
> >> >      iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
> >> >
> >> > -    if ( !this_cpu(iommu_dont_flush_iotlb) )
> >> > -        rc = iommu_flush_iotlb_pages(domain, daddr_to_dfn(addr), 1);
> >>
> >> This code not getting replaced by another addition right in this
> >> source file, and this function's only caller being
> >> intel_iommu_unmap_page() makes me wonder why you don't
> >> have the unmap functions similarly hand back a flush indicator.
> >
> > Well, the assumption is that unmap is always modifying an existing
> entry. Is
> > that assumption wrong?
> 
> I could certainly see an unmap happening for an already
> unmapped area.

Ok, I'll generalise it then.

  Paul

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel