Message ID | d373796d-eb4d-e5ad-9e0c-a03d70aff2c0@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 12/16/16 19:47 +0000, Andrew Cooper wrote: >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" ? > Yes, I will use your suggestion. >> + */ >> +#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 Great! It doesn't only help to simplify the logic in this patch, but also helps later patches that pass VMXERR_ flags from exec_user(). Thanks for the suggestion! > * - 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. > will change >> +{ >> + 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. > will remove them >> + "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. > ditto >> + >> + 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. > Exactly, thanks! Haozhong
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 */ /*