[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] gnttab: make sure grant map operations don't skip their IOMMU part
> -----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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |