[PATCH] x86/HVM: mis adjustments

[PATCH] x86/HVM: mis adjustments

[Jan Beulich]

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

[PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter

[Jan Beulich]

[-- 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

[PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

[-- 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

Re: [PATCH 1/2] x86/HVM: constify hvm_virtual_to_linear_addr()'s segment register parameter

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

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

Ping: Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

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