Patchwork x86/HVM: drop bogus #PF raising from linear->phys translation

login
register
mail settings
Submitter Jan Beulich
Date Sept. 7, 2017, 8:14 a.m.
Message ID <59B11BF302000078001781EC@prv-mh.provo.novell.com>
Download mbox | patch
Permalink /patch/333451/
State New
Headers show

Comments

Jan Beulich - Sept. 7, 2017, 8:14 a.m.
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>
Paul Durrant - Sept. 7, 2017, 8:24 a.m.
> -----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;
>          }
> 
> 
>
Andrew Cooper - Sept. 7, 2017, 10:17 a.m.
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;
>          }
>  
>
>
>
Paul Durrant - Sept. 7, 2017, 10:21 a.m.
> -----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;

> >          }

> >

> >

> >

> >
Jan Beulich - Sept. 7, 2017, 10:25 a.m.
>>> 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
Andrew Cooper - Sept. 7, 2017, 10:34 a.m.
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
Jan Beulich - Sept. 7, 2017, 10:35 a.m.
>>> 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

Patch

--- 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;
         }