All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>,
	Paul Durrant <paul.durrant@citrix.com>, Tim Deegan <tim@xen.org>,
	Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH for 4.9 6/6] x86/emul: Require callers to provide LMA in the emulation context
Date: Thu, 6 Apr 2017 10:47:47 +0100	[thread overview]
Message-ID: <15376186-772a-75a3-eb2e-cc268ccdd4a4@citrix.com> (raw)
In-Reply-To: <58E6033B020000780014DAAE@prv-mh.provo.novell.com>

On 06/04/17 07:58, Jan Beulich wrote:
>>>> On 05.04.17 at 18:24, <andrew.cooper3@citrix.com> wrote:
>> On 03/04/17 11:10, Jan Beulich wrote:
>>>>>> On 31.03.17 at 21:50, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

  reply	other threads:[~2017-04-06  9:47 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-31 19:50 [PATCH for 4.9 0/6] x86/emul: Fixes Andrew Cooper
2017-03-31 19:50 ` [PATCH for 4.9 1/6] x86/hvm: Correct some address space terminology Andrew Cooper
2017-04-03  8:24   ` Paul Durrant
2017-04-03  8:24   ` Jan Beulich
2017-04-03 10:19     ` Andrew Cooper
2017-04-03 10:29       ` Jan Beulich
2017-03-31 19:50 ` [PATCH for 4.9 2/6] x86/hvm: Correct long mode predicate Andrew Cooper
2017-04-03  8:26   ` Paul Durrant
2017-04-03  8:30   ` Jan Beulich
2017-04-03  8:50   ` George Dunlap
2017-04-05  7:08   ` Tian, Kevin
2017-03-31 19:50 ` [PATCH for 4.9 3/6] x86/hvm: Fix segmentation logic for system segments Andrew Cooper
2017-04-03  8:31   ` Paul Durrant
2017-04-03  9:13   ` Jan Beulich
2017-04-03 14:27     ` Andrew Cooper
2017-04-03 15:07       ` Jan Beulich
2017-04-03 15:42         ` Andrew Cooper
2017-04-03 16:08           ` Jan Beulich
2017-04-03 17:37             ` Andrew Cooper
2017-04-04 10:18               ` Andrew Cooper
2017-04-04 10:32                 ` Jan Beulich
2017-03-31 19:50 ` [PATCH for 4.9 4/6] x86/svm: Introduce svm_emul_swint_injection() Andrew Cooper
2017-04-03  9:30   ` Jan Beulich
2017-04-03 14:04   ` Boris Ostrovsky
2017-03-31 19:50 ` [PATCH for 4.9 5/6] x86/emul: Drop swint_emulate infrastructure Andrew Cooper
2017-04-03  8:36   ` Paul Durrant
2017-04-03  9:38   ` Jan Beulich
2017-03-31 19:50 ` [PATCH for 4.9 6/6] x86/emul: Require callers to provide LMA in the emulation context Andrew Cooper
2017-04-03  8:40   ` Paul Durrant
2017-04-03 10:10   ` Jan Beulich
2017-04-05 16:24     ` Andrew Cooper
2017-04-06  6:58       ` Jan Beulich
2017-04-06  9:47         ` Andrew Cooper [this message]
2017-04-06 14:14           ` Jan Beulich
2017-04-06 16:32             ` Andrew Cooper
2017-04-07  8:35               ` Jan Beulich
2017-04-05 16:07   ` Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=15376186-772a-75a3-eb2e-cc268ccdd4a4@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=julien.grall@arm.com \
    --cc=paul.durrant@citrix.com \
    --cc=tim@xen.org \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.