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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/23/2014 02:23 PM, Andrew Cooper wrote:
> 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.

What about an inline (or a macro)? It won't help with code size but is a 
good thing from code maintainability point of vew.

-boris