Re: [Xen-devel] [XTF PATCH 04/16] vvmx: add C wrappers of vmxon/vmread/vmptrld

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/12/16 13:43, Haozhong Zhang wrote:

Review of the technical side of the patch, leaving the licensing to the
other thread.

Reordering for better logical clarity for my suggestion.

> diff --git a/tests/vvmx/util.h b/tests/vvmx/util.h
> new file mode 100644
> index 0000000..57d3398
> --- /dev/null
> +++ b/tests/vvmx/util.h
> @@ -0,0 +1,78 @@
> +#ifndef XTF_TESTS_VVMX_UTIL_H
> +#define XTF_TESTS_VVMX_UTIL_H
> +
> +#include <arch/x86/exinfo.h>
> +#include <arch/x86/hvm/vmx/vmcs.h>
> +
> +/**
> + * Flags for errors during the execution of a VMX instruction.
> + *
> + * NB. Besides VMXERR_NOERR, other flags are not mutually exclusive,
> + * because we should not assume Xen implements the nested VMX
> + * instructions correctly.

I understand what you are trying to say here, although phrasing it like
this isn't great.

How about "these flags are not mutually exclusive because we need to
test all aspects of Xen's behaviour" ?

> + */
> +#define VMXERR_NOERR          0
> +#define VMXERR_VMFAIL_VALID   (1 << 0)
> +#define VMXERR_VMFAIL_INVALID (1 << 1)
> +#define VMXERR_FAULT          (1 << 2)

As for the flags themselves, they are very closely related to the
existing exinfo_t infrastructure, and I see that you pass around quite a
lot of fault/error state below.

There are 23 spare bits in exinfo_t, and I don't forsee any more general
use in it.

XTF already has things like GDTE_AVAIL{0...3} which are dedicated
resources available to tests.  If it helps simplify the logic later, I
think it would be entirely reasonable to formally reserve some bits in
extinfo_t for test-specific use.

For example, something like:

diff --git a/include/arch/x86/exinfo.h b/include/arch/x86/exinfo.h
index eef39b6..1e9c10b 100644
--- a/include/arch/x86/exinfo.h
+++ b/include/arch/x86/exinfo.h
@@ -13,6 +13,7 @@
  *
  * - Bottom 16 bits are error code
  * - Next 8 bits are the entry vector
+ * - Next 4 bits reserved for test-specific information
  * - Top bit it set to disambiguate @#DE from no exception
  */
 typedef unsigned int exinfo_t;
@@ -33,6 +34,9 @@ static inline unsigned int exinfo_ec(exinfo_t info)
     return info & 0xffff;
 }
 
+/* Bits reserved for test-specific information. */
+#define EXINFO_BIT_AVAIL0 24
+#define EXINFO_BIT_AVAIL1 25
+#define EXINFO_BIT_AVAIL2 26
+#define EXINFO_BIT_AVAIL3 27
+#define EXINFO_AVAIL_MASK (0xfu << EXINFO_BIT_AVAIL0)
+
 #endif /* XTF_X86_EXINFO_H */
 
 /*

and,

#define VMXERR_VMFAIL_VALID   (1u << EXINFO_BIT_AVAIL0)
#define VMXERR_VMFAIL_INVALID (1u << EXINFO_BIT_AVAIL1)

which would allow you to return all vmx-instruction related error
information in a single exinfo_t.

> diff --git a/tests/vvmx/util.c b/tests/vvmx/util.c
> new file mode 100644
> index 0000000..8cd35c5
> --- /dev/null
> +++ b/tests/vvmx/util.c
> @@ -0,0 +1,83 @@
> +#include <xtf.h>
> +#include <arch/x86/hvm/vmx/vmcs.h>
> +#include "util.h"
> +
> +#define VMPTRLD_OPCODE  ".byte 0x0f,0xc7\n"             /* reg/opcode: /6 */
> +#define VMREAD_OPCODE   ".byte 0x0f,0x78\n"
> +#define VMXON_OPCODE    ".byte 0xf3,0x0f,0xc7\n"
> +
> +#define MODRM_EAX_06    ".byte 0x30\n" /* [EAX], with reg/opcode: /6 */
> +#define MODRM_EAX_ECX   ".byte 0xc1\n" /* EAX, ECX */
> +
> +uint8_t vmxon(uint64_t paddr, exinfo_t *fault_info)

paddr_t please.

> +{
> +    exinfo_t fault = 0;
> +    uint8_t valid = 0, invalid = 0;
> +
> +    asm volatile("1: "VMXON_OPCODE MODRM_EAX_06 "\n\t"
> +                 "   setc %0; setz %1 \n\t"

I tend not to use \n\t in inline assembly, because it detracts from the
C code, and have never found it useful for the few occasions I have
needed to debug a .E file.

> +                 "2: \n\t"
> +                 _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_edi)
> +                 : "=q" (invalid), "=q" (valid), "+D" (fault)
> +                 : "a" (&paddr)
> +                 : "memory", "cc");

"cc" clobbers are unilaterally assumed for x86.  No need to specify them.

> +
> +    if ( fault && fault_info )
> +        *fault_info = fault;
> +
> +    return (fault ? VMXERR_FAULT : 0) |
> +           (valid ? VMXERR_VMFAIL_VALID : 0) |
> +           (invalid ? VMXERR_VMFAIL_INVALID : 0);

If you chose to use the exinfo_t proposal above, you can do something
rather more cunning here.

bool ex_record_vmxerr_edi(struct cpu_regs *regs, const struct
extable_entry *ex)
{
    if ( regs->flags & X86_EFLAGS_CF )
        regs->di |= VMXERR_VMFAIL_INVALID;
    if ( regs->flags & X86_EFLAGS_ZF )
        regs->di |= VMXERR_VMFAIL_VALID;

    regs->ip = ex->fixup;

    return true;
}

and

    asm volatile ("1:" VMXON_OPCODE MODRM_EAX_06
                  "2: ud2a;"
                  "3:"
                  _ASM_EXTABLE_HANDLER(1b, 3b, ex_record_fault_edi)
                  _ASM_EXTABLE_HANDLER(2b, 3b, ex_record_vmxerr_edi)
                  ...

This would avoid the repetitive common tail to all of these functions,
and get all of your error information into edi in one go.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel