Re: [PATCH v2 3/4] iommu: elide flushing for higher order map/unmap operations

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.12.18 at 16:36, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 04 December 2018 15:17
>> 
>> >>> On 03.12.18 at 18:40, <paul.durrant@citrix.com> 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.

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

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

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

Jan


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