Re: [Xen-devel] [PATCH] x86/HVM: drop bogus #PF raising from linear->phys translation

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 07 September 2017 11:18
> To: Jan Beulich <JBeulich@xxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> 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@xxxxxxxx>
> >
> > --- 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;
> >          }
> >
> >
> >
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel