> -Original Message-
> From: Xen-devel On Behalf Of Jan
> Beulich
> Sent: 21 November 2019 17:38
> To: xen-devel@lists.xenproject.org
> Cc: Juergen Gross ; Stefano Stabellini
> ; Julien Grall ; Wei Liu
> ; Konrad Wilk ; George Dunlap
> ; Andrew Cooper ;
> Ian Jackson
> 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
>
> --- 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;
> intrc = GNTST_okay;
> -u32old_pin;
> -u32act_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 = _entry(lgt, handle);
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
>
