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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.02.18 at 11:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Granted I didn't realize at the time that breakpoints would go all
silent.

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

I'd be very much in favor of this, if only there was a single byte insn
documented to cause #UD now and forever. Abusing what is INTO or
SALC outside of 64-bit mode doesn't look very attractive.

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

Oh, if it goes away by the end of the series, then that's fine.
(When replying here I hadn't looked at the full patch yet, so please
accept my apologies if this is properly explained in the description.)

Jan


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