| Message ID | 59B13E9102000078001783E2@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 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; > } > > >
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; > } > > > >
>>> 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
>>> 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
>>> 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
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
>>> 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
> -----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; > } > > >
--- 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; }
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>