| Message ID | 1482165475-26302-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.12.16 at 17:37, <andrew.cooper3@citrix.com> wrote: > Introduce vendor_is() to allow emulation to have vendor-specific > behaviour. Adjust the SYSCALL behaviour on Intel to raise #UD when > executed outside of 64bit mode. I'd rather not see us go this route. I've been carrying a patch making the vendor an input (not submitted so far due to other at least contextual prereqs missing when I last check, but sent out just a minute ago). What do you think? > in_longmode() has different return semantics from rc, so use a separate > integer for the purpose. The ugliness of the additional #ifdef doesn't seem to warrant this. What I've been thinking the other day though is: Why don't we put the whole SYSCALL emulation into a __XEN__ conditional (implying __x86_64__, i.e. allowing the inner ones to be removed)? That would likely also limit the impact of the pending register name changes which I hope to be able to submit soon (once enough prereqs have gone in). Jan
On 20/12/2016 08:22, Jan Beulich wrote: >>>> On 19.12.16 at 17:37, <andrew.cooper3@citrix.com> wrote: >> Introduce vendor_is() to allow emulation to have vendor-specific >> behaviour. Adjust the SYSCALL behaviour on Intel to raise #UD when >> executed outside of 64bit mode. > I'd rather not see us go this route. I've been carrying a patch > making the vendor an input (not submitted so far due to other > at least contextual prereqs missing when I last check, but sent > out just a minute ago). What do you think? Rebasing on top of that would be far more simple. > >> in_longmode() has different return semantics from rc, so use a separate >> integer for the purpose. > The ugliness of the additional #ifdef doesn't seem to warrant > this. It is a matter of correctness. The only reason we exit from it cleanly is because the cannot_emulate path discards rc, while the other paths overwrite rc because of hitting the read_msr() or write_segment() hooks in each case. > What I've been thinking the other day though is: Why > don't we put the whole SYSCALL emulation into a __XEN__ > conditional (implying __x86_64__, i.e. allowing the inner ones > to be removed)? This depends on whether we think it is ever realistic to be able to be able to test things like this in the userspace harness. I am not sure we realistically can. ~Andrew
>>> On 20.12.16 at 13:14, <andrew.cooper3@citrix.com> wrote: > On 20/12/2016 08:22, Jan Beulich wrote: >>>>> On 19.12.16 at 17:37, <andrew.cooper3@citrix.com> wrote: >> What I've been thinking the other day though is: Why >> don't we put the whole SYSCALL emulation into a __XEN__ >> conditional (implying __x86_64__, i.e. allowing the inner ones >> to be removed)? > > This depends on whether we think it is ever realistic to be able to be > able to test things like this in the userspace harness. I am not sure > we realistically can. Well - it's all about register modifications, which we surely could test. But this would be a rather contrived test, the usefulness of which I would question. Jan
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 165eebb..419c2da 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1327,6 +1327,45 @@ static bool vcpu_has( #define host_and_vcpu_must_have(feat) vcpu_must_have(feat) #endif +#define X86_VENDOR_INTEL 0 +#define X86_VENDOR_AMD 2 + +static bool vendor_is( + struct x86_emulate_ctxt *ctxt, + const struct x86_emulate_ops *ops, + int vendor) +{ + unsigned int eax = 0, ebx = 0, ecx = 0, edx = 0; + int rc = X86EMUL_OKAY; + + fail_if(!ops->cpuid); + rc = ops->cpuid(&eax, &ebx, &ecx, &edx, ctxt); + if ( rc == X86EMUL_OKAY ) + { + switch ( vendor ) + { + case X86_VENDOR_INTEL: + return (ebx == 0x756e6547u && /* "GenuineIntel" */ + ecx == 0x6c65746eu && + edx == 0x49656e69u); + + case X86_VENDOR_AMD: + return (ebx == 0x68747541u && /* "AuthenticAMD" */ + ecx == 0x444d4163u && + edx == 0x69746e65u); + default: + rc = ~X86EMUL_OKAY; + break; + } + } + + done: + return rc == X86EMUL_OKAY; +} + +#define vendor_is_intel() vendor_is(ctxt, ops, X86_VENDOR_INTEL) +#define vendor_is_amd() vendor_is(ctxt, ops, X86_VENDOR_AMD) + static int in_longmode( struct x86_emulate_ctxt *ctxt, @@ -4623,10 +4662,15 @@ x86_emulate( break; } - case X86EMUL_OPC(0x0f, 0x05): /* syscall */ { + case X86EMUL_OPC(0x0f, 0x05): /* syscall */ + { uint64_t msr_content; +#ifdef __x86_64__ + int lm; +#endif - generate_exception_if(!in_protmode(ctxt, ops), EXC_UD); + generate_exception_if(!in_protmode(ctxt, ops) || + (vendor_is_intel() && !mode_64bit()), EXC_UD); /* Inject #UD if syscall/sysret are disabled. */ fail_if(ops->read_msr == NULL); @@ -4645,10 +4689,10 @@ x86_emulate( sreg.attr.bytes = 0xc93; /* G+DB+P+S+Data */ #ifdef __x86_64__ - rc = in_longmode(ctxt, ops); - if ( rc < 0 ) + lm = in_longmode(ctxt, ops); + if ( lm < 0 ) goto cannot_emulate; - if ( rc ) + if ( lm ) { cs.attr.bytes = 0xa9b; /* L+DB+P+S+Code */
Introduce vendor_is() to allow emulation to have vendor-specific behaviour. Adjust the SYSCALL behaviour on Intel to raise #UD when executed outside of 64bit mode. in_longmode() has different return semantics from rc, so use a separate integer for the purpose. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/x86_emulate/x86_emulate.c | 54 ++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 5 deletions(-)