[PATCH] x86/HVM: correct repeat count update in linear->phys translation

[PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Jan Beulich]

For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
handling to work correctly, *reps must not be set to zero when
returning X86EMUL_UNHANDLEABLE.

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,16 @@ 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;
+                *reps = 0;
                 x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
                 return X86EMUL_EXCEPTION;
             }
+            *reps = done;
             break;
         }
 




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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 07 September 2017 11:42
> 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: correct repeat count update in linear->phys
> translation
> 
> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
> handling to work correctly, *reps must not be set to zero when
> returning X86EMUL_UNHANDLEABLE.

Does it really need to be zero when returning X86EMUL_EXCEPTION?

  Paul

> 
> 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,16 @@ 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;
> +                *reps = 0;
>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt-
> >ctxt);
>                  return X86EMUL_EXCEPTION;
>              }
> +            *reps = done;
>              break;
>          }
> 
> 
> 


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Andrew Cooper]

On 07/09/17 11:41, Jan Beulich wrote:
> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
> handling to work correctly, *reps must not be set to zero when
> returning X86EMUL_UNHANDLEABLE.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Why is this?  In the case that X86EMUL_UNHANDLEABLE is returned, the
emulator appears to override nr_reps to 1.

I'm clearly missing something.

>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -566,15 +566,16 @@ 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;
> +                *reps = 0;
>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);

Independently to the issue at hand, this looks suspicious for the
reverse direction.

Hardware will issue a walk for the first byte of access, and optionally
a second at the start of the subsequent page for a straddled access. 
For the reverse case, this looks like it will truncate down to the start
of the lower linear address, which I bet isn't how hardware actually
behaves.

(In some copious free time, I should really put together a discontinuous
rep insn XTF test).

~Andrew

>                  return X86EMUL_EXCEPTION;
>              }
> +            *reps = done;
>              break;
>          }
>  
>
>
>


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Jan Beulich]

>>> On 07.09.17 at 12:50, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 07 September 2017 11:42
>> 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: correct repeat count update in linear->phys
>> translation
>> 
>> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
>> handling to work correctly, *reps must not be set to zero when
>> returning X86EMUL_UNHANDLEABLE.
> 
> Does it really need to be zero when returning X86EMUL_EXCEPTION?

Yes. Since an exception may occur on other than the first
iteration, in that case the number of successfully done one
needs to be handed back. See also the other #PF invocation
a few lines up.

Jan


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Jan Beulich]

>>> On 07.09.17 at 13:15, <andrew.cooper3@citrix.com> wrote:
> On 07/09/17 11:41, Jan Beulich wrote:
>> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
>> handling to work correctly, *reps must not be set to zero when
>> returning X86EMUL_UNHANDLEABLE.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Why is this?  In the case that X86EMUL_UNHANDLEABLE is returned, the
> emulator appears to override nr_reps to 1.

Where did you see that? What we have is

        if ( (nr_reps > 1 || rc == X86EMUL_UNHANDLEABLE) && ops->rep_ins )
            rc = ops->rep_ins(port, dst.mem.seg, dst.mem.off, dst.bytes,
                              &nr_reps, ctxt);
        if ( nr_reps >= 1 && rc == X86EMUL_UNHANDLEABLE )
        {
            fail_if(!ops->read_io || !ops->write);
            if ( (rc = ops->read_io(port, dst.bytes, &dst.val, ctxt)) != 0 )
                goto done;
            nr_reps = 0;
        }

>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -566,15 +566,16 @@ 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;
>> +                *reps = 0;
>>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, 
> &hvmemul_ctxt->ctxt);
> 
> Independently to the issue at hand, this looks suspicious for the
> reverse direction.
> 
> Hardware will issue a walk for the first byte of access, and optionally
> a second at the start of the subsequent page for a straddled access. 
> For the reverse case, this looks like it will truncate down to the start
> of the lower linear address, which I bet isn't how hardware actually
> behaves.

Good point. Since I've played with this just now anyway, let me
see if I can get this corrected in another patch.

Jan


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Jan Beulich]

>>> On 07.09.17 at 13:15, <andrew.cooper3@citrix.com> wrote:
> On 07/09/17 11:41, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -566,15 +566,16 @@ 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;
>> +                *reps = 0;
>>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, 
> &hvmemul_ctxt->ctxt);
> 
> Independently to the issue at hand, this looks suspicious for the
> reverse direction.
> 
> Hardware will issue a walk for the first byte of access, and optionally
> a second at the start of the subsequent page for a straddled access. 
> For the reverse case, this looks like it will truncate down to the start
> of the lower linear address, which I bet isn't how hardware actually
> behaves.

Actually - no, this looks all fine. Note the "ASSERT(!reverse)" in
context above. Reverse page straddling accesses are being
handled by a single forward iteration several lines up from here.

Jan


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Andrew Cooper]

On 07/09/17 12:24, Jan Beulich wrote:
>>>> On 07.09.17 at 13:15, <andrew.cooper3@citrix.com> wrote:
>> On 07/09/17 11:41, Jan Beulich wrote:
>>> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
>>> handling to work correctly, *reps must not be set to zero when
>>> returning X86EMUL_UNHANDLEABLE.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Why is this?  In the case that X86EMUL_UNHANDLEABLE is returned, the
>> emulator appears to override nr_reps to 1.
> Where did you see that? What we have is
>
>         if ( (nr_reps > 1 || rc == X86EMUL_UNHANDLEABLE) && ops->rep_ins )
>             rc = ops->rep_ins(port, dst.mem.seg, dst.mem.off, dst.bytes,
>                               &nr_reps, ctxt);
>         if ( nr_reps >= 1 && rc == X86EMUL_UNHANDLEABLE )
>         {
>             fail_if(!ops->read_io || !ops->write);
>             if ( (rc = ops->read_io(port, dst.bytes, &dst.val, ctxt)) != 0 )
>                 goto done;
>             nr_reps = 0;
>         }

Ah - the INS/OUTS handing is different to the MOVS/STOS, where the
MOVS/STOS does cope fine with reps being zero.

With a suitable adjustment to the commit message, Reviewed-by: Andrew
Cooper <andrew.cooper3@citrix.com>

>
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -566,15 +566,16 @@ 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;
>>> +                *reps = 0;
>>>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, 
>> &hvmemul_ctxt->ctxt);
>>
>> Independently to the issue at hand, this looks suspicious for the
>> reverse direction.
>>
>> Hardware will issue a walk for the first byte of access, and optionally
>> a second at the start of the subsequent page for a straddled access. 
>> For the reverse case, this looks like it will truncate down to the start
>> of the lower linear address, which I bet isn't how hardware actually
>> behaves.
> Good point. Since I've played with this just now anyway, let me
> see if I can get this corrected in another patch.

(Merging the other thread to avoid unnecessary emails)

> Actually - no, this looks all fine. Note the "ASSERT(!reverse)" in
> context above. Reverse page straddling accesses are being
> handled by a single forward iteration several lines up from here.

Good point.  I've got a patch which cleans up the pagefault handling a
little, which I will rebase over this patch.

~Andrew

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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Jan Beulich]

>>> On 07.09.17 at 13:35, <andrew.cooper3@citrix.com> wrote:
> On 07/09/17 12:24, Jan Beulich wrote:
>>>>> On 07.09.17 at 13:15, <andrew.cooper3@citrix.com> wrote:
>>> On 07/09/17 11:41, Jan Beulich wrote:
>>>> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
>>>> handling to work correctly, *reps must not be set to zero when
>>>> returning X86EMUL_UNHANDLEABLE.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Why is this?  In the case that X86EMUL_UNHANDLEABLE is returned, the
>>> emulator appears to override nr_reps to 1.
>> Where did you see that? What we have is
>>
>>         if ( (nr_reps > 1 || rc == X86EMUL_UNHANDLEABLE) && ops->rep_ins )
>>             rc = ops->rep_ins(port, dst.mem.seg, dst.mem.off, dst.bytes,
>>                               &nr_reps, ctxt);
>>         if ( nr_reps >= 1 && rc == X86EMUL_UNHANDLEABLE )
>>         {
>>             fail_if(!ops->read_io || !ops->write);
>>             if ( (rc = ops->read_io(port, dst.bytes, &dst.val, ctxt)) != 0 )
>>                 goto done;
>>             nr_reps = 0;
>>         }
> 
> Ah - the INS/OUTS handing is different to the MOVS/STOS, where the
> MOVS/STOS does cope fine with reps being zero.

Oh, right.

> With a suitable adjustment to the commit message, Reviewed-by: Andrew
> Cooper <andrew.cooper3@citrix.com>

I'd just dropped the MOVS/STOS from the text, but left it unchanged
otherwise.

Jan


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

Re: [PATCH] x86/HVM: correct repeat count update in linear->phys translation

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 07 September 2017 11:42
> 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: correct repeat count update in linear->phys
> translation
> 
> For the insn emulator's fallback logic in REP MOVS/STOS/INS/OUTS
> handling to work correctly, *reps must not be set to zero when
> returning X86EMUL_UNHANDLEABLE.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>

Acked-by: Paul Durrant <paul.durrant@citrix.com>

 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -566,15 +566,16 @@ 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;
> +                *reps = 0;
>                  x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt-
> >ctxt);
>                  return X86EMUL_EXCEPTION;
>              }
> +            *reps = done;
>              break;
>          }
> 
> 
> 


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