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 15:14, Jan Beulich wrote:
>>>> On 06.04.17 at 11:47, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> Right.
>
>> 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.
> That's not the conclusion I would draw. There is a reason we emulate
> call gate accesses already now for 32-bit guests (just not via
> x86_emulate()) - precisely to guarantee guests need _not_ be aware.
>
>> 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.
> I don't think the validate function should do any such overriding.
> Specifically it shouldn't alter ctxt->lma, risking to surprise x86_emulate().

I have folded:

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d010aa3..96bc280 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5412,7 +5412,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned
long addr,
             .vendor = d->arch.cpuid->x86_vendor,
             .addr_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
             .sp_size   = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
-            .lma = true,
+            .lma       = !is_pv_32bit_domain(d),
         },
     };
     int rc;
@@ -5567,7 +5567,7 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned
long addr,
         .vendor = v->domain->arch.cpuid->x86_vendor,
         .addr_size = addr_size,
         .sp_size = addr_size,
-        .lma = true,
+        .lma = !is_pv_32bit_vcpu(v),
         .data = &mmio_ro_ctxt
     };
     int rc;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 09dc2f1..5b9bf21 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2966,7 +2966,7 @@ static int emulate_privileged_op(struct
cpu_user_regs *regs)
     struct priv_op_ctxt ctxt = {
         .ctxt.regs = regs,
         .ctxt.vendor = currd->arch.cpuid->x86_vendor,
-        .ctxt.lma = true,
+        .ctxt.lma = !is_pv_32bit_domain(currd),
     };
     int rc;
     unsigned int eflags, ar;


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