RE: [PATCH 4/6] remove remaining uses of iommu_legacy_map/unmap

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 26 July 2020 09:36
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; 
> Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>; George
> Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; 
> Julien Grall
> <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Jun Nakajima 
> <jun.nakajima@xxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH 4/6] remove remaining uses of 
> iommu_legacy_map/unmap
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 24.07.2020 18:46, Paul Durrant wrote:
> > ---
> >  xen/arch/x86/mm.c               | 22 +++++++++++++++-----
> >  xen/arch/x86/mm/p2m-ept.c       | 22 +++++++++++++-------
> >  xen/arch/x86/mm/p2m-pt.c        | 17 +++++++++++----
> >  xen/arch/x86/mm/p2m.c           | 28 ++++++++++++++++++-------
> >  xen/arch/x86/x86_64/mm.c        | 27 ++++++++++++++++++------
> >  xen/common/grant_table.c        | 36 +++++++++++++++++++++++++-------
> >  xen/common/memory.c             |  7 +++----
> >  xen/drivers/passthrough/iommu.c | 37 +--------------------------------
> >  xen/include/xen/iommu.h         | 20 +++++-------------
> >  9 files changed, 123 insertions(+), 93 deletions(-)
> 
> Overall more code. I wonder whether a map-and-flush function (named
> differently than the current ones) wouldn't still be worthwhile to
> have.

Agreed but an extra 30 lines is not huge. I'd still like to keep map/unmap and 
flush separate but I'll see if I can reduce the added lines.

> 
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1225,11 +1225,25 @@ map_grant_ref(
> >              kind = IOMMUF_readable;
> >          else
> >              kind = 0;
> > -        if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind) )
> > +        if ( kind )
> >          {
> > -            double_gt_unlock(lgt, rgt);
> > -            rc = GNTST_general_error;
> > -            goto undo_out;
> > +            dfn_t dfn = _dfn(mfn_x(mfn));
> > +            unsigned int flush_flags = 0;
> > +            int err;
> > +
> > +            err = iommu_map(ld, dfn, mfn, 0, kind, &flush_flags);
> > +            if ( err )
> > +                rc = GNTST_general_error;
> > +
> > +            err = iommu_iotlb_flush(ld, dfn, 1, flush_flags);
> > +            if ( err )
> > +                rc = GNTST_general_error;
> > +
> > +            if ( rc != GNTST_okay )
> > +            {
> > +                double_gt_unlock(lgt, rgt);
> > +                goto undo_out;
> > +            }
> >          }
> 
> The mapping needs to happen with at least ld's lock held, yes. But
> is the same true also for the flushing? Can't (not necessarily
> right in this change) the flush be pulled out of the function and
> instead done once per batch that got processed?
> 

True, the locks need not be held across the flush. I'll have a look at batching 
too.

  Paul

> Jan