Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/09/14 19:20, Boris Ostrovsky wrote:
> On 09/23/2014 12:09 PM, Andrew Cooper wrote:
>> Analysis of XSAs 105 and 106 show that is possible to force a race
>> condition
>> which causes any arbitrary instruction to be emulated.
>>
>> To aid testing, explicitly introduce the Forced Emulation Prefix for
>> debug
>> builds alone.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Eddie Dong <eddie.dong@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> ---
>> v2: (all suggested by Jan)
>>   * Use hvm_fetch_from_guest_virt_nofault() in preference to
>> copy_from_guest()
>>   * Vastly reduce use of #ifndef NDEBUG
>> ---
>>   docs/misc/xen-command-line.markdown |   11 +++++++++++
>>   xen/arch/x86/hvm/hvm.c              |    5 +++++
>>   xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>>   xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>>   xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>>   5 files changed, 49 insertions(+)
>>
>> diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-command-line.markdown
>> index af93e17..389701a 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>>     Recognized in debug builds of the hypervisor only.
>>   +### hvm\_fep
>> +> `= <boolean>`
>> +
>> +> Default: `false`
>> +
>> +Allow use of the Forced Emulation Prefix in HVM guests, to allow
>> emulation of
>> +arbitrary instructions.
>> +
>> +This option is intended for development purposes, and is only
>> available in
>> +debug builds of the hypervisor.
>> +
>>   ### hvm\_port80
>>   > `= <boolean>`
>>   diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 5c7e0a4..34f28d0 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__
>> (".bss.page_aligned")))
>>   static bool_t __initdata opt_hap_enabled = 1;
>>   boolean_param("hap", opt_hap_enabled);
>>   +#ifndef opt_hvm_fep
>> +bool_t opt_hvm_fep;
>> +boolean_param("hvm_fep", opt_hvm_fep);
>> +#endif
>> +
>>   static int cpu_callback(
>>       struct notifier_block *nfb, unsigned long action, void *hcpu)
>>   {
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index b6beefc..cda968b 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct
>> cpu_user_regs *regs)
>>       struct hvm_emulate_ctxt ctxt;
>>       int rc;
>>   +    if ( opt_hvm_fep )
>> +    {
>> +        char sig[5]; /* ud2; .ascii "xen" */
>> +
>> +        if ( (hvm_fetch_from_guest_virt_nofault(
>> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
>> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
>> +        {
>> +            regs->eip += sizeof(sig);
>> +            regs->eflags &= ~X86_EFLAGS_RF;
>> +        }
>> +    }
>
> This code is exactly the same for SVM and VMX. Can it be factored out?
>
> -boris

It can, and I considered that, but it would prevent optimising to
nothing for non-debug builds.  Given that it was a single simple if()
statement, I chose not to.

~Andrew