| Message ID | e698a39a-f9fc-18d7-07cd-d0b9da9c0eae@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 01/12/2021 11:52, Jan Beulich wrote: > --- a/xen/arch/x86/mm/p2m-pod.c > +++ b/xen/arch/x86/mm/p2m-pod.c > @@ -1345,19 +1345,15 @@ mark_populate_on_demand(struct domain *d > } > } > > + pod_lock(p2m); > + p2m->pod.entry_count += (1UL << order) - pod_count; > + pod_unlock(p2m); > + > /* Now, actually do the two-way mapping */ > rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, > p2m_populate_on_demand, p2m->default_access); > if ( rc == 0 ) > - { > - pod_lock(p2m); > - p2m->pod.entry_count += 1UL << order; > - p2m->pod.entry_count -= pod_count; > - BUG_ON(p2m->pod.entry_count < 0); > - pod_unlock(p2m); > - > ioreq_request_mapcache_invalidate(d); > - } > else if ( order ) > { > /* > @@ -1369,6 +1365,13 @@ mark_populate_on_demand(struct domain *d > d, gfn_l, order, rc); > domain_crash(d); > } > + else if ( !pod_count ) > + { > + pod_lock(p2m); > + BUG_ON(!p2m->pod.entry_count); > + --p2m->pod.entry_count; > + pod_unlock(p2m); > + } This is very confusing logic to follow at the best of times. Given that the p2m is already locked, what's wrong with holding the pod_lock() across p2m_set_entry()? At the very least, this needs some comments helping to explain why these two hunks are undo's of each other, given how differently they're written. ~Andrew
--- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -1345,19 +1345,15 @@ mark_populate_on_demand(struct domain *d } } + pod_lock(p2m); + p2m->pod.entry_count += (1UL << order) - pod_count; + pod_unlock(p2m); + /* Now, actually do the two-way mapping */ rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_populate_on_demand, p2m->default_access); if ( rc == 0 ) - { - pod_lock(p2m); - p2m->pod.entry_count += 1UL << order; - p2m->pod.entry_count -= pod_count; - BUG_ON(p2m->pod.entry_count < 0); - pod_unlock(p2m); - ioreq_request_mapcache_invalidate(d); - } else if ( order ) { /* @@ -1369,6 +1365,13 @@ mark_populate_on_demand(struct domain *d d, gfn_l, order, rc); domain_crash(d); } + else if ( !pod_count ) + { + pod_lock(p2m); + BUG_ON(!p2m->pod.entry_count); + --p2m->pod.entry_count; + pod_unlock(p2m); + } out: gfn_unlock(p2m, gfn, order);
When not holding the PoD lock across the entire region covering P2M update and stats update, the entry count should indicate too large a value in preference to a too small one, to avoid functions bailing early when they find the count is zero. Hence increments should happen ahead of P2M updates, while decrements should happen only after. Deal with the one place where this hasn't been the case yet. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Resending due to the original submission having had the same contents twice.