* [Xen-devel] [PATCH 0/2] x86/p2m: PoD accounting adjustments
@ 2020-01-23 11:49 Jan Beulich
2020-01-23 11:51 ` [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry() Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2020-01-23 11:49 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry()
2020-01-23 11:49 [Xen-devel] [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
@ 2020-01-23 11:51 ` Jan Beulich
2020-02-21 14:03 ` Andrew Cooper
2020-01-23 11:51 ` [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation() Jan Beulich
2020-02-04 11:36 ` [Xen-devel] Ping: [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-01-23 11:51 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation()
2020-01-23 11:49 [Xen-devel] [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
2020-01-23 11:51 ` [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry() Jan Beulich
@ 2020-01-23 11:51 ` Jan Beulich
2020-02-21 14:04 ` Andrew Cooper
2020-02-04 11:36 ` [Xen-devel] Ping: [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-01-23 11:51 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] Ping: [PATCH 0/2] x86/p2m: PoD accounting adjustments
2020-01-23 11:49 [Xen-devel] [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
2020-01-23 11:51 ` [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry() Jan Beulich
2020-01-23 11:51 ` [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation() Jan Beulich
@ 2020-02-04 11:36 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2020-02-04 11:36 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry()
2020-01-23 11:51 ` [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry() Jan Beulich
@ 2020-02-21 14:03 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2020-02-21 14:03 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation()
2020-01-23 11:51 ` [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation() Jan Beulich
@ 2020-02-21 14:04 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2020-02-21 14:04 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-21 14:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 11:49 [Xen-devel] [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
2020-01-23 11:51 ` [Xen-devel] [PATCH 1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry() Jan Beulich
2020-02-21 14:03 ` Andrew Cooper
2020-01-23 11:51 ` [Xen-devel] [PATCH 2/2] x86/p2m: adjust non-PoD accounting in p2m_pod_decrease_reservation() Jan Beulich
2020-02-21 14:04 ` Andrew Cooper
2020-02-04 11:36 ` [Xen-devel] Ping: [PATCH 0/2] x86/p2m: PoD accounting adjustments Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).