x86/mm: drop bogus paging mode assertion

Olaf has observed this assertion to trigger after an aborted migration
of a PV guest:

(XEN) Xen call trace:
(XEN)    [<ffff82d0802a85dc>] do_page_fault+0x39f/0x55c
(XEN)    [<ffff82d08036b7d8>] x86_64/entry.S#handle_exception_saved+0x66/0xa4
(XEN)    [<ffff82d0802a9274>] __copy_to_user_ll+0x22/0x30
(XEN)    [<ffff82d0802772d4>] update_runstate_area+0x19c/0x228
(XEN)    [<ffff82d080277371>] domain.c#_update_runstate_area+0x11/0x39
(XEN)    [<ffff82d080277596>] context_switch+0x1fd/0xf25
(XEN)    [<ffff82d0802395c5>] schedule.c#schedule+0x303/0x6a8
(XEN)    [<ffff82d08023d067>] softirq.c#__do_softirq+0x6c/0x95
(XEN)    [<ffff82d08023d0da>] do_softirq+0x13/0x15
(XEN)    [<ffff82d08036b2f1>] x86_64/entry.S#process_softirqs+0x21/0x30

Release builds work fine, which is a first indication that the assertion
isn't really needed.

What's worse though - there appears to be a timing window where the
guest runs in shadow mode, but not in log-dirty mode, and that is what
triggers the assertion (the same could, afaict, be achieved by test-
enabling shadow mode on a PV guest). This is because turing off log-
dirty mode is being performed in two steps: First the log-dirty bit gets
cleared (paging_log_dirty_disable() [having paused the domain] ->
sh_disable_log_dirty() -> shadow_one_bit_disable()), followed by
unpausing the domain and only then clearing shadow mode (via
shadow_test_disable(), which pauses the domain a second time).

Hence besides removing the ASSERT() here (or optionally replacing it by
explicit translate and refcounts mode checks, but this seems rather
pointless now that the three are tied together) I wonder whether either
shadow_one_bit_disable() should turn off shadow mode if no other bit
besides PG_SH_enable remains set (just like shadow_one_bit_enable()
enables it if not already set), or the domain pausing scope should be
extended so that both steps occur without the domain getting a chance to
run in between.

Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
This commit is contained in:
Jan Beulich committed 2017-12-12 16:56:15 +01:00
1 parent 7f10619384
commit b95f7be32d
2 files changed
+1 -8

No files matched your search

+1 -5
View File
@@ -1334,12 +1334,8 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
*/
if ( paging_mode_enabled(d) && !paging_mode_external(d) )
{
int ret;
int ret = paging_fault(addr, regs);
/* Logdirty mode is the only expected paging mode for PV guests. */
ASSERT(paging_mode_only_log_dirty(d));
ret = paging_fault(addr, regs);
if ( ret == EXCRET_fault_fixed )
trace_trap_two_addr(TRC_PV_PAGING_FIXUP, regs->rip, addr);
return ret;
-3
View File
@@ -69,9 +69,6 @@
#define paging_mode_translate(_d) (!!((_d)->arch.paging.mode & PG_translate))
#define paging_mode_external(_d) (!!((_d)->arch.paging.mode & PG_external))
#define paging_mode_only_log_dirty(_d) \
(((_d)->arch.paging.mode & PG_MASK) == PG_log_dirty)
/* flags used for paging debug */
#define PAGING_DEBUG_LOGDIRTY 0
Reference in new issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.