Re: [Xen-devel] [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/02/2018 09:56, Jan Beulich wrote:
>>>> On 12.02.18 at 13:30, <wei.liu2@xxxxxxxxxx> wrote:
>> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>>> index 58f652d..bd3819a 100644
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>>          jz    exception_with_ints_disabled
>>>  
>>> -.Lcr4_pv32_orig:
>>> -        jmp   .Lcr4_pv32_done
>>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - 
>>> .Lcr4_pv32_orig), 0xcc
>>> -        .pushsection .altinstr_replacement, "ax"
>>> -.Lcr4_pv32_alt:
>>> -        mov   VCPU_domain(%rbx),%rax
>>> -.Lcr4_pv32_alt_end:
>>> -        .section .altinstructions, "a"
>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>> -                             X86_FEATURE_XEN_SMEP, \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>> -                             X86_FEATURE_XEN_SMAP, \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>> -        .popsection
>>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
>> This changed 0xcc to 0x90 but since it is just padding following an
>> unconditional jmp so it shouldn't matter.
> Well, it was for that very reason that I had picked 0xcc originally:
> We better know if some branch mistakenly leads into that region.

Know how?  At the time you wrote this, Xen silently executed its way
through debug traps, and it took some persuading to get you to ok the
patch which at least printed a line every time we a breakpoint in
hypervisor space.

If you actually want to notice going down the wrong path, then you want
a BUG.

> I also very much object to the literal 2 passed as an argument to
> .skip above: What if the label moves out far enough that a short
> branch won't be usable anymore?

Is the commit message not enough?  a) its not going to change, because
it hasn't changed since you put the code in originally and I don't
expect it to in the future, and b) it is a temporary necessary
requirement to make the series bisectable and reviewable.  This skip is
dropped in patch 6 when the automatic padding calculations work.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel