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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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