Re: [Xen-devel] [PATCH for 4.9 6/6] x86/emul: Require callers to provide LMA in the emulation context

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/04/17 07:58, Jan Beulich wrote:
>>>> On 05.04.17 at 18:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 03/04/17 11:10, Jan Beulich wrote:
>>>>>> On 31.03.17 at 21:50, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -5410,6 +5410,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long 
>>>> addr,
>>>>          .ctxt = {
>>>>              .regs = regs,
>>>>              .vendor = d->arch.cpuid->x86_vendor,
>>>> +            .lma = true,
>>>>              .addr_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
>>>>              .sp_size   = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
>>>>          },
>>>> @@ -5564,6 +5565,7 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned 
>>>> long addr,
>>>>      struct x86_emulate_ctxt ctxt = {
>>>>          .regs = regs,
>>>>          .vendor = v->domain->arch.cpuid->x86_vendor,
>>>> +        .lma = true,
>>> Hmm, both of these are correct from Xen's pov, but potentially
>>> wrong from the guest's. Since system segments aren't being
>>> dealt with here, I think this difference is benign, but I think it
>>> warrants at least a comment. If we ever meant to emulate
>>> LLDT, this would become at active problem, as the guest's view
>>> on gate descriptor layout would differ from that resulting from
>>> setting .lma to true here. Same for emulate_privileged_op() then.
>> As discovered in the meantime, things like LLDT/LTR and call gates are
>> far more complicated.
>>
>> Still, setting LMA to true here is the right thing to do, as it is an
>> accurate statement of processor state.  Despite the level of
>> compatibility for 32bit, a 32bit PV guest isn't entirely isolated from
>> the fact that Xen is 64bit.
> Yes, but still call gates (which we don't currently handle in the
> emulator itself) require 32-bit treatment for 32-bit guests, so
> setting lma to true would still seem wrong.

I thought you said that a compatibility mode `call $gate` still checked
the type in the high 8 bytes.

A 32bit PV guest therefore needs to be aware that it can't position call
gates adjacently, or it will suffer #GP[sel] for a failed typecheck.


Now, in this specific case we are in a position to cope, because either
way we end up in the gate op code, but if we wanted to override hardware
behaviour, it should be the validate function, which positively
identifies a far call/jmp, which should choose to override lma for the
purposes of faking up legacy mode behaviour.

>  And the value of lma
> is, as we now know, irrelevant for LDT and TSS descriptors (I'm
> about to verify AMD behaves identically to Intel here).

I checked AMD when I was testing this quirk.  The behaviour does appear
to be the same.

>
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>>> @@ -457,6 +457,9 @@ struct x86_emulate_ctxt
>>>>      /* Set this if writes may have side effects. */
>>>>      bool force_writeback;
>>>>  
>>>> +    /* Long mode active? */
>>>> +    bool lma;
>>> I don't think this should be in the input only section, as an insn
>>> can have the effect of clearing it.
>>>
>>> Also should x86_emulate_wrapper() perhaps assert the field to
>>> be false for 32-bit (test harness) builds?
>> I'd prefer not to.  There is nothing in principle wrong with trying to
>> feed a 32bit compatibility segment through a 32bit build of the test
>> harness.
> Actually I've just figured myself that such an assertion would better
> be checking that mode_64bit() implies lma to be set.

Good point.  I have folded:

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 1d5a698..59c2da8 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -7895,7 +7895,12 @@ int x86_emulate_wrapper(
     const struct x86_emulate_ops *ops)
 {
     unsigned long orig_ip = ctxt->regs->r(ip);
-    int rc = x86_emulate(ctxt, ops);
+    int rc;
+
+    if ( mode_64bit() )
+        ASSERT(ctxt->lma);
+
+    rc = x86_emulate(ctxt, ops);
 
     /* Retire flags should only be set for successful instruction
emulation. */
     if ( rc != X86EMUL_OKAY )

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel