| Message ID | 59B11BF302000078001781EC@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 07 September 2017 09:14 > To: xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com> > Subject: [PATCH] x86/HVM: drop bogus #PF raising from linear->phys > translation > > Translations spanning a page boundary not resulting in physically > contiguous addresses is not a reason to raise #PF. In fact by not doing > so accesses of this kind are being emulated correctly thanks to the > fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling > (non-string accesses to such locations are being split elsewhere and > hence have been working fine already). Yes, it does seem like this is way too low a layer to be raising this exception. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( > if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) > return X86EMUL_RETRY; > done /= bytes_per_rep; > - *reps = done; > if ( done == 0 ) > { > ASSERT(!reverse); > - if ( npfn != gfn_x(INVALID_GFN) ) > - return X86EMUL_UNHANDLEABLE; > - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt- > >ctxt); > - return X86EMUL_EXCEPTION; > + return X86EMUL_UNHANDLEABLE; > } > + *reps = done; > break; > } > > >
On 07/09/17 09:14, Jan Beulich wrote: > Translations spanning a page boundary not resulting in physically > contiguous addresses is not a reason to raise #PF. In fact by not doing > so accesses of this kind are being emulated correctly thanks to the > fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling > (non-string accesses to such locations are being split elsewhere and > hence have been working fine already). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( > if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) > return X86EMUL_RETRY; > done /= bytes_per_rep; > - *reps = done; > if ( done == 0 ) > { > ASSERT(!reverse); > - if ( npfn != gfn_x(INVALID_GFN) ) > - return X86EMUL_UNHANDLEABLE; > - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt); > - return X86EMUL_EXCEPTION; > + return X86EMUL_UNHANDLEABLE; I dont follow your reasoning. The pagefault path is only reachable when npfn is INVALID_GFN, which means a pagewalk did fail. It might be cleaner to split out the pagefault path out into a block at the bottom, but I think the logic is correct as-is. ~Andrew > } > + *reps = done; > break; > } > > > >
> -----Original Message----- > From: Andrew Cooper > Sent: 07 September 2017 11:18 > To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen- > devel@lists.xenproject.org> > Cc: Paul Durrant <Paul.Durrant@citrix.com> > Subject: Re: [PATCH] x86/HVM: drop bogus #PF raising from linear->phys > translation > > On 07/09/17 09:14, Jan Beulich wrote: > > Translations spanning a page boundary not resulting in physically > > contiguous addresses is not a reason to raise #PF. In fact by not doing > > so accesses of this kind are being emulated correctly thanks to the > > fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling > > (non-string accesses to such locations are being split elsewhere and > > hence have been working fine already). > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > --- a/xen/arch/x86/hvm/emulate.c > > +++ b/xen/arch/x86/hvm/emulate.c > > @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( > > if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) > > return X86EMUL_RETRY; > > done /= bytes_per_rep; > > - *reps = done; > > if ( done == 0 ) > > { > > ASSERT(!reverse); > > - if ( npfn != gfn_x(INVALID_GFN) ) > > - return X86EMUL_UNHANDLEABLE; > > - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt- > >ctxt); > > - return X86EMUL_EXCEPTION; > > + return X86EMUL_UNHANDLEABLE; > > I dont follow your reasoning. The pagefault path is only reachable when > npfn is INVALID_GFN, which means a pagewalk did fail. But even so, does it not make more sense that a call into hvmemul_linear_to_phys() simply fails and the caller decides whether an exception is warranted? It seems wrong for what is essentially a utility function to be deciding what to do. Paul > > It might be cleaner to split out the pagefault path out into a block at > the bottom, but I think the logic is correct as-is. > > ~Andrew > > > } > > + *reps = done; > > break; > > } > > > > > > > >
>>> On 07.09.17 at 12:17, <andrew.cooper3@citrix.com> wrote: > On 07/09/17 09:14, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( >> if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) >> return X86EMUL_RETRY; >> done /= bytes_per_rep; >> - *reps = done; >> if ( done == 0 ) >> { >> ASSERT(!reverse); >> - if ( npfn != gfn_x(INVALID_GFN) ) >> - return X86EMUL_UNHANDLEABLE; >> - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt); >> - return X86EMUL_EXCEPTION; >> + return X86EMUL_UNHANDLEABLE; > > I dont follow your reasoning. The pagefault path is only reachable when > npfn is INVALID_GFN, which means a pagewalk did fail. Hmm, good point. Yet the code as is definitely is not correct, as it causes the guest to triple-fault with the hvmloader test changes just sent. But since what you say regarding the #PF is true, it looks like it's really just the "*reps = done" placement which is causing the bad behavior; let me try with just that one moved to the proper place(s). Jan
On 07/09/17 11:21, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper >> Sent: 07 September 2017 11:18 >> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen- >> devel@lists.xenproject.org> >> Cc: Paul Durrant <Paul.Durrant@citrix.com> >> Subject: Re: [PATCH] x86/HVM: drop bogus #PF raising from linear->phys >> translation >> >> On 07/09/17 09:14, Jan Beulich wrote: >>> Translations spanning a page boundary not resulting in physically >>> contiguous addresses is not a reason to raise #PF. In fact by not doing >>> so accesses of this kind are being emulated correctly thanks to the >>> fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling >>> (non-string accesses to such locations are being split elsewhere and >>> hence have been working fine already). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( >>> if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) >>> return X86EMUL_RETRY; >>> done /= bytes_per_rep; >>> - *reps = done; >>> if ( done == 0 ) >>> { >>> ASSERT(!reverse); >>> - if ( npfn != gfn_x(INVALID_GFN) ) >>> - return X86EMUL_UNHANDLEABLE; >>> - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt- >>> ctxt); >>> - return X86EMUL_EXCEPTION; >>> + return X86EMUL_UNHANDLEABLE; >> I dont follow your reasoning. The pagefault path is only reachable when >> npfn is INVALID_GFN, which means a pagewalk did fail. > But even so, does it not make more sense that a call into hvmemul_linear_to_phys() simply fails and the caller decides whether an exception is warranted? It seems wrong for what is essentially a utility function to be deciding what to do. The function is unfortunately complicated by its handling of reps, but it is the architecturally appropriate place to raise pagefaults. linear_to_phys is "do a pagewalk and find where it goes", and callers don't have sufficient context to recreate a correct #PF. ~Andrew
>>> On 07.09.17 at 12:21, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Andrew Cooper >> Sent: 07 September 2017 11:18 >> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen- >> devel@lists.xenproject.org> >> Cc: Paul Durrant <Paul.Durrant@citrix.com> >> Subject: Re: [PATCH] x86/HVM: drop bogus #PF raising from linear->phys >> translation >> >> On 07/09/17 09:14, Jan Beulich wrote: >> > Translations spanning a page boundary not resulting in physically >> > contiguous addresses is not a reason to raise #PF. In fact by not doing >> > so accesses of this kind are being emulated correctly thanks to the >> > fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling >> > (non-string accesses to such locations are being split elsewhere and >> > hence have been working fine already). >> > >> > Signed-off-by: Jan Beulich <jbeulich@suse.com> >> > >> > --- a/xen/arch/x86/hvm/emulate.c >> > +++ b/xen/arch/x86/hvm/emulate.c >> > @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( >> > if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) >> > return X86EMUL_RETRY; >> > done /= bytes_per_rep; >> > - *reps = done; >> > if ( done == 0 ) >> > { >> > ASSERT(!reverse); >> > - if ( npfn != gfn_x(INVALID_GFN) ) >> > - return X86EMUL_UNHANDLEABLE; >> > - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt- >> >ctxt); >> > - return X86EMUL_EXCEPTION; >> > + return X86EMUL_UNHANDLEABLE; >> >> I dont follow your reasoning. The pagefault path is only reachable when >> npfn is INVALID_GFN, which means a pagewalk did fail. > > But even so, does it not make more sense that a call into > hvmemul_linear_to_phys() simply fails and the caller decides whether an > exception is warranted? It seems wrong for what is essentially a utility > function to be deciding what to do. No, the layer is the right one (there's another #PF being raised a few lines up). Already the direct caller of the function wouldn't know what CR2 value to specify for the #PF. Jan
--- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -566,15 +566,12 @@ static int hvmemul_linear_to_phys( if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) return X86EMUL_RETRY; done /= bytes_per_rep; - *reps = done; if ( done == 0 ) { ASSERT(!reverse); - if ( npfn != gfn_x(INVALID_GFN) ) - return X86EMUL_UNHANDLEABLE; - x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt); - return X86EMUL_EXCEPTION; + return X86EMUL_UNHANDLEABLE; } + *reps = done; break; }
Translations spanning a page boundary not resulting in physically contiguous addresses is not a reason to raise #PF. In fact by not doing so accesses of this kind are being emulated correctly thanks to the fallback logic in the insn emulator's REP MOVS/STOS/INS/OUTS handling (non-string accesses to such locations are being split elsewhere and hence have been working fine already). Signed-off-by: Jan Beulich <jbeulich@suse.com>