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 21:17, Boris Ostrovsky wrote:
> 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
>

I have vehement dislike of macros for things like this, where a static
inline should be used.

When attempting to make a static inline, the use of <asm/hvm/support.h>
in include/asm-x86/hvm.h (which is the only logical place for it IMO)
causes a failure to compile due to include dependency issues, and an
incomplete type definitions of struct domain and vcpu.

I currently lack enough TUITs to investigate.

~Andrew