On 11/16/18 2:10 PM, Razvan Cojocaru wrote:
> On 11/16/18 2:03 PM, George Dunlap wrote:
>> The code is definitely complicated enough, though, that I may have
>> missed something, which is why I asked Razvan if there was a reason he
>> changed it.
>>
>> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
>> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
>> max_remapped_pfn" for a future clean-up series.
>
> I've retraced my previous analysis and re-ran some tests, and I now
> remember (sorry it took a while) why the p2m->max_mapped_pfn =
> hostp2m->max_mapped_pfn was both necessary and not accidental.
>
> Let's say we set it to 0 in p2m_altp2m_init_ept(). Then,
> hap_track_dirty_vram() calls p2m_change_type_range(), which calls the
> newly added change_type_range().
>
> Change_type_range() looks like this:
>
> static void change_type_range(struct p2m_domain *p2m,
> unsigned long start, unsigned long end,
> p2m_type_t ot, p2m_type_t nt)
> {
> unsigned long gfn = start;
> struct domain *d = p2m->domain;
> int rc = 0;
>
> p2m->defer_nested_flush = 1;
>
> if ( unlikely(end > p2m->max_mapped_pfn) )
> {
> if ( !gfn )
> {
> p2m->change_entry_type_global(p2m, ot, nt);
> gfn = end;
> }
> end = p2m->max_mapped_pfn + 1;
> }
> if ( gfn < end )
> rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
> if ( rc )
> {
> printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from
> %d to %d\n",
> rc, d->domain_id, start, end - 1, ot, nt);
> domain_crash(d);
> }
>
> switch ( nt )
> {
> case p2m_ram_rw:
> if ( ot == p2m_ram_logdirty )
> rc = rangeset_remove_range(p2m->logdirty_ranges, start, end
> - 1);
> break;
> case p2m_ram_logdirty:
> if ( ot == p2m_ram_rw )
> rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
> break;
> default:
> break;
> }
> if ( rc )
> {
> printk(XENLOG_G_ERR "Error %d manipulating Dom%d's log-dirty
> ranges\n",
> rc, d->domain_id);
> domain_crash(d);
> }
>
> p2m->defer_nested_flush = 0;
> if ( nestedhvm_enabled(d) )
> p2m_flush_nestedp2m(d);
> }
>
> If we set p2m->max_mapped_pfn to 0, we're guaranteed to run into the if
> ( unlikely(end > p2m->max_mapped_pfn) ) body, where end =
> p2m->max_mapped_pfn + 1; will make end 1.
>
> Then, we will crash the hypervisor in rangeset_add_range(), where
> there's an ASSERT() stating that start <= end.
Ah, right, this was the original crash that you ran into several months
ago, which flagged up the whole logdirty range synchronization issue.
But that's partly a logic hole in change_entry_type_range(), which
assumes that start < p2m->max_mapped_pfn. It would be better to fix
that than to work around it by changing the meaning of max_mapped_pfn.
On the other hand, we want the logdirty rangesets to actually match the
host's rangesets; using altp2m->max_mapped_pfn for this is clearly
wrong. The easiest fix would be just to explicitly use the host's
max_mapped_pfn when calculating the clipping. A more complete fix would
involve calculating two different ranges -- a "rangeset" range and a
"invalidate" range, the second of which would be clipped on altp2ms by
{min,max}_remapped_gfn.
Something like the attached (compile-tested only). I'm partial to
having both patches applied, but I'd be open to arguments that we should
only use the first.
-George
From d92bd123f92d66aef394735a6d836fd104f01867 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@citrix.com>
Date: Fri, 16 Nov 2018 17:17:48 +0000
Subject: [PATCH 1/2] p2m: Always use hostp2m when clipping rangesets
The logdirty rangesets of the altp2ms need to be kept in sync with the
hostp2m. This means when iterating through the altp2ms, we need to
use the host p2m to clip the rangeset, not the indiviual altp2m's
value.
This change also:
- Documents that the end is non-inclusive
- Calculates an "inclusive" value for the end once, rather than
open-coding the modification, and (worse) back-modifying updates so
that the calculation ends up correct
- Clarifies the logic deciding whether to call
change_entry_type_global() or change_entry_type_range()
- Handles the case where start >= hostp2m->max_mapped_pfn
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
RFC: Wasn't sure what the best thing was to do if start >=
host_max_pfn. We silently clip the logdirty rangeset to
max_mapped_pfn, and the chosen behavior seems consistent with that.
But it seems like such a request would almost certainly be a bug
somewhere that people might like to find out about.
---
xen/arch/x86/mm/p2m.c | 46 +++++++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c401806562..6d764d1e22 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1037,32 +1037,44 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
return rc;
}
-/* Modify the p2m type of a range of gfns from ot to nt. */
+/* Modify the p2m type of [start, end) from ot to nt. */
static void change_type_range(struct p2m_domain *p2m,
unsigned long start, unsigned long end,
p2m_type_t ot, p2m_type_t nt)
{
- unsigned long gfn = start;
+ unsigned long rangeset_start, rangeset_end;
struct domain *d = p2m->domain;
+ unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
int rc = 0;
-
+
+ rangeset_start = start;
+ rangeset_end = end - 1;
+
+ /* Always clip the rangeset down to the host p2m */
+ if ( unlikely(rangeset_end > host_max_pfn) )
+ rangeset_end = host_max_pfn;
+
+ /* If the requested range is out of scope, return doing nothing */
+ if ( rangeset_start > rangeset_end )
+ return;
+
p2m->defer_nested_flush = 1;
- if ( unlikely(end > p2m->max_mapped_pfn) )
- {
- if ( !gfn )
- {
- p2m->change_entry_type_global(p2m, ot, nt);
- gfn = end;
- }
- end = p2m->max_mapped_pfn + 1;
- }
- if ( gfn < end )
- rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
+ /*
+ * If all valid gfns are in the invalidation range, just do a
+ * global type change. Otherwise, invalidate only the range we
+ * need.
+ */
+ if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
+ p2m->change_entry_type_global(p2m, ot, nt);
+ else
+ rc = p2m->change_entry_type_range(p2m, ot, nt,
+ rangeset_start, rangeset_end);
+
if ( rc )
{
printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
- rc, d->domain_id, start, end - 1, ot, nt);
+ rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
domain_crash(d);
}
@@ -1070,11 +1082,11 @@ static void change_type_range(struct p2m_domain *p2m,
{
case p2m_ram_rw:
if ( ot == p2m_ram_logdirty )
- rc = rangeset_remove_range(p2m->logdirty_ranges, start, end - 1);
+ rc = rangeset_remove_range(p2m->logdirty_ranges, rangeset_start, rangeset_end);
break;
case p2m_ram_logdirty:
if ( ot == p2m_ram_rw )
- rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
+ rc = rangeset_add_range(p2m->logdirty_ranges, rangeset_start, rangeset_end);
break;
default:
break;
--
2.19.1
From c2c6e0b9c27650607a5d15aca0d598ae7251678e Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@citrix.com>
Date: Fri, 16 Nov 2018 16:28:25 +0000
Subject: [PATCH 2/2] p2m: change_range_type: Only invalidate remapped gfns
change_range_type() invalidates gfn ranges to lazily change the type
of a range of gfns, and also modifies the logdirty rangesets of that
p2m. At the moment, it clips both down by the hostp2m.
While this will result in correct behavior, it's not entirely efficient,
since invalidated entries outside that range will, on fault, simply be
modified back to "empty" before faulting normally again.
Separate out the calculation of the two ranges. Keep using the
hostp2m's max_mapped_pfn to clip the logdirty ranges, but use
{max,min}_remapped_gfn to further clip the invalidation range for
alternate p2ms.
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/arch/x86/mm/p2m.c | 59 ++++++++++++++++++++++++++++++-------------
1 file changed, 41 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6d764d1e22..5a68e7fd78 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1043,41 +1043,64 @@ static void change_type_range(struct p2m_domain *p2m,
p2m_type_t ot, p2m_type_t nt)
{
unsigned long rangeset_start, rangeset_end;
+ unsigned long invalidate_start, invalidate_end;
struct domain *d = p2m->domain;
unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
+ unsigned long min_pfn = 0, max_pfn = p2m->max_mapped_pfn;
int rc = 0;
- rangeset_start = start;
- rangeset_end = end - 1;
+ /*
+ * If we have an altp2m, the logdirty rangeset range needs to
+ * match that of the hostp2m, but for efficiency, we want to clip
+ * down the the invalidation range according to the mapped values
+ * in the altp2m. Keep track of and clip the ranges separately.
+ */
+ rangeset_start = invalidate_start = start;
+ rangeset_end = invalidate_end = end - 1;
- /* Always clip the rangeset down to the host p2m */
+ /* Clip down to the host p2m */
if ( unlikely(rangeset_end > host_max_pfn) )
- rangeset_end = host_max_pfn;
+ rangeset_end = invalidate_end = host_max_pfn;
/* If the requested range is out of scope, return doing nothing */
if ( rangeset_start > rangeset_end )
return;
+ if ( p2m_is_altp2m(p2m) )
+ {
+ max_pfn = p2m->max_remapped_gfn;
+ min_pfn = p2m->min_remapped_gfn;
+ invalidate_end = min(invalidate_end, max_pfn);
+ invalidate_start = max(invalidate_start, min_pfn);
+ }
+
p2m->defer_nested_flush = 1;
/*
- * If all valid gfns are in the invalidation range, just do a
- * global type change. Otherwise, invalidate only the range we
- * need.
+ * If the p2m is empty, or the range is outside the currently
+ * mapped range, no need to do the invalidation; just update the
+ * rangeset.
*/
- if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
- p2m->change_entry_type_global(p2m, ot, nt);
- else
- rc = p2m->change_entry_type_range(p2m, ot, nt,
- rangeset_start, rangeset_end);
-
- if ( rc )
+ if ( invalidate_start < invalidate_end )
{
- printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
- rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
- domain_crash(d);
+ /*
+ * If all valid gfns are in the invalidation range, just do a
+ * global type change. Otherwise, invalidate only the range
+ * we need.
+ */
+ if ( invalidate_start <= min_pfn && invalidate_end >= max_pfn)
+ p2m->change_entry_type_global(p2m, ot, nt);
+ else
+ rc = p2m->change_entry_type_range(p2m, ot, nt,
+ invalidate_start, invalidate_end);
+ if ( rc )
+ {
+ printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
+ rc, d->domain_id, invalidate_start, invalidate_end, ot, nt);
+ domain_crash(d);
+ }
}
-
+
switch ( nt )
{
case p2m_ram_rw:
--
2.19.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
