On 11/17/18 6:58 PM, Razvan Cojocaru wrote:
> On 11/16/18 9:50 PM, Razvan Cojocaru wrote:
>> On 11/16/18 7:59 PM, George Dunlap wrote:
>>> 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.
>>
>> Thanks! I haven't yet been able to think in depth about the logic, but I
>> did manage to apply them. Just applying the first one allows me to set
>> p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
>> and everything appears to work well.
>>
>> With both patches applies, the display remains frozen (things appear to
>> behave - externally - in the same way as before the series).
>
> Right, I know why the second patch keeps the display frozen. It's
> because for altp2ms (where it matters most), the patch basically does
> invalidate_start = max(invalidate_start, p2m->min_remapped_gfn) and
> invalidate_end = min(invalidate_end, p2m->max_remapped_gfn).
>
> However, as previously requested, I've now made p2m->max_remapped_gfn
> begin life as 0 for altp2ms, and p2m->min_remapped_gfn is initialized to
> INVALID_GFN, which is decimal 18446744073709551615. So we get
> invalidate_end: 0, invalidate_start: 18446744073709551615,
> invalidate_end < invalidate_start, resulting in nothing being done for
> altp2ms, which is functionally back to square one.
But this doesn't explain why my reasoning was wrong; and it's always
dangerous to use a system whose behavior you don't really understand,
even if it seems to work.
It turns out I'd made a mistake in saying that altp2m->max_mapped_pfn ==
alt2m->max_remapped_gfn. The first is the highest gfn present in the
altp2m, either copied from the hostp2m or changed; the second is the
highest value changed (via p2m_altp2m_change_gfn()).
What about using the attached patch instead?
-George
From f4c72ecb95cfa5d597b8e28e99e681cfeaa32199 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 mapped 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 the
current p2m's max_mapped_pfn to further clip the invalidation range
for alternate p2ms.
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/arch/x86/mm/p2m.c | 61 +++++++++++++++++++++++++++++--------------
1 file changed, 42 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6d764d1e22..b97e138452 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 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) )
+ invalidate_end = min(invalidate_end, max_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 )
- {
- 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 ( invalidate_start < invalidate_end )
+ {
+ /*
+ * If all valid gfns are in the invalidation range, just do a
+ * global type change. Otherwise, invalidate only the range
+ * we need.
+ *
+ * NB that invalidate_end can't logically be >max_pfn at this
+ * point. If this changes, the == will need to be changed to
+ * >=.
+ */
+ ASSERT(invalidate_end <= max_pfn);
+ if ( !invalidate_start && 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
