* [PATCH] x86/HVM: mis adjustments @ 2016-06-08 13:35 Jan Beulich 2016-06-08 13:42 ` [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter Jan Beulich 2016-06-08 13:43 ` [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() Jan Beulich 0 siblings, 2 replies; 13+ messages in thread From: Jan Beulich @ 2016-06-08 13:35 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper 1: constify hvm_virtual_to_linear_addr()'s segment register parameter 2: re-order operations in hvm_ud_intercept() Signed-off-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter 2016-06-08 13:35 [PATCH] x86/HVM: mis adjustments Jan Beulich @ 2016-06-08 13:42 ` Jan Beulich 2016-06-09 11:25 ` Andrew Cooper 2016-06-08 13:43 ` [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2016-06-08 13:42 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper [-- Attachment #1: Type: text/plain, Size: 910 bytes --] ... to clarify to callers that they don't need to fear the pointed to data changing (which will be made use of subsequently). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2411,7 +2411,7 @@ int hvm_set_cr4(unsigned long value, boo bool_t hvm_virtual_to_linear_addr( enum x86_segment seg, - struct segment_register *reg, + const struct segment_register *reg, unsigned long offset, unsigned int bytes, enum hvm_access_type access_type, --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -469,7 +469,7 @@ enum hvm_access_type { }; bool_t hvm_virtual_to_linear_addr( enum x86_segment seg, - struct segment_register *reg, + const struct segment_register *reg, unsigned long offset, unsigned int bytes, enum hvm_access_type access_type, [-- Attachment #2: x86-HVM-v2l-const.patch --] [-- Type: text/plain, Size: 983 bytes --] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter ... to clarify to callers that they don't need to fear the pointed to data changing (which will be made use of subsequently). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2411,7 +2411,7 @@ int hvm_set_cr4(unsigned long value, boo bool_t hvm_virtual_to_linear_addr( enum x86_segment seg, - struct segment_register *reg, + const struct segment_register *reg, unsigned long offset, unsigned int bytes, enum hvm_access_type access_type, --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -469,7 +469,7 @@ enum hvm_access_type { }; bool_t hvm_virtual_to_linear_addr( enum x86_segment seg, - struct segment_register *reg, + const struct segment_register *reg, unsigned long offset, unsigned int bytes, enum hvm_access_type access_type, [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter 2016-06-08 13:42 ` [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter Jan Beulich @ 2016-06-09 11:25 ` Andrew Cooper 0 siblings, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2016-06-09 11:25 UTC (permalink / raw) To: Jan Beulich, xen-devel On 08/06/16 14:42, Jan Beulich wrote: > ... to clarify to callers that they don't need to fear the pointed to > data changing (which will be made use of subsequently). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-08 13:35 [PATCH] x86/HVM: mis adjustments Jan Beulich 2016-06-08 13:42 ` [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter Jan Beulich @ 2016-06-08 13:43 ` Jan Beulich 2016-06-09 11:34 ` Andrew Cooper 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2016-06-08 13:43 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper [-- Attachment #1: Type: text/plain, Size: 2020 bytes --] Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() already does (and that hvm_virtual_to_linear_addr() doesn't alter it). At once increase the length passed to hvm_virtual_to_linear_addr() by one: There definitely needs to be at least one more opcode byte, and we can avoid missing a wraparound case this way. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3834,19 +3834,20 @@ void hvm_ud_intercept(struct cpu_user_re { struct hvm_emulate_ctxt ctxt; + hvm_emulate_prepare(&ctxt, regs); + if ( opt_hvm_fep ) { struct vcpu *cur = current; - struct segment_register cs; + const struct segment_register *cs = &ctxt.seg_reg[x86_seg_cs]; unsigned long addr; char sig[5]; /* ud2; .ascii "xen" */ - hvm_get_segment_register(cur, x86_seg_cs, &cs); - if ( hvm_virtual_to_linear_addr(x86_seg_cs, &cs, regs->eip, - sizeof(sig), hvm_access_insn_fetch, + if ( hvm_virtual_to_linear_addr(x86_seg_cs, cs, regs->eip, + sizeof(sig) + 1, hvm_access_insn_fetch, (hvm_long_mode_enabled(cur) && - cs.attr.fields.l) ? 64 : - cs.attr.fields.db ? 32 : 16, &addr) && + cs->attr.fields.l) ? 64 : + cs->attr.fields.db ? 32 : 16, &addr) && (hvm_fetch_from_guest_virt_nofault(sig, addr, sizeof(sig), 0) == HVMCOPY_okay) && (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) ) @@ -3856,8 +3857,6 @@ void hvm_ud_intercept(struct cpu_user_re } } - hvm_emulate_prepare(&ctxt, regs); - switch ( hvm_emulate_one(&ctxt) ) { case X86EMUL_UNHANDLEABLE: [-- Attachment #2: x86-HVM-fep-reorder.patch --] [-- Type: text/plain, Size: 2068 bytes --] x86/HVM: re-order operations in hvm_ud_intercept() Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() already does (and that hvm_virtual_to_linear_addr() doesn't alter it). At once increase the length passed to hvm_virtual_to_linear_addr() by one: There definitely needs to be at least one more opcode byte, and we can avoid missing a wraparound case this way. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3834,19 +3834,20 @@ void hvm_ud_intercept(struct cpu_user_re { struct hvm_emulate_ctxt ctxt; + hvm_emulate_prepare(&ctxt, regs); + if ( opt_hvm_fep ) { struct vcpu *cur = current; - struct segment_register cs; + const struct segment_register *cs = &ctxt.seg_reg[x86_seg_cs]; unsigned long addr; char sig[5]; /* ud2; .ascii "xen" */ - hvm_get_segment_register(cur, x86_seg_cs, &cs); - if ( hvm_virtual_to_linear_addr(x86_seg_cs, &cs, regs->eip, - sizeof(sig), hvm_access_insn_fetch, + if ( hvm_virtual_to_linear_addr(x86_seg_cs, cs, regs->eip, + sizeof(sig) + 1, hvm_access_insn_fetch, (hvm_long_mode_enabled(cur) && - cs.attr.fields.l) ? 64 : - cs.attr.fields.db ? 32 : 16, &addr) && + cs->attr.fields.l) ? 64 : + cs->attr.fields.db ? 32 : 16, &addr) && (hvm_fetch_from_guest_virt_nofault(sig, addr, sizeof(sig), 0) == HVMCOPY_okay) && (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) ) @@ -3856,8 +3857,6 @@ void hvm_ud_intercept(struct cpu_user_re } } - hvm_emulate_prepare(&ctxt, regs); - switch ( hvm_emulate_one(&ctxt) ) { case X86EMUL_UNHANDLEABLE: [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-08 13:43 ` [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() Jan Beulich @ 2016-06-09 11:34 ` Andrew Cooper 2016-06-09 12:31 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2016-06-09 11:34 UTC (permalink / raw) To: Jan Beulich, xen-devel On 08/06/16 14:43, Jan Beulich wrote: > Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() > already does (and that hvm_virtual_to_linear_addr() doesn't alter it). > > At once increase the length passed to hvm_virtual_to_linear_addr() by > one: There definitely needs to be at least one more opcode byte, and we > can avoid missing a wraparound case this way. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I looked into this when you suggested it, but it latches the wrong eip in the emulation state, and you will end up re-emulating the ud2a instruction, rather than the following instruction. I would be tempted just to leave this code in its current condition. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 11:34 ` Andrew Cooper @ 2016-06-09 12:31 ` Jan Beulich 2016-06-09 14:06 ` Andrew Cooper 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2016-06-09 12:31 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: > On 08/06/16 14:43, Jan Beulich wrote: >> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >> >> At once increase the length passed to hvm_virtual_to_linear_addr() by >> one: There definitely needs to be at least one more opcode byte, and we >> can avoid missing a wraparound case this way. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I looked into this when you suggested it, but it latches the wrong eip > in the emulation state, and you will end up re-emulating the ud2a > instruction, rather than the following instruction. Where is there any latching of eip? All hvm_emulate_prepare() does is storing the regs pointer. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 12:31 ` Jan Beulich @ 2016-06-09 14:06 ` Andrew Cooper 2016-06-09 14:13 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2016-06-09 14:06 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel On 09/06/16 13:31, Jan Beulich wrote: >>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >> On 08/06/16 14:43, Jan Beulich wrote: >>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>> >>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>> one: There definitely needs to be at least one more opcode byte, and we >>> can avoid missing a wraparound case this way. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> I looked into this when you suggested it, but it latches the wrong eip >> in the emulation state, and you will end up re-emulating the ud2a >> instruction, rather than the following instruction. > Where is there any latching of eip? All hvm_emulate_prepare() does > is storing the regs pointer. Oh - so it does. I clearly looked over it too quickly. What wraparound issue are you referring to? Adding 1 will cause incorrect behaviour when the emulation prefix ends at the segment limit. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 14:06 ` Andrew Cooper @ 2016-06-09 14:13 ` Jan Beulich 2016-06-09 14:27 ` Andrew Cooper 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2016-06-09 14:13 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: > On 09/06/16 13:31, Jan Beulich wrote: >>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>> On 08/06/16 14:43, Jan Beulich wrote: >>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>> >>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>> one: There definitely needs to be at least one more opcode byte, and we >>>> can avoid missing a wraparound case this way. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> I looked into this when you suggested it, but it latches the wrong eip >>> in the emulation state, and you will end up re-emulating the ud2a >>> instruction, rather than the following instruction. >> Where is there any latching of eip? All hvm_emulate_prepare() does >> is storing the regs pointer. > > Oh - so it does. I clearly looked over it too quickly. > > What wraparound issue are you referring to? Adding 1 will cause > incorrect behaviour when the emulation prefix ends at the segment limit. I don't think so: The prefix together with the actual instruction encoding should be viewed as a single instruction, and it crossing the segment limit should #GP. It wrapping at the prefix/encoding boundary is the case that I'm specifically referring to (this case should also #GP, but wouldn't without this adjustment). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 14:13 ` Jan Beulich @ 2016-06-09 14:27 ` Andrew Cooper 2016-06-09 15:05 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2016-06-09 14:27 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel On 09/06/16 15:13, Jan Beulich wrote: >>>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: >> On 09/06/16 13:31, Jan Beulich wrote: >>>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>>> On 08/06/16 14:43, Jan Beulich wrote: >>>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>>> >>>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>>> one: There definitely needs to be at least one more opcode byte, and we >>>>> can avoid missing a wraparound case this way. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> I looked into this when you suggested it, but it latches the wrong eip >>>> in the emulation state, and you will end up re-emulating the ud2a >>>> instruction, rather than the following instruction. >>> Where is there any latching of eip? All hvm_emulate_prepare() does >>> is storing the regs pointer. >> Oh - so it does. I clearly looked over it too quickly. >> >> What wraparound issue are you referring to? Adding 1 will cause >> incorrect behaviour when the emulation prefix ends at the segment limit. > I don't think so: The prefix together with the actual instruction > encoding should be viewed as a single instruction, and it crossing > the segment limit should #GP. It wrapping at the prefix/encoding > boundary is the case that I'm specifically referring to (this case > should also #GP, but wouldn't without this adjustment). But the force emulation prefix specifically doesn't behave like other prefixes. It doesn't count towards the 15 byte instruction limit, and if the emulated instruction does fault, we want the fault pointing at the emulated instruction, not the force prefix. We should avoid making any link. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 14:27 ` Andrew Cooper @ 2016-06-09 15:05 ` Jan Beulich 2016-06-17 8:19 ` Ping: " Jan Beulich 2016-06-17 9:37 ` Andrew Cooper 0 siblings, 2 replies; 13+ messages in thread From: Jan Beulich @ 2016-06-09 15:05 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 09.06.16 at 16:27, <andrew.cooper3@citrix.com> wrote: > On 09/06/16 15:13, Jan Beulich wrote: >>>>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: >>> On 09/06/16 13:31, Jan Beulich wrote: >>>>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>>>> On 08/06/16 14:43, Jan Beulich wrote: >>>>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>>>> >>>>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>>>> one: There definitely needs to be at least one more opcode byte, and we >>>>>> can avoid missing a wraparound case this way. >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> I looked into this when you suggested it, but it latches the wrong eip >>>>> in the emulation state, and you will end up re-emulating the ud2a >>>>> instruction, rather than the following instruction. >>>> Where is there any latching of eip? All hvm_emulate_prepare() does >>>> is storing the regs pointer. >>> Oh - so it does. I clearly looked over it too quickly. >>> >>> What wraparound issue are you referring to? Adding 1 will cause >>> incorrect behaviour when the emulation prefix ends at the segment limit. >> I don't think so: The prefix together with the actual instruction >> encoding should be viewed as a single instruction, and it crossing >> the segment limit should #GP. It wrapping at the prefix/encoding >> boundary is the case that I'm specifically referring to (this case >> should also #GP, but wouldn't without this adjustment). > > But the force emulation prefix specifically doesn't behave like other > prefixes. > > It doesn't count towards the 15 byte instruction limit, and if the > emulated instruction does fault, we want the fault pointing at the > emulated instruction, not the force prefix. We should avoid making any > link. Well, are you saying placing such a prefix right below the boundary of a flat segment is _expected_ to get the instruction at address 0 emulated? I don't think I could buy that. The patch makes no other connection between prefix and actual insn. And #GP because of such a boundary condition should imo point at the prefix; only all faults associated with the actual insn should point there. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Ping: Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 15:05 ` Jan Beulich @ 2016-06-17 8:19 ` Jan Beulich 2016-06-17 9:37 ` Andrew Cooper 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2016-06-17 8:19 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 09.06.16 at 17:05, <JBeulich@suse.com> wrote: >>>> On 09.06.16 at 16:27, <andrew.cooper3@citrix.com> wrote: >> On 09/06/16 15:13, Jan Beulich wrote: >>>>>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: >>>> On 09/06/16 13:31, Jan Beulich wrote: >>>>>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>>>>> On 08/06/16 14:43, Jan Beulich wrote: >>>>>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>>>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>>>>> >>>>>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>>>>> one: There definitely needs to be at least one more opcode byte, and we >>>>>>> can avoid missing a wraparound case this way. >>>>>>> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> I looked into this when you suggested it, but it latches the wrong eip >>>>>> in the emulation state, and you will end up re-emulating the ud2a >>>>>> instruction, rather than the following instruction. >>>>> Where is there any latching of eip? All hvm_emulate_prepare() does >>>>> is storing the regs pointer. >>>> Oh - so it does. I clearly looked over it too quickly. >>>> >>>> What wraparound issue are you referring to? Adding 1 will cause >>>> incorrect behaviour when the emulation prefix ends at the segment limit. >>> I don't think so: The prefix together with the actual instruction >>> encoding should be viewed as a single instruction, and it crossing >>> the segment limit should #GP. It wrapping at the prefix/encoding >>> boundary is the case that I'm specifically referring to (this case >>> should also #GP, but wouldn't without this adjustment). >> >> But the force emulation prefix specifically doesn't behave like other >> prefixes. >> >> It doesn't count towards the 15 byte instruction limit, and if the >> emulated instruction does fault, we want the fault pointing at the >> emulated instruction, not the force prefix. We should avoid making any >> link. > > Well, are you saying placing such a prefix right below the boundary > of a flat segment is _expected_ to get the instruction at address 0 > emulated? I don't think I could buy that. The patch makes no other > connection between prefix and actual insn. And #GP because of > such a boundary condition should imo point at the prefix; only all > faults associated with the actual insn should point there. Ping? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-09 15:05 ` Jan Beulich 2016-06-17 8:19 ` Ping: " Jan Beulich @ 2016-06-17 9:37 ` Andrew Cooper 2016-06-17 10:01 ` Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2016-06-17 9:37 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel On 09/06/16 16:05, Jan Beulich wrote: >>>> On 09.06.16 at 16:27, <andrew.cooper3@citrix.com> wrote: >> On 09/06/16 15:13, Jan Beulich wrote: >>>>>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: >>>> On 09/06/16 13:31, Jan Beulich wrote: >>>>>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>>>>> On 08/06/16 14:43, Jan Beulich wrote: >>>>>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>>>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>>>>> >>>>>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>>>>> one: There definitely needs to be at least one more opcode byte, and we >>>>>>> can avoid missing a wraparound case this way. >>>>>>> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> I looked into this when you suggested it, but it latches the wrong eip >>>>>> in the emulation state, and you will end up re-emulating the ud2a >>>>>> instruction, rather than the following instruction. >>>>> Where is there any latching of eip? All hvm_emulate_prepare() does >>>>> is storing the regs pointer. >>>> Oh - so it does. I clearly looked over it too quickly. >>>> >>>> What wraparound issue are you referring to? Adding 1 will cause >>>> incorrect behaviour when the emulation prefix ends at the segment limit. >>> I don't think so: The prefix together with the actual instruction >>> encoding should be viewed as a single instruction, and it crossing >>> the segment limit should #GP. It wrapping at the prefix/encoding >>> boundary is the case that I'm specifically referring to (this case >>> should also #GP, but wouldn't without this adjustment). >> But the force emulation prefix specifically doesn't behave like other >> prefixes. >> >> It doesn't count towards the 15 byte instruction limit, and if the >> emulated instruction does fault, we want the fault pointing at the >> emulated instruction, not the force prefix. We should avoid making any >> link. > Well, are you saying placing such a prefix right below the boundary > of a flat segment is _expected_ to get the instruction at address 0 > emulated? I don't think I could buy that. The patch makes no other > connection between prefix and actual insn. And #GP because of > such a boundary condition should imo point at the prefix; only all > faults associated with the actual insn should point there. Ok. That sounds reasonable. Would it be possible to add a small comment to the code? Even with the commit message, I was confused as to the nature of the +1. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() 2016-06-17 9:37 ` Andrew Cooper @ 2016-06-17 10:01 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2016-06-17 10:01 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 17.06.16 at 11:37, <andrew.cooper3@citrix.com> wrote: > On 09/06/16 16:05, Jan Beulich wrote: >>>>> On 09.06.16 at 16:27, <andrew.cooper3@citrix.com> wrote: >>> On 09/06/16 15:13, Jan Beulich wrote: >>>>>>> On 09.06.16 at 16:06, <andrew.cooper3@citrix.com> wrote: >>>>> On 09/06/16 13:31, Jan Beulich wrote: >>>>>>>>> On 09.06.16 at 13:34, <andrew.cooper3@citrix.com> wrote: >>>>>>> On 08/06/16 14:43, Jan Beulich wrote: >>>>>>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare() >>>>>>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it). >>>>>>>> >>>>>>>> At once increase the length passed to hvm_virtual_to_linear_addr() by >>>>>>>> one: There definitely needs to be at least one more opcode byte, and we >>>>>>>> can avoid missing a wraparound case this way. >>>>>>>> >>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>> I looked into this when you suggested it, but it latches the wrong eip >>>>>>> in the emulation state, and you will end up re-emulating the ud2a >>>>>>> instruction, rather than the following instruction. >>>>>> Where is there any latching of eip? All hvm_emulate_prepare() does >>>>>> is storing the regs pointer. >>>>> Oh - so it does. I clearly looked over it too quickly. >>>>> >>>>> What wraparound issue are you referring to? Adding 1 will cause >>>>> incorrect behaviour when the emulation prefix ends at the segment limit. >>>> I don't think so: The prefix together with the actual instruction >>>> encoding should be viewed as a single instruction, and it crossing >>>> the segment limit should #GP. It wrapping at the prefix/encoding >>>> boundary is the case that I'm specifically referring to (this case >>>> should also #GP, but wouldn't without this adjustment). >>> But the force emulation prefix specifically doesn't behave like other >>> prefixes. >>> >>> It doesn't count towards the 15 byte instruction limit, and if the >>> emulated instruction does fault, we want the fault pointing at the >>> emulated instruction, not the force prefix. We should avoid making any >>> link. >> Well, are you saying placing such a prefix right below the boundary >> of a flat segment is _expected_ to get the instruction at address 0 >> emulated? I don't think I could buy that. The patch makes no other >> connection between prefix and actual insn. And #GP because of >> such a boundary condition should imo point at the prefix; only all >> faults associated with the actual insn should point there. > > Ok. That sounds reasonable. Would it be possible to add a small > comment to the code? Even with the commit message, I was confused as to > the nature of the +1. + /* + * Note that in the call below we pass 1 more than the signature + * size, to guard against the overall code sequence wrapping between + * "prefix" and actual instruction. There's necessarily at least one + * actual instruction byte required, so this won't cause failure on + * legitimate uses. + */ > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-17 10:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-08 13:35 [PATCH] x86/HVM: mis adjustments Jan Beulich 2016-06-08 13:42 ` [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter Jan Beulich 2016-06-09 11:25 ` Andrew Cooper 2016-06-08 13:43 ` [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept() Jan Beulich 2016-06-09 11:34 ` Andrew Cooper 2016-06-09 12:31 ` Jan Beulich 2016-06-09 14:06 ` Andrew Cooper 2016-06-09 14:13 ` Jan Beulich 2016-06-09 14:27 ` Andrew Cooper 2016-06-09 15:05 ` Jan Beulich 2016-06-17 8:19 ` Ping: " Jan Beulich 2016-06-17 9:37 ` Andrew Cooper 2016-06-17 10:01 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).