* [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
@ 2016-12-14 11:35 Andrew Cooper
2016-12-14 12:52 ` Jan Beulich
2016-12-14 14:11 ` [PATCH v2] " Andrew Cooper
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2016-12-14 11:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
The assertion about guest paging mode must only apply to guest pagefaults.
This ASSERT() accidentally also trips if Xen takes a pagefault when in HVM
context, such as a copy_to_user() failure in the shadow pagetable code.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2d79ee0..fb41a2a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1798,7 +1798,7 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
return 0;
/* Logdirty mode is the only expected paging mode for PV guests. */
- if ( paging_mode_enabled(d) )
+ if ( guest_mode(regs) && paging_mode_enabled(d) )
ASSERT(paging_mode_only_log_dirty(d));
if ( !(regs->error_code & PFEC_page_present) &&
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
2016-12-14 11:35 [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c Andrew Cooper
@ 2016-12-14 12:52 ` Jan Beulich
2016-12-14 13:56 ` Andrew Cooper
2016-12-14 14:11 ` [PATCH v2] " Andrew Cooper
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-12-14 12:52 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 14.12.16 at 12:35, <andrew.cooper3@citrix.com> wrote:
> The assertion about guest paging mode must only apply to guest pagefaults.
>
> This ASSERT() accidentally also trips if Xen takes a pagefault when in HVM
> context, such as a copy_to_user() failure in the shadow pagetable code.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But isn't the other piece the original patch touched in the same
function suffering a similar problem? I.e. can't this mis-trigger
when dealing with a page fault in Xen while in the context of a
HVM guest in log-dirty mode, and hence needs re-adding the
!paging_mode_external() check (or alternatively using
paging_mode_only_log_dirty() instead of paging_mode_log_dirty())?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
2016-12-14 12:52 ` Jan Beulich
@ 2016-12-14 13:56 ` Andrew Cooper
2016-12-14 14:46 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2016-12-14 13:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 14/12/16 12:52, Jan Beulich wrote:
>>>> On 14.12.16 at 12:35, <andrew.cooper3@citrix.com> wrote:
>> The assertion about guest paging mode must only apply to guest pagefaults.
>>
>> This ASSERT() accidentally also trips if Xen takes a pagefault when in HVM
>> context, such as a copy_to_user() failure in the shadow pagetable code.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> But isn't the other piece the original patch touched in the same
> function suffering a similar problem? I.e. can't this mis-trigger
> when dealing with a page fault in Xen while in the context of a
> HVM guest in log-dirty mode, and hence needs re-adding the
> !paging_mode_external() check (or alternatively using
> paging_mode_only_log_dirty() instead of paging_mode_log_dirty())?
Good point, although it should still be guest_mode() to be more clearly
avoiding the hypervisor case.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
2016-12-14 11:35 [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c Andrew Cooper
2016-12-14 12:52 ` Jan Beulich
@ 2016-12-14 14:11 ` Andrew Cooper
2016-12-14 14:53 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2016-12-14 14:11 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
The assertion about guest paging mode must only apply to guest pagefaults, as
should the later call to paging_fault().
This ASSERT() accidentally also trips if Xen takes a pagefault when in HVM
context, such as a copy_to_user() failure in the shadow pagetable code.
Merge the ASSERT() into the later paging_fault() callsite, prediating
everything on guest_mode() to start with.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2:
* Avoid calling paging_fault() for hypervisor pagefaults.
---
xen/arch/x86/traps.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2d79ee0..7f28d0a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1797,10 +1797,6 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
if ( in_irq() || !(regs->eflags & X86_EFLAGS_IF) )
return 0;
- /* Logdirty mode is the only expected paging mode for PV guests. */
- if ( paging_mode_enabled(d) )
- ASSERT(paging_mode_only_log_dirty(d));
-
if ( !(regs->error_code & PFEC_page_present) &&
(pagefault_by_memadd(addr, regs)) )
return handle_memadd_fault(addr, regs);
@@ -1831,10 +1827,17 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
return EXCRET_fault_fixed;
}
- /* Logdirty guests call back into the paging code to update shadows. */
- if ( paging_mode_log_dirty(d) )
+ /*
+ * Logdirty guests call back into the paging code to update shadows. We
+ * don't expect any other paging modes with PV guests.
+ */
+ if ( guest_mode(regs) && paging_mode_enabled(d) )
{
- int ret = paging_fault(addr, regs);
+ int ret;
+
+ 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->eip, addr);
return ret;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
2016-12-14 13:56 ` Andrew Cooper
@ 2016-12-14 14:46 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-12-14 14:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 14.12.16 at 14:56, <andrew.cooper3@citrix.com> wrote:
> On 14/12/16 12:52, Jan Beulich wrote:
>>>>> On 14.12.16 at 12:35, <andrew.cooper3@citrix.com> wrote:
>>> The assertion about guest paging mode must only apply to guest pagefaults.
>>>
>>> This ASSERT() accidentally also trips if Xen takes a pagefault when in HVM
>>> context, such as a copy_to_user() failure in the shadow pagetable code.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> But isn't the other piece the original patch touched in the same
>> function suffering a similar problem? I.e. can't this mis-trigger
>> when dealing with a page fault in Xen while in the context of a
>> HVM guest in log-dirty mode, and hence needs re-adding the
>> !paging_mode_external() check (or alternatively using
>> paging_mode_only_log_dirty() instead of paging_mode_log_dirty())?
>
> Good point, although it should still be guest_mode() to be more clearly
> avoiding the hypervisor case.
Remember the old comment you've replaced: This code appears to
be meant to also take care of certain hypervisor faults (I guess
when accessing guest memory while the guest is in log-dirty mode).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/traps: Correct overly-general assertion introduced in c/s d5c251c
2016-12-14 14:11 ` [PATCH v2] " Andrew Cooper
@ 2016-12-14 14:53 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-12-14 14:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 14.12.16 at 15:11, <andrew.cooper3@citrix.com> wrote:
> @@ -1831,10 +1827,17 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
> return EXCRET_fault_fixed;
> }
>
> - /* Logdirty guests call back into the paging code to update shadows. */
> - if ( paging_mode_log_dirty(d) )
> + /*
> + * Logdirty guests call back into the paging code to update shadows. We
> + * don't expect any other paging modes with PV guests.
> + */
> + if ( guest_mode(regs) && paging_mode_enabled(d) )
As said in the (apparently to late) reply to v1 - I think guest_mode()
is wrong to use here, as hypervisor accesses to guest buffers also
need to be handled by paging_fault() when the guest is in log-dirty
mode.
Jan
> {
> - int ret = paging_fault(addr, regs);
> + int ret;
> +
> + 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->eip, addr);
> return ret;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-14 14:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14 11:35 [PATCH] x86/traps: Correct overly-general assertion introduced in c/s d5c251c Andrew Cooper
2016-12-14 12:52 ` Jan Beulich
2016-12-14 13:56 ` Andrew Cooper
2016-12-14 14:46 ` Jan Beulich
2016-12-14 14:11 ` [PATCH v2] " Andrew Cooper
2016-12-14 14:53 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.