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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.17 at 18:32, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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;

With that
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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