> -----Original Message-----
> From: Xen-devel [mailto:[email protected]] On Behalf
> Of Jan Beulich
> Sent: 04 December 2018 16:02
> To: Paul Durrant <[email protected]>
> Cc: Kevin Tian <[email protected]>; Stefano Stabellini
> <[email protected]>; Wei Liu <[email protected]>; Konrad Rzeszutek
> Wilk <[email protected]>; Andrew Cooper <[email protected]>;
> Tim (Xen.org) <[email protected]>; George Dunlap <[email protected]>;
> Julien Grall <[email protected]>; Suravee Suthikulpanit
> <[email protected]>; xen-devel <xen-
> [email protected]>; Ian Jackson <[email protected]>; Brian
> Woods <[email protected]>; Roger Pau Monne <[email protected]>
> Subject: Re: [Xen-devel] [PATCH v2 3/4] iommu: elide flushing for higher
> order map/unmap operations
>
> >>> On 04.12.18 at 16:36, <[email protected]> wrote:
> >> From: Jan Beulich [mailto:[email protected]]
> >> Sent: 04 December 2018 15:17
> >>
> >> >>> On 03.12.18 at 18:40, <[email protected]> wrote:
> >> > --- a/xen/arch/arm/p2m.c
> >> > +++ b/xen/arch/arm/p2m.c
> >> > @@ -971,8 +971,17 @@ static int __p2m_set_entry(struct p2m_domain
> *p2m,
> >> >
> >> > if ( need_iommu_pt_sync(p2m->domain) &&
> >> > (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
> >> > + {
> >> > + unsigned int flush_flags = 0;
> >> > +
> >> > + if ( lpae_is_valid(orig_pte) )
> >> > + flush_flags |= IOMMU_FLUSHF_modified;
> >> > + if ( lpae_is_valid(*entry) )
> >> > + flush_flags |= IOMMU_FLUSHF_added;
> >>
> >> Shouldn't this be "else if" with the meaning assigned to both
> >> types? From an abstract perspective I'd also expect that for
> >> a single mapping no more than one of the flags can come
> >> back set (through the iommu_ops interface).
> >
> > That's not how I see it. My rationale is:
> >
> > - present PTE made non-present, or modified -> IOMMU_FLUSHF_modified
> > - new PTE value is present -> IOMMU_FLUSHF_added
> >
> > So, a single op can set any combination of bits and thus the above code
> does
> > not use 'else if'.
>
> I can't fit this with the code comments:
>
> enum
> {
> _IOMMU_FLUSHF_added, /* no modified entries, just additional entries
> */
> _IOMMU_FLUSHF_modified, /* modified entries */
> };
>
> ..., in particular the "no modified entries" part.
That was supposed to emphasize the need for the other flag was needed the case
of a mapping that modifies an existing entry... I'll re-state using a block
comment.
>
> >> > @@ -84,7 +86,7 @@ static bool set_iommu_pde_present(uint32_t *pde,
> >> unsigned long next_mfn,
> >> >
> >> > if ( maddr_old != maddr_next || iw != old_w || ir != old_r
> ||
> >> > old_level != next_level )
> >> > - need_flush = true;
> >> > + flush_flags = IOMMU_FLUSHF_modified;
> >>
> >> Why uniformly "modified"?
> >
> > Because the AMD IOMMU does require flushing for a non-present -> present
> > transition AFAICT. The old code certainly implies this.
>
> It is one thing what the flush function does with the value, but
> another whether the modifying function "lies". I'm not opposed
> to simplification, but then a comment needs to explain this.
>
Ok. Maybe it is simpler not to omit requesting the 'added' flush here and then
just ignore it in the flush method.
> >> > @@ -235,6 +236,9 @@ void __hwdom_init iommu_hwdom_init(struct domain
> *d)
> >> > process_pending_softirqs();
> >> > }
> >> >
> >> > + while ( !flush_flags && iommu_flush_all(d) )
> >> > + break;
> >>
> >> Is there a comment missing here explaining the seemingly odd
> >> loop?
> >
> > I'm merely using the construct you suggested, but I can add a comment.
>
> And I'm fine with the construct, but in the other place (for which
> we did discuss this for the earlier version) there is a comment.
>
Ok. I'll add a similar comment.
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -633,11 +633,14 @@ static int __must_check
> iommu_flush_iotlb(struct
> >> domain *d, dfn_t dfn,
> >> >
> >> > static int __must_check iommu_flush_iotlb_pages(struct domain *d,
> >> > dfn_t dfn,
> >> > - unsigned int
> >> page_count)
> >> > + unsigned int
> >> page_count,
> >> > + unsigned int
> >> flush_flags)
> >> > {
> >> > ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> >> > + ASSERT(flush_flags);
> >> >
> >> > - return iommu_flush_iotlb(d, dfn, 1, page_count);
> >> > + return iommu_flush_iotlb(d, dfn, flush_flags &
> >> IOMMU_FLUSHF_modified,
> >> > + page_count);
> >>
> >> Why the restriction to "modified"?
> >
> > The parameter is a bool which should be true if an existing PTE was
> modified
> > or false otherwise. I can make this !!(flush_flags &
> IOMMU_FLUSHF_modified) is
> > you prefer.
>
> No, that wasn't my point. The question is why this isn't just
> "flush_flags", without any masking. Iirc there are precautions
> in the VT-d code to deal with hardware which may cache
> non-present entries. In that case "added" requires flushing too.
>
I don't understand. iommu_flush_iotlb()'s third argument is
'dma_old_pte_present' so that should be true iff IOMMU_FLUSHF_modified is set.
IOMMU_FLUSHF_added is irrelevant to the implementation.
Paul
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
