The first and third patches are cleaned up versions of an earlier v3 submission by Yang. 1: Nested VMX: check VMX capability before read VMX related MSRs 2: VMX: clean up capability checks 3: Nested VMX: fix IA32_VMX_CR4_FIXED1 msr emulation 4: x86: make hvm_cpuid() tolerate NULL pointers Signed-off-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich
2013-Sep-23 10:10 UTC
[PATCH v4 1/4] Nested VMX: check VMX capability before read VMX related MSRs
VMX MSRs only available when the CPU support the VMX feature. In addition,
VMX_TRUE* MSRs only available when bit 55 of VMX_BASIC MSR is set.
Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Cleanup.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -78,6 +78,7 @@ static DEFINE_PER_CPU(struct list_head,
static DEFINE_PER_CPU(bool_t, vmxon);
static u32 vmcs_revision_id __read_mostly;
+u64 __read_mostly vmx_basic_msr;
static void __init vmx_display_features(void)
{
@@ -301,6 +302,8 @@ static int vmx_init_vmcs_config(void)
vmx_vmexit_control = _vmx_vmexit_control;
vmx_vmentry_control = _vmx_vmentry_control;
cpu_has_vmx_ins_outs_instr_info = !!(vmx_basic_msr_high &
(1U<<22));
+ vmx_basic_msr = ((u64)vmx_basic_msr_high << 32) |
+ vmx_basic_msr_low;
vmx_display_features();
}
else
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1813,12 +1813,33 @@ int nvmx_handle_invvpid(struct cpu_user_
int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
{
struct vcpu *v = current;
+ unsigned int ecx, dummy;
u64 data = 0, host_data = 0;
int r = 1;
if ( !nestedhvm_enabled(v->domain) )
return 0;
+ /* VMX capablity MSRs are available only when guest supports VMX. */
+ hvm_cpuid(0x1, &dummy, &dummy, &ecx, &dummy);
+ if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
+ return 0;
+
+ /*
+ * Those MSRs are available only when bit 55 of
+ * MSR_IA32_VMX_BASIC is set.
+ */
+ switch ( msr )
+ {
+ case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
+ case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
+ case MSR_IA32_VMX_TRUE_EXIT_CTLS:
+ case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+ if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
+ return 0;
+ break;
+ }
+
rdmsrl(msr, host_data);
/*
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -284,6 +284,8 @@ extern bool_t cpu_has_vmx_ins_outs_instr
*/
#define VMX_BASIC_DEFAULT1_ZERO (1ULL << 55)
+extern u64 vmx_basic_msr;
+
/* Guest interrupt status */
#define VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK 0x0FF
#define VMX_GUEST_INTR_STATUS_SVI_OFFSET 8
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
VMCS size validation on APs should check against BP''s size.
No need for a separate cpu_has_vmx_ins_outs_instr_info variable
anymore.
Use proper symbolics.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -70,7 +70,6 @@ u32 vmx_secondary_exec_control __read_mo
u32 vmx_vmexit_control __read_mostly;
u32 vmx_vmentry_control __read_mostly;
u64 vmx_ept_vpid_cap __read_mostly;
-bool_t cpu_has_vmx_ins_outs_instr_info __read_mostly;
static DEFINE_PER_CPU_READ_MOSTLY(struct vmcs_struct *, vmxon_region);
static DEFINE_PER_CPU(struct vmcs_struct *, current_vmcs);
@@ -294,24 +293,33 @@ static int vmx_init_vmcs_config(void)
if ( !vmx_pin_based_exec_control )
{
/* First time through. */
- vmcs_revision_id = vmx_basic_msr_low;
+ vmcs_revision_id = vmx_basic_msr_low &
VMX_BASIC_REVISION_MASK;
vmx_pin_based_exec_control = _vmx_pin_based_exec_control;
vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
vmx_secondary_exec_control = _vmx_secondary_exec_control;
vmx_ept_vpid_cap = _vmx_ept_vpid_cap;
vmx_vmexit_control = _vmx_vmexit_control;
vmx_vmentry_control = _vmx_vmentry_control;
- cpu_has_vmx_ins_outs_instr_info = !!(vmx_basic_msr_high &
(1U<<22));
vmx_basic_msr = ((u64)vmx_basic_msr_high << 32) |
vmx_basic_msr_low;
vmx_display_features();
+
+ /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
+ if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32))
>
+ PAGE_SIZE )
+ {
+ printk("VMX: CPU%d VMCS size is too big (%Lu bytes)\n",
+ smp_processor_id(),
+ vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >>
32));
+ return -EINVAL;
+ }
}
else
{
/* Globals are already initialised: re-check them. */
mismatch |= cap_check(
"VMCS revision ID",
- vmcs_revision_id, vmx_basic_msr_low);
+ vmcs_revision_id, vmx_basic_msr_low & VMX_BASIC_REVISION_MASK);
mismatch |= cap_check(
"Pin-Based Exec Control",
vmx_pin_based_exec_control, _vmx_pin_based_exec_control);
@@ -331,13 +339,21 @@ static int vmx_init_vmcs_config(void)
"EPT and VPID Capability",
vmx_ept_vpid_cap, _vmx_ept_vpid_cap);
if ( cpu_has_vmx_ins_outs_instr_info !-
!!(vmx_basic_msr_high & (1U<<22)) )
+ !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32))
)
{
printk("VMX INS/OUTS Instruction Info: saw %d expected
%d\n",
- !!(vmx_basic_msr_high & (1U<<22)),
+ !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >>
32)),
cpu_has_vmx_ins_outs_instr_info);
mismatch = 1;
}
+ if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32))
!+ ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) )
+ {
+ printk("VMX: CPU%d unexpected VMCS size %Lu\n",
+ smp_processor_id(),
+ vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >>
32));
+ mismatch = 1;
+ }
if ( mismatch )
{
printk("VMX: Capabilities fatally differ between CPU%d and
CPU0\n",
@@ -346,16 +362,8 @@ static int vmx_init_vmcs_config(void)
}
}
- /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
- if ( (vmx_basic_msr_high & 0x1fff) > PAGE_SIZE )
- {
- printk("VMX: CPU%d VMCS size is too big (%u bytes)\n",
- smp_processor_id(), vmx_basic_msr_high & 0x1fff);
- return -EINVAL;
- }
-
/* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
- if ( vmx_basic_msr_high & (1u<<16) )
+ if ( vmx_basic_msr_high & (VMX_BASIC_32BIT_ADDRESSES >> 32) )
{
printk("VMX: CPU%d limits VMX structure pointers to 32
bits\n",
smp_processor_id());
@@ -363,10 +371,12 @@ static int vmx_init_vmcs_config(void)
}
/* Require Write-Back (WB) memory type for VMCS accesses. */
- if ( ((vmx_basic_msr_high >> 18) & 15) != 6 )
+ opt = (vmx_basic_msr_high & (VMX_BASIC_MEMORY_TYPE_MASK >> 32)) /
+ ((VMX_BASIC_MEMORY_TYPE_MASK & -VMX_BASIC_MEMORY_TYPE_MASK)
>> 32);
+ if ( opt != MTRR_TYPE_WRBACK )
{
printk("VMX: CPU%d has unexpected VMCS access type %u\n",
- smp_processor_id(), (vmx_basic_msr_high >> 18) & 15);
+ smp_processor_id(), opt);
return -EINVAL;
}
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -210,8 +210,6 @@ extern u32 vmx_vmentry_control;
#define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING 0x00004000
extern u32 vmx_secondary_exec_control;
-extern bool_t cpu_has_vmx_ins_outs_instr_info;
-
#define VMX_EPT_EXEC_ONLY_SUPPORTED 0x00000001
#define VMX_EPT_WALK_LENGTH_4_SUPPORTED 0x00000040
#define VMX_EPT_MEMORY_TYPE_UC 0x00000100
@@ -278,6 +276,12 @@ extern bool_t cpu_has_vmx_ins_outs_instr
#define VMX_INTR_SHADOW_SMI 0x00000004
#define VMX_INTR_SHADOW_NMI 0x00000008
+#define VMX_BASIC_REVISION_MASK 0x7fffffff
+#define VMX_BASIC_VMCS_SIZE_MASK (0x1fffULL << 32)
+#define VMX_BASIC_32BIT_ADDRESSES (1ULL << 48)
+#define VMX_BASIC_DUAL_MONITOR (1ULL << 49)
+#define VMX_BASIC_MEMORY_TYPE_MASK (0xfULL << 50)
+#define VMX_BASIC_INS_OUT_INFO (1ULL << 54)
/*
* bit 55 of IA32_VMX_BASIC MSR, indicating whether any VMX controls that
* default to 1 may be cleared to 0.
@@ -285,6 +289,8 @@ extern bool_t cpu_has_vmx_ins_outs_instr
#define VMX_BASIC_DEFAULT1_ZERO (1ULL << 55)
extern u64 vmx_basic_msr;
+#define cpu_has_vmx_ins_outs_instr_info \
+ (!!(vmx_basic_msr & VMX_BASIC_INS_OUT_INFO))
/* Guest interrupt status */
#define VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK 0x0FF
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-23 10:11 UTC
[PATCH v4 3/4] Nested VMX: fix IA32_VMX_CR4_FIXED1 msr emulation
Currently, it use hardcode value for IA32_VMX_CR4_FIXED1. This is wrong.
We should check guest''s cpuid to know which bits are writeable in CR4
by guest
and allow the guest to set the corresponding bit only when guest has the
feature.
Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Cleanup.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1813,7 +1813,7 @@ int nvmx_handle_invvpid(struct cpu_user_
int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
{
struct vcpu *v = current;
- unsigned int ecx, dummy;
+ unsigned int eax, ebx, ecx, edx, dummy;
u64 data = 0, host_data = 0;
int r = 1;
@@ -1821,7 +1821,7 @@ int nvmx_msr_read_intercept(unsigned int
return 0;
/* VMX capablity MSRs are available only when guest supports VMX. */
- hvm_cpuid(0x1, &dummy, &dummy, &ecx, &dummy);
+ hvm_cpuid(0x1, &dummy, &dummy, &ecx, &edx);
if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
return 0;
@@ -1945,8 +1945,55 @@ int nvmx_msr_read_intercept(unsigned int
data = X86_CR4_VMXE;
break;
case MSR_IA32_VMX_CR4_FIXED1:
- /* allow 0-settings except SMXE */
- data = 0x267ff & ~X86_CR4_SMXE;
+ if ( edx & cpufeat_mask(X86_FEATURE_VME) )
+ data |= X86_CR4_VME | X86_CR4_PVI;
+ if ( edx & cpufeat_mask(X86_FEATURE_TSC) )
+ data |= X86_CR4_TSD;
+ if ( edx & cpufeat_mask(X86_FEATURE_DE) )
+ data |= X86_CR4_DE;
+ if ( edx & cpufeat_mask(X86_FEATURE_PSE) )
+ data |= X86_CR4_PSE;
+ if ( edx & cpufeat_mask(X86_FEATURE_PAE) )
+ data |= X86_CR4_PAE;
+ if ( edx & cpufeat_mask(X86_FEATURE_MCE) )
+ data |= X86_CR4_MCE;
+ if ( edx & cpufeat_mask(X86_FEATURE_PGE) )
+ data |= X86_CR4_PGE;
+ if ( edx & cpufeat_mask(X86_FEATURE_FXSR) )
+ data |= X86_CR4_OSFXSR;
+ if ( edx & cpufeat_mask(X86_FEATURE_XMM) )
+ data |= X86_CR4_OSXMMEXCPT;
+ if ( ecx & cpufeat_mask(X86_FEATURE_VMXE) )
+ data |= X86_CR4_VMXE;
+ if ( ecx & cpufeat_mask(X86_FEATURE_SMXE) )
+ data |= X86_CR4_SMXE;
+ if ( ecx & cpufeat_mask(X86_FEATURE_PCID) )
+ data |= X86_CR4_PCIDE;
+ if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
+ data |= X86_CR4_OSXSAVE;
+
+ hvm_cpuid(0x0, &eax, &dummy, &dummy, &dummy);
+ switch ( eax )
+ {
+ default:
+ hvm_cpuid(0xa, &eax, &dummy, &dummy, &dummy);
+ /* Check whether guest has the perf monitor feature. */
+ if ( (eax & 0xff) && (eax & 0xff00) )
+ data |= X86_CR4_PCE;
+ /* fall through */
+ case 0x7 ... 0x9:
+ ecx = 0;
+ hvm_cpuid(0x7, &dummy, &ebx, &ecx, &dummy);
+ if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
+ data |= X86_CR4_FSGSBASE;
+ if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
+ data |= X86_CR4_SMEP;
+ if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
+ data |= X86_CR4_SMAP;
+ /* fall through */
+ case 0x0 ... 0x6:
+ break;
+ }
break;
case MSR_IA32_VMX_MISC:
/* Do not support CR3-target feature now */
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -148,6 +148,7 @@
#define X86_FEATURE_INVPCID (7*32+10) /* Invalidate Process Context ID */
#define X86_FEATURE_RTM (7*32+11) /* Restricted Transactional Memory */
#define X86_FEATURE_NO_FPU_SEL (7*32+13) /* FPU CS/DS stored as zero */
+#define X86_FEATURE_SMAP (7*32+20) /* Supervisor Mode Access Prevention */
#define cpu_has(c, bit) test_bit(bit, (c)->x86_capability)
#define boot_cpu_has(bit) test_bit(bit, boot_cpu_data.x86_capability)
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -87,6 +87,7 @@
#define X86_CR4_PCIDE 0x20000 /* enable PCID */
#define X86_CR4_OSXSAVE 0x40000 /* enable XSAVE/XRSTOR */
#define X86_CR4_SMEP 0x100000/* enable SMEP */
+#define X86_CR4_SMAP 0x200000/* enable SMAP */
/*
* Trap/fault mnemonics.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-23 10:12 UTC
[PATCH v4 4/4] x86: make hvm_cpuid() tolerate NULL pointers
Now that we other HVM code start making more extensive use of
hvm_cpuid(), let''s not force every caller to declare dummy variables
for output not cared about.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2757,7 +2757,17 @@ void hvm_cpuid(unsigned int input, unsig
{
struct vcpu *v = current;
struct domain *d = v->domain;
- unsigned int count = *ecx;
+ unsigned int count, dummy = 0;
+
+ if ( !eax )
+ eax = &dummy;
+ if ( !ebx )
+ ebx = &dummy;
+ if ( !ecx )
+ ecx = &dummy;
+ count = *ecx;
+ if ( !edx )
+ edx = &dummy;
if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
return;
@@ -2765,7 +2775,7 @@ void hvm_cpuid(unsigned int input, unsig
if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) )
return;
- domain_cpuid(d, input, *ecx, eax, ebx, ecx, edx);
+ domain_cpuid(d, input, count, eax, ebx, ecx, edx);
switch ( input )
{
@@ -2853,15 +2863,15 @@ int hvm_msr_read_intercept(unsigned int
{
struct vcpu *v = current;
uint64_t *var_range_base, *fixed_range_base;
- int index, mtrr;
- uint32_t cpuid[4];
+ bool_t mtrr;
+ unsigned int edx, index;
int ret = X86EMUL_OKAY;
var_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.var_ranges;
fixed_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.fixed_ranges;
- hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]);
- mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR));
+ hvm_cpuid(1, NULL, NULL, NULL, &edx);
+ mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR));
switch ( msr )
{
@@ -2969,15 +2979,15 @@ int hvm_msr_read_intercept(unsigned int
int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
{
struct vcpu *v = current;
- int index, mtrr;
- uint32_t cpuid[4];
+ bool_t mtrr;
+ unsigned int edx, index;
int ret = X86EMUL_OKAY;
HVMTRACE_3D(MSR_WRITE, msr,
(uint32_t)msr_content, (uint32_t)(msr_content >> 32));
- hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]);
- mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR));
+ hvm_cpuid(1, NULL, NULL, NULL, &edx);
+ mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR));
hvm_memory_event_msr(msr, msr_content);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -806,13 +806,13 @@ static inline void svm_lwp_load(struct v
/* Update LWP_CFG MSR (0xc0000105). Return -1 if error; otherwise returns 0. */
static int svm_update_lwp_cfg(struct vcpu *v, uint64_t msr_content)
{
- unsigned int eax, ebx, ecx, edx;
+ unsigned int edx;
uint32_t msr_low;
static uint8_t lwp_intr_vector;
if ( xsave_enabled(v) && cpu_has_lwp )
{
- hvm_cpuid(0x8000001c, &eax, &ebx, &ecx, &edx);
+ hvm_cpuid(0x8000001c, NULL, NULL, NULL, &edx);
msr_low = (uint32_t)msr_content;
/* generate #GP if guest tries to turn on unsupported features. */
@@ -1163,10 +1163,10 @@ static void svm_init_erratum_383(struct
static int svm_handle_osvw(struct vcpu *v, uint32_t msr, uint64_t *val, bool_t
read)
{
- uint eax, ebx, ecx, edx;
+ unsigned int ecx;
/* Guest OSVW support */
- hvm_cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
+ hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
if ( !test_bit((X86_FEATURE_OSVW & 31), &ecx) )
return -1;
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1813,7 +1813,7 @@ int nvmx_handle_invvpid(struct cpu_user_
int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
{
struct vcpu *v = current;
- unsigned int eax, ebx, ecx, edx, dummy;
+ unsigned int eax, ebx, ecx, edx;
u64 data = 0, host_data = 0;
int r = 1;
@@ -1821,7 +1821,7 @@ int nvmx_msr_read_intercept(unsigned int
return 0;
/* VMX capablity MSRs are available only when guest supports VMX. */
- hvm_cpuid(0x1, &dummy, &dummy, &ecx, &edx);
+ hvm_cpuid(0x1, NULL, NULL, &ecx, &edx);
if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
return 0;
@@ -1972,18 +1972,18 @@ int nvmx_msr_read_intercept(unsigned int
if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
data |= X86_CR4_OSXSAVE;
- hvm_cpuid(0x0, &eax, &dummy, &dummy, &dummy);
+ hvm_cpuid(0x0, &eax, NULL, NULL, NULL);
switch ( eax )
{
default:
- hvm_cpuid(0xa, &eax, &dummy, &dummy, &dummy);
+ hvm_cpuid(0xa, &eax, NULL, NULL, NULL);
/* Check whether guest has the perf monitor feature. */
if ( (eax & 0xff) && (eax & 0xff00) )
data |= X86_CR4_PCE;
/* fall through */
case 0x7 ... 0x9:
ecx = 0;
- hvm_cpuid(0x7, &dummy, &ebx, &ecx, &dummy);
+ hvm_cpuid(0x7, NULL, &ebx, &ecx, NULL);
if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
data |= X86_CR4_FSGSBASE;
if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Andrew Cooper
2013-Sep-23 10:34 UTC
Re: [PATCH v4 4/4] x86: make hvm_cpuid() tolerate NULL pointers
On 23/09/13 11:12, Jan Beulich wrote:> Now that we other HVM code start making more extensive use of > hvm_cpuid(), let''s not force every caller to declare dummy variables > for output not cared about. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This also looks to fix Coverities (IMHO valid) objection to dereferencing ecx and storing it when passed an pointer to an uninitialised variable.> > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2757,7 +2757,17 @@ void hvm_cpuid(unsigned int input, unsig > { > struct vcpu *v = current; > struct domain *d = v->domain; > - unsigned int count = *ecx; > + unsigned int count, dummy = 0; > + > + if ( !eax ) > + eax = &dummy; > + if ( !ebx ) > + ebx = &dummy; > + if ( !ecx ) > + ecx = &dummy; > + count = *ecx; > + if ( !edx ) > + edx = &dummy; > > if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) ) > return; > @@ -2765,7 +2775,7 @@ void hvm_cpuid(unsigned int input, unsig > if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) ) > return; > > - domain_cpuid(d, input, *ecx, eax, ebx, ecx, edx); > + domain_cpuid(d, input, count, eax, ebx, ecx, edx); > > switch ( input ) > { > @@ -2853,15 +2863,15 @@ int hvm_msr_read_intercept(unsigned int > { > struct vcpu *v = current; > uint64_t *var_range_base, *fixed_range_base; > - int index, mtrr; > - uint32_t cpuid[4]; > + bool_t mtrr; > + unsigned int edx, index; > int ret = X86EMUL_OKAY; > > var_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.var_ranges; > fixed_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.fixed_ranges; > > - hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]); > - mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR)); > + hvm_cpuid(1, NULL, NULL, NULL, &edx); > + mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR)); > > switch ( msr ) > { > @@ -2969,15 +2979,15 @@ int hvm_msr_read_intercept(unsigned int > int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content) > { > struct vcpu *v = current; > - int index, mtrr; > - uint32_t cpuid[4]; > + bool_t mtrr; > + unsigned int edx, index; > int ret = X86EMUL_OKAY; > > HVMTRACE_3D(MSR_WRITE, msr, > (uint32_t)msr_content, (uint32_t)(msr_content >> 32)); > > - hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]); > - mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR)); > + hvm_cpuid(1, NULL, NULL, NULL, &edx); > + mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR)); > > hvm_memory_event_msr(msr, msr_content); > > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -806,13 +806,13 @@ static inline void svm_lwp_load(struct v > /* Update LWP_CFG MSR (0xc0000105). Return -1 if error; otherwise returns 0. */ > static int svm_update_lwp_cfg(struct vcpu *v, uint64_t msr_content) > { > - unsigned int eax, ebx, ecx, edx; > + unsigned int edx; > uint32_t msr_low; > static uint8_t lwp_intr_vector; > > if ( xsave_enabled(v) && cpu_has_lwp ) > { > - hvm_cpuid(0x8000001c, &eax, &ebx, &ecx, &edx); > + hvm_cpuid(0x8000001c, NULL, NULL, NULL, &edx); > msr_low = (uint32_t)msr_content; > > /* generate #GP if guest tries to turn on unsupported features. */ > @@ -1163,10 +1163,10 @@ static void svm_init_erratum_383(struct > > static int svm_handle_osvw(struct vcpu *v, uint32_t msr, uint64_t *val, bool_t read) > { > - uint eax, ebx, ecx, edx; > + unsigned int ecx; > > /* Guest OSVW support */ > - hvm_cpuid(0x80000001, &eax, &ebx, &ecx, &edx); > + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL); > if ( !test_bit((X86_FEATURE_OSVW & 31), &ecx) ) > return -1; > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1813,7 +1813,7 @@ int nvmx_handle_invvpid(struct cpu_user_ > int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > { > struct vcpu *v = current; > - unsigned int eax, ebx, ecx, edx, dummy; > + unsigned int eax, ebx, ecx, edx; > u64 data = 0, host_data = 0; > int r = 1; > > @@ -1821,7 +1821,7 @@ int nvmx_msr_read_intercept(unsigned int > return 0; > > /* VMX capablity MSRs are available only when guest supports VMX. */ > - hvm_cpuid(0x1, &dummy, &dummy, &ecx, &edx); > + hvm_cpuid(0x1, NULL, NULL, &ecx, &edx); > if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) ) > return 0; > > @@ -1972,18 +1972,18 @@ int nvmx_msr_read_intercept(unsigned int > if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) ) > data |= X86_CR4_OSXSAVE; > > - hvm_cpuid(0x0, &eax, &dummy, &dummy, &dummy); > + hvm_cpuid(0x0, &eax, NULL, NULL, NULL); > switch ( eax ) > { > default: > - hvm_cpuid(0xa, &eax, &dummy, &dummy, &dummy); > + hvm_cpuid(0xa, &eax, NULL, NULL, NULL); > /* Check whether guest has the perf monitor feature. */ > if ( (eax & 0xff) && (eax & 0xff00) ) > data |= X86_CR4_PCE; > /* fall through */ > case 0x7 ... 0x9: > ecx = 0; > - hvm_cpuid(0x7, &dummy, &ebx, &ecx, &dummy); > + hvm_cpuid(0x7, NULL, &ebx, &ecx, NULL); > if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) ) > data |= X86_CR4_FSGSBASE; > if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) ) > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Boris Ostrovsky
2013-Sep-23 13:42 UTC
Re: [PATCH v4 4/4] x86: make hvm_cpuid() tolerate NULL pointers
On 09/23/2013 06:12 AM, Jan Beulich wrote:> Now that we other HVM code start making more extensive use ofSomething is wrong with this sentence. I think "we" is not needed. Other than that Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> -boris> hvm_cpuid(), let''s not force every caller to declare dummy variables > for output not cared about. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2757,7 +2757,17 @@ void hvm_cpuid(unsigned int input, unsig > { > struct vcpu *v = current; > struct domain *d = v->domain; > - unsigned int count = *ecx; > + unsigned int count, dummy = 0; > + > + if ( !eax ) > + eax = &dummy; > + if ( !ebx ) > + ebx = &dummy; > + if ( !ecx ) > + ecx = &dummy; > + count = *ecx; > + if ( !edx ) > + edx = &dummy; > > if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) ) > return; > @@ -2765,7 +2775,7 @@ void hvm_cpuid(unsigned int input, unsig > if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) ) > return; > > - domain_cpuid(d, input, *ecx, eax, ebx, ecx, edx); > + domain_cpuid(d, input, count, eax, ebx, ecx, edx); > > switch ( input ) > { > @@ -2853,15 +2863,15 @@ int hvm_msr_read_intercept(unsigned int > { > struct vcpu *v = current; > uint64_t *var_range_base, *fixed_range_base; > - int index, mtrr; > - uint32_t cpuid[4]; > + bool_t mtrr; > + unsigned int edx, index; > int ret = X86EMUL_OKAY; > > var_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.var_ranges; > fixed_range_base = (uint64_t *)v->arch.hvm_vcpu.mtrr.fixed_ranges; > > - hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]); > - mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR)); > + hvm_cpuid(1, NULL, NULL, NULL, &edx); > + mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR)); > > switch ( msr ) > { > @@ -2969,15 +2979,15 @@ int hvm_msr_read_intercept(unsigned int > int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content) > { > struct vcpu *v = current; > - int index, mtrr; > - uint32_t cpuid[4]; > + bool_t mtrr; > + unsigned int edx, index; > int ret = X86EMUL_OKAY; > > HVMTRACE_3D(MSR_WRITE, msr, > (uint32_t)msr_content, (uint32_t)(msr_content >> 32)); > > - hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]); > - mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR)); > + hvm_cpuid(1, NULL, NULL, NULL, &edx); > + mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR)); > > hvm_memory_event_msr(msr, msr_content); > > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -806,13 +806,13 @@ static inline void svm_lwp_load(struct v > /* Update LWP_CFG MSR (0xc0000105). Return -1 if error; otherwise returns 0. */ > static int svm_update_lwp_cfg(struct vcpu *v, uint64_t msr_content) > { > - unsigned int eax, ebx, ecx, edx; > + unsigned int edx; > uint32_t msr_low; > static uint8_t lwp_intr_vector; > > if ( xsave_enabled(v) && cpu_has_lwp ) > { > - hvm_cpuid(0x8000001c, &eax, &ebx, &ecx, &edx); > + hvm_cpuid(0x8000001c, NULL, NULL, NULL, &edx); > msr_low = (uint32_t)msr_content; > > /* generate #GP if guest tries to turn on unsupported features. */ > @@ -1163,10 +1163,10 @@ static void svm_init_erratum_383(struct > > static int svm_handle_osvw(struct vcpu *v, uint32_t msr, uint64_t *val, bool_t read) > { > - uint eax, ebx, ecx, edx; > + unsigned int ecx; > > /* Guest OSVW support */ > - hvm_cpuid(0x80000001, &eax, &ebx, &ecx, &edx); > + hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL); > if ( !test_bit((X86_FEATURE_OSVW & 31), &ecx) ) > return -1; > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1813,7 +1813,7 @@ int nvmx_handle_invvpid(struct cpu_user_ > int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > { > struct vcpu *v = current; > - unsigned int eax, ebx, ecx, edx, dummy; > + unsigned int eax, ebx, ecx, edx; > u64 data = 0, host_data = 0; > int r = 1; > > @@ -1821,7 +1821,7 @@ int nvmx_msr_read_intercept(unsigned int > return 0; > > /* VMX capablity MSRs are available only when guest supports VMX. */ > - hvm_cpuid(0x1, &dummy, &dummy, &ecx, &edx); > + hvm_cpuid(0x1, NULL, NULL, &ecx, &edx); > if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) ) > return 0; > > @@ -1972,18 +1972,18 @@ int nvmx_msr_read_intercept(unsigned int > if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) ) > data |= X86_CR4_OSXSAVE; > > - hvm_cpuid(0x0, &eax, &dummy, &dummy, &dummy); > + hvm_cpuid(0x0, &eax, NULL, NULL, NULL); > switch ( eax ) > { > default: > - hvm_cpuid(0xa, &eax, &dummy, &dummy, &dummy); > + hvm_cpuid(0xa, &eax, NULL, NULL, NULL); > /* Check whether guest has the perf monitor feature. */ > if ( (eax & 0xff) && (eax & 0xff00) ) > data |= X86_CR4_PCE; > /* fall through */ > case 0x7 ... 0x9: > ecx = 0; > - hvm_cpuid(0x7, &dummy, &ebx, &ecx, &dummy); > + hvm_cpuid(0x7, NULL, &ebx, &ecx, NULL); > if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) ) > data |= X86_CR4_FSGSBASE; > if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) ) > >
Jan Beulich
2013-Sep-23 13:45 UTC
Re: [PATCH v4 4/4] x86: make hvm_cpuid() tolerate NULL pointers
>>> On 23.09.13 at 15:42, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: > On 09/23/2013 06:12 AM, Jan Beulich wrote: >> Now that we other HVM code start making more extensive use of > > Something is wrong with this sentence. I think "we" is not needed.Oh, yes. Dropped.> Other than that > > Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>Thanks! Jan
Boris Ostrovsky
2013-Sep-23 14:50 UTC
Re: [PATCH v4 1/4] Nested VMX: check VMX capability before read VMX related MSRs
On 09/23/2013 06:10 AM, Jan Beulich wrote:> --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1813,12 +1813,33 @@ int nvmx_handle_invvpid(struct cpu_user_ > int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > { > struct vcpu *v = current; > + unsigned int ecx, dummy; > u64 data = 0, host_data = 0; > int r = 1; > > if ( !nestedhvm_enabled(v->domain) ) > return 0; > > + /* VMX capablity MSRs are available only when guest supports VMX. */ > + hvm_cpuid(0x1, &dummy, &dummy, &ecx, &dummy);Depending on order in which you are going to commit patches you could change hvm_cpuid() dummy arguments to NULLs to make use of your "make hvm_cpuid() ..." patch. In [3/4] as well. -boris
Jan Beulich
2013-Sep-23 14:52 UTC
Re: [PATCH v4 1/4] Nested VMX: check VMX capability before read VMX related MSRs
>>> On 23.09.13 at 16:50, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: > On 09/23/2013 06:10 AM, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -1813,12 +1813,33 @@ int nvmx_handle_invvpid(struct cpu_user_ >> int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) >> { >> struct vcpu *v = current; >> + unsigned int ecx, dummy; >> u64 data = 0, host_data = 0; >> int r = 1; >> >> if ( !nestedhvm_enabled(v->domain) ) >> return 0; >> >> + /* VMX capablity MSRs are available only when guest supports VMX. */ >> + hvm_cpuid(0x1, &dummy, &dummy, &ecx, &dummy); > > Depending on order in which you are going to commit patches you could > change hvm_cpuid() dummy arguments to NULLs to make use of your "make > hvm_cpuid() ..." patch. In [3/4] as well.That last patch in the series does just that. Jan
Jan Beulich
2013-Sep-30 12:05 UTC
Ping: [PATCH v4 0/4] x86/HVM: miscellaneous improvements
>>> On 23.09.13 at 12:04, "Jan Beulich" <JBeulich@suse.com> wrote: > The first and third patches are cleaned up versions of an earlier v3 > submission by Yang. > > 1: Nested VMX: check VMX capability before read VMX related MSRs > 2: VMX: clean up capability checks > 3: Nested VMX: fix IA32_VMX_CR4_FIXED1 msr emulation > 4: x86: make hvm_cpuid() tolerate NULL pointers > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Nakajima, Jun
2013-Oct-02 15:48 UTC
Re: Ping: [PATCH v4 0/4] x86/HVM: miscellaneous improvements
On Mon, Sep 30, 2013 at 5:05 AM, Jan Beulich <JBeulich@suse.com> wrote:> >>> On 23.09.13 at 12:04, "Jan Beulich" <JBeulich@suse.com> wrote: > > The first and third patches are cleaned up versions of an earlier v3 > > submission by Yang. > > > > 1: Nested VMX: check VMX capability before read VMX related MSRs > > 2: VMX: clean up capability checks > > 3: Nested VMX: fix IA32_VMX_CR4_FIXED1 msr emulation > > 4: x86: make hvm_cpuid() tolerate NULL pointers > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > >Acked-by: Jun Nakajima <jun.nakajima@intel.com> -- Jun Intel Open Source Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Apparently Analagous Threads
- [PATCH] Nested VMX: Allow to set CR4.OSXSAVE if guest has xsave feature
- [PATCH] Nested VMX: Expose unrestricted guest feature to guest
- [PATCH v3 0/4] nested vmx: enable VMCS shadowing feature
- [PATCH 00/11] Add virtual EPT support Xen.
- [PATCH v2 0/3] nested vmx bug fixes