| Message ID | cce7c12e-907b-bc1b-5092-a1d7874c099a@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gnttab: make sure grant map operations don't skip their IOMMU part | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan > Beulich > Sent: 21 November 2019 17:38 > To: xen-devel@lists.xenproject.org > Cc: Juergen Gross <jgross@suse.com>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu > <wl@xen.org>; Konrad Wilk <konrad.wilk@oracle.com>; George Dunlap > <George.Dunlap@eu.citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Ian Jackson <ian.jackson@citrix.com> > 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@suse.com> > > --- 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@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 22/11/2019 11:22, Durrant, Paul wrote: >> @@ -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. I agree. Everything LGTM, so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- 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) ) { 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);
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@suse.com>