1: fix PoD accounting in guest_physmap_add_entry() 2: adjust non-PoD accounting in p2m_pod_decrease_reservation() Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
The initial observation was that the mfn_valid() check comes too late:
Neither mfn_add() nor mfn_to_page() (let alone de-referencing the
result of the latter) are valid for MFNs failing this check. Move it up
and - noticing that there's no caller doing so - also add an assertion
that this should never produce "false" here.
In turn this would have meant that the "else" to that if() could now go
away, which didn't seem right at all. And indeed, considering callers
like memory_exchange() or various grant table functions, the PoD
accounting should have been outside of that if() from the very
beginning.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -883,6 +883,12 @@ guest_physmap_add_entry(struct domain *d
if ( p2m_is_foreign(t) )
return -EINVAL;
+ if ( !mfn_valid(mfn) )
+ {
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+
p2m_lock(p2m);
P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn_x(gfn), mfn_x(mfn));
@@ -984,12 +990,13 @@ guest_physmap_add_entry(struct domain *d
}
/* Now, actually do the two-way mapping */
- if ( mfn_valid(mfn) )
+ rc = p2m_set_entry(p2m, gfn, mfn, page_order, t, p2m->default_access);
+ if ( rc == 0 )
{
- rc = p2m_set_entry(p2m, gfn, mfn, page_order, t,
- p2m->default_access);
- if ( rc )
- goto out; /* Failed to update p2m, bail without updating m2p. */
+ pod_lock(p2m);
+ p2m->pod.entry_count -= pod_count;
+ BUG_ON(p2m->pod.entry_count < 0);
+ pod_unlock(p2m);
if ( !p2m_is_grant(t) )
{
@@ -998,20 +1005,6 @@ guest_physmap_add_entry(struct domain *d
gfn_x(gfn_add(gfn, i)));
}
}
- else
- {
- gdprintk(XENLOG_WARNING, "Adding bad mfn to p2m map (%#lx -> %#lx)\n",
- gfn_x(gfn), mfn_x(mfn));
- rc = p2m_set_entry(p2m, gfn, INVALID_MFN, page_order,
- p2m_invalid, p2m->default_access);
- if ( rc == 0 )
- {
- pod_lock(p2m);
- p2m->pod.entry_count -= pod_count;
- BUG_ON(p2m->pod.entry_count < 0);
- pod_unlock(p2m);
- }
- }
out:
p2m_unlock(p2m);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23/01/2020 11:51, Jan Beulich wrote: > The initial observation was that the mfn_valid() check comes too late: > Neither mfn_add() nor mfn_to_page() (let alone de-referencing the > result of the latter) are valid for MFNs failing this check. Move it up > and - noticing that there's no caller doing so - also add an assertion > that this should never produce "false" here. > > In turn this would have meant that the "else" to that if() could now go > away, which didn't seem right at all. And indeed, considering callers > like memory_exchange() or various grant table functions, the PoD > accounting should have been outside of that if() from the very > beginning. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Throughout the function the equation
pod + nonpod == (1UL << order)
should hold. This has been violated by the final loop of the function:
* changing a range from a type other than p2m_populate_on_demand to
p2m_invalid doesn't alter the amount of non-PoD pages in the region,
* changing a range from p2m_populate_on_demand to p2m_invalid does
increase the amount of non-PoD pages in the region along with
decreasing the amount of PoD pages there.
Fortunately the variable isn't used anymore after the loop. Instead of
correcting the updating of the "nonpod" variable, however, drop it
altogether, to avoid getting the above equation to not hold again by a
future change.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -509,7 +509,7 @@ p2m_pod_decrease_reservation(struct doma
unsigned long ret = 0, i, n;
struct p2m_domain *p2m = p2m_get_hostp2m(d);
bool_t steal_for_cache;
- long pod, nonpod, ram;
+ long pod = 0, ram = 0;
gfn_lock(p2m, gfn, order);
pod_lock(p2m);
@@ -524,8 +524,6 @@ p2m_pod_decrease_reservation(struct doma
if ( unlikely(d->is_dying) )
goto out_unlock;
- pod = nonpod = ram = 0;
-
/* Figure out if we need to steal some freed memory for our cache */
steal_for_cache = ( p2m->pod.entry_count > p2m->pod.count );
@@ -539,19 +537,15 @@ p2m_pod_decrease_reservation(struct doma
n = 1UL << min(order, cur_order);
if ( t == p2m_populate_on_demand )
pod += n;
- else
- {
- nonpod += n;
- if ( p2m_is_ram(t) )
- ram += n;
- }
+ else if ( p2m_is_ram(t) )
+ ram += n;
}
/* No populate-on-demand? Don't need to steal anything? Then we're done!*/
if ( !pod && !steal_for_cache )
goto out_unlock;
- if ( !nonpod )
+ if ( i == pod )
{
/*
* All PoD: Mark the whole region invalid and tell caller
@@ -587,7 +581,7 @@ p2m_pod_decrease_reservation(struct doma
p2m_pod_zero_check_superpage(p2m, _gfn(gfn_x(gfn) & ~(SUPERPAGE_PAGES - 1))) )
{
pod = 1UL << order;
- ram = nonpod = 0;
+ ram = 0;
ASSERT(steal_for_cache == (p2m->pod.entry_count > p2m->pod.count));
}
@@ -655,7 +649,6 @@ p2m_pod_decrease_reservation(struct doma
steal_for_cache = ( p2m->pod.entry_count > p2m->pod.count );
- nonpod -= n;
ram -= n;
ret += n;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23/01/2020 11:51, Jan Beulich wrote: > Throughout the function the equation > > pod + nonpod == (1UL << order) > > should hold. This has been violated by the final loop of the function: > * changing a range from a type other than p2m_populate_on_demand to > p2m_invalid doesn't alter the amount of non-PoD pages in the region, > * changing a range from p2m_populate_on_demand to p2m_invalid does > increase the amount of non-PoD pages in the region along with > decreasing the amount of PoD pages there. > Fortunately the variable isn't used anymore after the loop. Instead of > correcting the updating of the "nonpod" variable, however, drop it > altogether, to avoid getting the above equation to not hold again by a > future change. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23.01.2020 12:49, Jan Beulich wrote: > 1: fix PoD accounting in guest_physmap_add_entry() > 2: adjust non-PoD accounting in p2m_pod_decrease_reservation() George? Thanks, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.