Re: [Xen-devel] [PATCH] gnttab: make sure grant map operations don't skip their IOMMU part

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

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan
> Beulich
> Sent: 21 November 2019 17:38
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Konrad Wilk <konrad.wilk@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Ian Jackson <ian.jackson@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH] gnttab: make sure grant map operations don't
> skip their IOMMU part
> 
> Two almost simultaneous mapping requests need to make sure that at the
> completion of the earlier one IOMMU mappings (established explicitly
> here in the PV case) have been put in place. Forever since the splitting
> of the grant table lock a violation of this has been possible (using
> simplified pin counts, as it doesn't matter whether we talk about read
> or write mappings here):
> 
> initial state: act->pin = 0
> 
> vCPU A: progress the operation past the dropping of the locks after the
>         act->pin updates (act->pin = 1, old_pin = 0, act_pin = 1)
> 
> vCPU B: progress the operation past the dropping of the locks after the
>         act->pin updates (act->pin = 2, old_pin = 1, act_pin = 2)
> 
> vCPU B: (re-)acquire both gt locks, mapkind() returns 0, but both
>         iommu_legacy_map() invocations get skipped due to non-zero
>         old_pin
> 
> vCPU B: return to caller without IOMMU mapping
> 
> vCPU A: (re-)acquire both gt locks, mapkind() returns 0,
>         iommu_legacy_map() gets invoked
> 
> With the locks dropped intermediately, whether to invoke
> iommu_legacy_map() must depend on only the return value of mapkind()
> and of course the kind of mapping request being processed, just like
> is already the case in unmap_common().
> 
> Also fix the style of the adjacent comment, and correct a nearby one
> still referring to a prior name of what is now mapkind().
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -917,8 +917,6 @@ map_grant_ref(
>      mfn_t mfn;
>      struct page_info *pg = NULL;
>      int            rc = GNTST_okay;
> -    u32            old_pin;
> -    u32            act_pin;
>      unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
>      bool           host_map_created = false;
>      struct active_grant_entry *act = NULL;
> @@ -1027,7 +1025,6 @@ map_grant_ref(
>          }
>      }
> 
> -    old_pin = act->pin;
>      if ( op->flags & GNTMAP_device_map )
>          act->pin += (op->flags & GNTMAP_readonly) ?
>              GNTPIN_devr_inc : GNTPIN_devw_inc;
> @@ -1036,7 +1033,6 @@ map_grant_ref(
>              GNTPIN_hstr_inc : GNTPIN_hstw_inc;
> 
>      mfn = act->mfn;
> -    act_pin = act->pin;
> 
>      cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
> 
> @@ -1144,27 +1140,22 @@ map_grant_ref(
>      if ( need_iommu )
>      {
>          unsigned int kind;
> -        int err = 0;
> 
>          double_gt_lock(lgt, rgt);
> 
> -        /* We're not translated, so we know that gmfns and mfns are
> -           the same things, so the IOMMU entry is always 1-to-1. */
> +        /*
> +         * We're not translated, so we know that dfns and mfns are
> +         * the same things, so the IOMMU entry is always 1-to-1.
> +         */
>          kind = mapkind(lgt, rd, mfn);
> -        if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
> -             !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
> -        {
> -            if ( !(kind & MAPKIND_WRITE) )
> -                err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
> -                                       IOMMUF_readable |
> IOMMUF_writable);
> -        }
> -        else if ( act_pin && !old_pin )
> -        {
> -            if ( !kind )
> -                err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
> -                                       IOMMUF_readable);
> -        }
> -        if ( err )
> +        if ( !(op->flags & GNTMAP_readonly) &&
> +             !(kind & MAPKIND_WRITE) )
> +            kind = IOMMUF_readable | IOMMUF_writable;
> +        else if ( !kind )
> +            kind = IOMMUF_readable;
> +        else
> +            kind = 0;
> +        if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind)

Re-using 'kind' in this way slightly obfuscates things. I'm sure the compiler 
would still generate reasonable code if you used a separate 'flags' variable 
within the same scope.

  Paul

> )
>          {
>              double_gt_unlock(lgt, rgt);
>              rc = GNTST_general_error;
> @@ -1179,7 +1170,7 @@ map_grant_ref(
>       * other fields so just ensure the flags field is stored last.
>       *
>       * However, if gnttab_need_iommu_mapping() then this would race
> -     * with a concurrent mapcount() call (on an unmap, for example)
> +     * with a concurrent mapkind() call (on an unmap, for example)
>       * and a lock is required.
>       */
>      mt = &maptrack_entry(lgt, handle);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel