Here are the patches I mentioned while reviewing Liu Jinsong's MPX
series.
Patch 1 is a further cleanup of xcr0 handling, and patch 2 introduces
nested virtualization support for MPX.
Please review.
Thanks,
Paolo
Paolo Bonzini (2):
KVM: x86: introduce kvm_supported_xcr0()
KVM: x86: Add nested virtualization support for MPX
arch/x86/kvm/cpuid.c | 27 ++++++++++++++++-----------
arch/x86/kvm/vmx.c | 16 ++++++++++++++++
arch/x86/kvm/x86.c | 4 +---
arch/x86/kvm/x86.h | 2 ++
4 files changed, 35 insertions(+), 14 deletions(-)
--
1.8.3.1
XSAVE support for KVM is already using host_xcr0 & KVM_SUPPORTED_XCR0 as
a "dynamic" version of KVM_SUPPORTED_XCR0.
However, this is not enough because the MPX bits should not be presented
to the guest unless kvm_x86_ops confirms the support. So, replace all
instances of host_xcr0 & KVM_SUPPORTED_XCR0 with a new function
kvm_supported_xcr0() that also has this check.
Note that here:
if (xstate_bv & ~KVM_SUPPORTED_XCR0)
return -EINVAL;
if (xstate_bv & ~host_cr0)
return -EINVAL;
the code is equivalent to
if ((xstate_bv & ~KVM_SUPPORTED_XCR0) ||
(xstate_bv & ~host_cr0)
return -EINVAL;
i.e. "xstate_bv & (~KVM_SUPPORTED_XCR0 | ~host_cr0)" which is in turn
equal to "xstate_bv & ~(KVM_SUPPORTED_XCR0 & host_cr0)". So we should
also use the new function there.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/cpuid.c | 27 ++++++++++++++++-----------
arch/x86/kvm/x86.c | 4 +---
arch/x86/kvm/x86.h | 2 ++
3 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ddc8a7e165df..18aefb4d0927 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -43,6 +43,16 @@ static u32 xstate_required_size(u64 xstate_bv)
return ret;
}
+u64 kvm_supported_xcr0(void)
+{
+ u64 xcr0 = KVM_SUPPORTED_XCR0 & host_xcr0;
+
+ if (!kvm_x86_ops->mpx_supported || !kvm_x86_ops->mpx_supported())
+ xcr0 &= ~(XSTATE_BNDREGS | XSTATE_BNDCSR);
+
+ return xcr0;
+}
+
void kvm_update_cpuid(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
@@ -73,7 +83,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
} else {
vcpu->arch.guest_supported_xcr0 =
(best->eax | ((u64)best->edx << 32)) &
- host_xcr0 & KVM_SUPPORTED_XCR0;
+ kvm_supported_xcr0();
vcpu->arch.guest_xstate_size = best->ebx =
xstate_required_size(vcpu->arch.xcr0);
}
@@ -210,13 +220,6 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry->flags = 0;
}
-static bool supported_xcr0_bit(unsigned bit)
-{
- u64 mask = ((u64)1 << bit);
-
- return mask & KVM_SUPPORTED_XCR0 & host_xcr0;
-}
-
#define F(x) bit(X86_FEATURE_##x)
static int __do_cpuid_ent_emulated(struct kvm_cpuid_entry2 *entry,
@@ -439,16 +442,18 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
}
case 0xd: {
int idx, i;
+ u64 supported = kvm_supported_xcr0();
- entry->eax &= host_xcr0 & KVM_SUPPORTED_XCR0;
- entry->edx &= (host_xcr0 & KVM_SUPPORTED_XCR0) >> 32;
+ entry->eax &= supported;
+ entry->edx &= supported >> 32;
entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
for (idx = 1, i = 1; idx < 64; ++idx) {
+ u64 mask = ((u64)1 << idx);
if (*nent >= maxnent)
goto out;
do_cpuid_1_ent(&entry[i], function, idx);
- if (entry[i].eax == 0 || !supported_xcr0_bit(idx))
+ if (entry[i].eax == 0 || !(supported & mask))
continue;
entry[i].flags |=
KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6530019116b0..4e038cda45a2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3048,9 +3048,7 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
* CPUID leaf 0xD, index 0, EDX:EAX. This is for compatibility
* with old userspace.
*/
- if (xstate_bv & ~KVM_SUPPORTED_XCR0)
- return -EINVAL;
- if (xstate_bv & ~host_xcr0)
+ if (xstate_bv & ~kvm_supported_xcr0())
return -EINVAL;
memcpy(&vcpu->arch.guest_fpu.state->xsave,
guest_xsave->region, vcpu->arch.guest_xstate_size);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 392ecbff0030..8c97bac9a895 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -126,6 +126,8 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
| XSTATE_BNDREGS | XSTATE_BNDCSR)
extern u64 host_xcr0;
+extern u64 kvm_supported_xcr0(void);
+
extern unsigned int min_timer_period_us;
extern struct static_key kvm_no_apic_vcpu;
--
1.8.3.1
This is simple to do, the "host" BNDCFGS is either 0 or the guest value.
However, both controls have to be present. We cannot provide MPX if
we only have one of the "load BNDCFGS" or "clear BNDCFGS" controls.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/vmx.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 729b1e42aded..da28ac46ca88 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -202,6 +202,7 @@ struct __packed vmcs12 {
u64 guest_pdptr1;
u64 guest_pdptr2;
u64 guest_pdptr3;
+ u64 guest_bndcfgs;
u64 host_ia32_pat;
u64 host_ia32_efer;
u64 host_ia32_perf_global_ctrl;
@@ -534,6 +535,7 @@ static const unsigned long shadow_read_write_fields[] = {
GUEST_CS_LIMIT,
GUEST_CS_BASE,
GUEST_ES_BASE,
+ GUEST_BNDCFGS,
CR0_GUEST_HOST_MASK,
CR0_READ_SHADOW,
CR4_READ_SHADOW,
@@ -589,6 +591,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
FIELD64(GUEST_PDPTR1, guest_pdptr1),
FIELD64(GUEST_PDPTR2, guest_pdptr2),
FIELD64(GUEST_PDPTR3, guest_pdptr3),
+ FIELD64(GUEST_BNDCFGS, guest_bndcfgs),
FIELD64(HOST_IA32_PAT, host_ia32_pat),
FIELD64(HOST_IA32_EFER, host_ia32_efer),
FIELD64(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl),
@@ -719,6 +722,7 @@ static unsigned long nested_ept_get_cr3(struct kvm_vcpu *vcpu);
static u64 construct_eptp(unsigned long root_hpa);
static void kvm_cpu_vmxon(u64 addr);
static void kvm_cpu_vmxoff(void);
+static bool vmx_mpx_supported(void);
static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
static void vmx_set_segment(struct kvm_vcpu *vcpu,
struct kvm_segment *var, int seg);
@@ -2279,6 +2283,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
}
nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
+ if (vmx_mpx_supported())
+ nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
/* entry controls */
rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
@@ -2292,6 +2298,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
VM_ENTRY_LOAD_IA32_PAT;
nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
VM_ENTRY_LOAD_IA32_EFER);
+ if (vmx_mpx_supported())
+ nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
/* cpu-based controls */
rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
@@ -7847,6 +7855,9 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
set_cr4_guest_host_mask(vmx);
+ if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)
+ vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
+
if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
vmcs_write64(TSC_OFFSET,
vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
@@ -8277,6 +8288,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
+ vmcs12->guest_bndcfgs = vmcs_readl(GUEST_BNDCFGS);
/* update exit information fields: */
@@ -8386,6 +8398,10 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
vmcs_writel(GUEST_IDTR_BASE, vmcs12->host_idtr_base);
vmcs_writel(GUEST_GDTR_BASE, vmcs12->host_gdtr_base);
+ /* If not VM_EXIT_CLEAR_BNDCFGS, the L2 value propagates to L1. */
+ if (vmcs12->vm_exit_controls & VM_EXIT_CLEAR_BNDCFGS)
+ vmcs_write64(GUEST_BNDCFGS, 0);
+
if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
vcpu->arch.pat = vmcs12->host_ia32_pat;
--
1.8.3.1
On 2014-02-25 18:49, Paolo Bonzini wrote:
> This is simple to do, the "host" BNDCFGS is either 0 or the guest value.
> However, both controls have to be present. We cannot provide MPX if
> we only have one of the "load BNDCFGS" or "clear BNDCFGS" controls.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 729b1e42aded..da28ac46ca88 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -202,6 +202,7 @@ struct __packed vmcs12 {
> u64 guest_pdptr1;
> u64 guest_pdptr2;
> u64 guest_pdptr3;
> + u64 guest_bndcfgs;
> u64 host_ia32_pat;
> u64 host_ia32_efer;
> u64 host_ia32_perf_global_ctrl;
> @@ -534,6 +535,7 @@ static const unsigned long shadow_read_write_fields[] = {
> GUEST_CS_LIMIT,
> GUEST_CS_BASE,
> GUEST_ES_BASE,
> + GUEST_BNDCFGS,
> CR0_GUEST_HOST_MASK,
> CR0_READ_SHADOW,
> CR4_READ_SHADOW,
> @@ -589,6 +591,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
> FIELD64(GUEST_PDPTR1, guest_pdptr1),
> FIELD64(GUEST_PDPTR2, guest_pdptr2),
> FIELD64(GUEST_PDPTR3, guest_pdptr3),
> + FIELD64(GUEST_BNDCFGS, guest_bndcfgs),
> FIELD64(HOST_IA32_PAT, host_ia32_pat),
> FIELD64(HOST_IA32_EFER, host_ia32_efer),
> FIELD64(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl),
> @@ -719,6 +722,7 @@ static unsigned long nested_ept_get_cr3(struct kvm_vcpu *vcpu);
> static u64 construct_eptp(unsigned long root_hpa);
> static void kvm_cpu_vmxon(u64 addr);
> static void kvm_cpu_vmxoff(void);
> +static bool vmx_mpx_supported(void);
> static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
> static void vmx_set_segment(struct kvm_vcpu *vcpu,
> struct kvm_segment *var, int seg);
> @@ -2279,6 +2283,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
> }
> nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
> VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
> + if (vmx_mpx_supported())
> + nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
>
> /* entry controls */
> rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
> @@ -2292,6 +2298,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
> VM_ENTRY_LOAD_IA32_PAT;
> nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
> VM_ENTRY_LOAD_IA32_EFER);
> + if (vmx_mpx_supported())
> + nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
>
> /* cpu-based controls */
> rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
> @@ -7847,6 +7855,9 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>
> set_cr4_guest_host_mask(vmx);
>
> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)
> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
> +
> if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
> vmcs_write64(TSC_OFFSET,
> vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
> @@ -8277,6 +8288,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
> vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
> vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
> + vmcs12->guest_bndcfgs = vmcs_readl(GUEST_BNDCFGS);
Can we read this value unconditionally, even when the host does not
support the feature?
>
> /* update exit information fields: */
>
> @@ -8386,6 +8398,10 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> vmcs_writel(GUEST_IDTR_BASE, vmcs12->host_idtr_base);
> vmcs_writel(GUEST_GDTR_BASE, vmcs12->host_gdtr_base);
>
> + /* If not VM_EXIT_CLEAR_BNDCFGS, the L2 value propagates to L1. */
> + if (vmcs12->vm_exit_controls & VM_EXIT_CLEAR_BNDCFGS)
> + vmcs_write64(GUEST_BNDCFGS, 0);
> +
> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
> vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
> vcpu->arch.pat = vmcs12->host_ia32_pat;
>
Do we also have a unit test to stress this? Or are we lacking silicon
with MPX and corresponding VMX features?
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
Il 25/02/2014 19:05, Jan Kiszka ha scritto:
> On 2014-02-25 18:49, Paolo Bonzini wrote:
>> This is simple to do, the "host" BNDCFGS is either 0 or the guest value.
>> However, both controls have to be present. We cannot provide MPX if
>> we only have one of the "load BNDCFGS" or "clear BNDCFGS" controls.
>>
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> arch/x86/kvm/vmx.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 729b1e42aded..da28ac46ca88 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -202,6 +202,7 @@ struct __packed vmcs12 {
>> u64 guest_pdptr1;
>> u64 guest_pdptr2;
>> u64 guest_pdptr3;
>> + u64 guest_bndcfgs;
>> u64 host_ia32_pat;
>> u64 host_ia32_efer;
>> u64 host_ia32_perf_global_ctrl;
>> @@ -534,6 +535,7 @@ static const unsigned long shadow_read_write_fields[] = {
>> GUEST_CS_LIMIT,
>> GUEST_CS_BASE,
>> GUEST_ES_BASE,
>> + GUEST_BNDCFGS,
>> CR0_GUEST_HOST_MASK,
>> CR0_READ_SHADOW,
>> CR4_READ_SHADOW,
>> @@ -589,6 +591,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
>> FIELD64(GUEST_PDPTR1, guest_pdptr1),
>> FIELD64(GUEST_PDPTR2, guest_pdptr2),
>> FIELD64(GUEST_PDPTR3, guest_pdptr3),
>> + FIELD64(GUEST_BNDCFGS, guest_bndcfgs),
>> FIELD64(HOST_IA32_PAT, host_ia32_pat),
>> FIELD64(HOST_IA32_EFER, host_ia32_efer),
>> FIELD64(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl),
>> @@ -719,6 +722,7 @@ static unsigned long nested_ept_get_cr3(struct kvm_vcpu *vcpu);
>> static u64 construct_eptp(unsigned long root_hpa);
>> static void kvm_cpu_vmxon(u64 addr);
>> static void kvm_cpu_vmxoff(void);
>> +static bool vmx_mpx_supported(void);
>> static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
>> static void vmx_set_segment(struct kvm_vcpu *vcpu,
>> struct kvm_segment *var, int seg);
>> @@ -2279,6 +2283,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>> }
>> nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>> VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
>> + if (vmx_mpx_supported())
>> + nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
>>
>> /* entry controls */
>> rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>> @@ -2292,6 +2298,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>> VM_ENTRY_LOAD_IA32_PAT;
>> nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
>> VM_ENTRY_LOAD_IA32_EFER);
>> + if (vmx_mpx_supported())
>> + nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
>>
>> /* cpu-based controls */
>> rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>> @@ -7847,6 +7855,9 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>
>> set_cr4_guest_host_mask(vmx);
>>
>> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)
>> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
>> +
>> if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
>> vmcs_write64(TSC_OFFSET,
>> vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
>> @@ -8277,6 +8288,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>> vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
>> vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
>> vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
>> + vmcs12->guest_bndcfgs = vmcs_readl(GUEST_BNDCFGS);
>
> Can we read this value unconditionally, even when the host does not
> support the feature?
return -EWRONGPATCH;
>>
>> /* update exit information fields: */
>>
>> @@ -8386,6 +8398,10 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>> vmcs_writel(GUEST_IDTR_BASE, vmcs12->host_idtr_base);
>> vmcs_writel(GUEST_GDTR_BASE, vmcs12->host_gdtr_base);
>>
>> + /* If not VM_EXIT_CLEAR_BNDCFGS, the L2 value propagates to L1. */
>> + if (vmcs12->vm_exit_controls & VM_EXIT_CLEAR_BNDCFGS)
>> + vmcs_write64(GUEST_BNDCFGS, 0);
>> +
>> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
>> vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>> vcpu->arch.pat = vmcs12->host_ia32_pat;
>>
>
> Do we also have a unit test to stress this? Or are we lacking silicon
> with MPX and corresponding VMX features?
No silicon yet.
There is an emulator, but it is already slow enough without nested
virtualization... it would be three-level virtualization :)
Paolo
Il 25/02/2014 19:13, Paolo Bonzini ha scritto:
> Il 25/02/2014 19:05, Jan Kiszka ha scritto:
>> On 2014-02-25 18:49, Paolo Bonzini wrote:
>>> This is simple to do, the "host" BNDCFGS is either 0 or the guest value.
>>> However, both controls have to be present. We cannot provide MPX if
>>> we only have one of the "load BNDCFGS" or "clear BNDCFGS" controls.
>>>
>>> Signed-off-by: Paolo Bonzini <[email protected]>
>>> ---
>>> arch/x86/kvm/vmx.c | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 729b1e42aded..da28ac46ca88 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -202,6 +202,7 @@ struct __packed vmcs12 {
>>> u64 guest_pdptr1;
>>> u64 guest_pdptr2;
>>> u64 guest_pdptr3;
>>> + u64 guest_bndcfgs;
>>> u64 host_ia32_pat;
>>> u64 host_ia32_efer;
>>> u64 host_ia32_perf_global_ctrl;
>>> @@ -534,6 +535,7 @@ static const unsigned long
>>> shadow_read_write_fields[] = {
>>> GUEST_CS_LIMIT,
>>> GUEST_CS_BASE,
>>> GUEST_ES_BASE,
>>> + GUEST_BNDCFGS,
>>> CR0_GUEST_HOST_MASK,
>>> CR0_READ_SHADOW,
>>> CR4_READ_SHADOW,
>>> @@ -589,6 +591,7 @@ static const unsigned short
>>> vmcs_field_to_offset_table[] = {
>>> FIELD64(GUEST_PDPTR1, guest_pdptr1),
>>> FIELD64(GUEST_PDPTR2, guest_pdptr2),
>>> FIELD64(GUEST_PDPTR3, guest_pdptr3),
>>> + FIELD64(GUEST_BNDCFGS, guest_bndcfgs),
>>> FIELD64(HOST_IA32_PAT, host_ia32_pat),
>>> FIELD64(HOST_IA32_EFER, host_ia32_efer),
>>> FIELD64(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl),
>>> @@ -719,6 +722,7 @@ static unsigned long nested_ept_get_cr3(struct
>>> kvm_vcpu *vcpu);
>>> static u64 construct_eptp(unsigned long root_hpa);
>>> static void kvm_cpu_vmxon(u64 addr);
>>> static void kvm_cpu_vmxoff(void);
>>> +static bool vmx_mpx_supported(void);
>>> static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
>>> static void vmx_set_segment(struct kvm_vcpu *vcpu,
>>> struct kvm_segment *var, int seg);
>>> @@ -2279,6 +2283,8 @@ static __init void
>>> nested_vmx_setup_ctls_msrs(void)
>>> }
>>> nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>>> VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
>>> + if (vmx_mpx_supported())
>>> + nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
>>>
>>> /* entry controls */
>>> rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>>> @@ -2292,6 +2298,8 @@ static __init void
>>> nested_vmx_setup_ctls_msrs(void)
>>> VM_ENTRY_LOAD_IA32_PAT;
>>> nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
>>> VM_ENTRY_LOAD_IA32_EFER);
>>> + if (vmx_mpx_supported())
>>> + nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
>>>
>>> /* cpu-based controls */
>>> rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>>> @@ -7847,6 +7855,9 @@ static void prepare_vmcs02(struct kvm_vcpu
>>> *vcpu, struct vmcs12 *vmcs12)
>>>
>>> set_cr4_guest_host_mask(vmx);
>>>
>>> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)
>>> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
>>> +
>>> if (vmcs12->cpu_based_vm_exec_control &
>>> CPU_BASED_USE_TSC_OFFSETING)
>>> vmcs_write64(TSC_OFFSET,
>>> vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
>>> @@ -8277,6 +8288,7 @@ static void prepare_vmcs12(struct kvm_vcpu
>>> *vcpu, struct vmcs12 *vmcs12,
>>> vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
>>> vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
>>> vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
>>> + vmcs12->guest_bndcfgs = vmcs_readl(GUEST_BNDCFGS);
>>
>> Can we read this value unconditionally, even when the host does not
>> support the feature?
>
> return -EWRONGPATCH;
Makes sense to clarify since I'll only be able to send the right patch
tomorrow.
I had not noticed this problem because vmcs_readl just returns a random
value if it fails and GUEST_BNDCFGS will not be vmcs_written.
x86/vmx.flat passes even with this bug.
However, this should be a vmcs_read64.
Paolo
>>>
>>> /* update exit information fields: */
>>>
>>> @@ -8386,6 +8398,10 @@ static void load_vmcs12_host_state(struct
>>> kvm_vcpu *vcpu,
>>> vmcs_writel(GUEST_IDTR_BASE, vmcs12->host_idtr_base);
>>> vmcs_writel(GUEST_GDTR_BASE, vmcs12->host_gdtr_base);
>>>
>>> + /* If not VM_EXIT_CLEAR_BNDCFGS, the L2 value propagates to L1. */
>>> + if (vmcs12->vm_exit_controls & VM_EXIT_CLEAR_BNDCFGS)
>>> + vmcs_write64(GUEST_BNDCFGS, 0);
>>> +
>>> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
>>> vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>>> vcpu->arch.pat = vmcs12->host_ia32_pat;
>>>
>>
>> Do we also have a unit test to stress this? Or are we lacking silicon
>> with MPX and corresponding VMX features?
>
> No silicon yet.
>
> There is an emulator, but it is already slow enough without nested
> virtualization... it would be three-level virtualization :)
>
> Paolo
>
On 2014-02-25 19:13, Paolo Bonzini wrote:
> Il 25/02/2014 19:05, Jan Kiszka ha scritto:
>> On 2014-02-25 18:49, Paolo Bonzini wrote:
>>> This is simple to do, the "host" BNDCFGS is either 0 or the guest value.
>>> However, both controls have to be present. We cannot provide MPX if
>>> we only have one of the "load BNDCFGS" or "clear BNDCFGS" controls.
>>>
>>> Signed-off-by: Paolo Bonzini <[email protected]>
>>> ---
>>> arch/x86/kvm/vmx.c | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 729b1e42aded..da28ac46ca88 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -202,6 +202,7 @@ struct __packed vmcs12 {
>>> u64 guest_pdptr1;
>>> u64 guest_pdptr2;
>>> u64 guest_pdptr3;
>>> + u64 guest_bndcfgs;
>>> u64 host_ia32_pat;
>>> u64 host_ia32_efer;
>>> u64 host_ia32_perf_global_ctrl;
>>> @@ -534,6 +535,7 @@ static const unsigned long
>>> shadow_read_write_fields[] = {
>>> GUEST_CS_LIMIT,
>>> GUEST_CS_BASE,
>>> GUEST_ES_BASE,
>>> + GUEST_BNDCFGS,
>>> CR0_GUEST_HOST_MASK,
>>> CR0_READ_SHADOW,
>>> CR4_READ_SHADOW,
>>> @@ -589,6 +591,7 @@ static const unsigned short
>>> vmcs_field_to_offset_table[] = {
>>> FIELD64(GUEST_PDPTR1, guest_pdptr1),
>>> FIELD64(GUEST_PDPTR2, guest_pdptr2),
>>> FIELD64(GUEST_PDPTR3, guest_pdptr3),
>>> + FIELD64(GUEST_BNDCFGS, guest_bndcfgs),
>>> FIELD64(HOST_IA32_PAT, host_ia32_pat),
>>> FIELD64(HOST_IA32_EFER, host_ia32_efer),
>>> FIELD64(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl),
>>> @@ -719,6 +722,7 @@ static unsigned long nested_ept_get_cr3(struct
>>> kvm_vcpu *vcpu);
>>> static u64 construct_eptp(unsigned long root_hpa);
>>> static void kvm_cpu_vmxon(u64 addr);
>>> static void kvm_cpu_vmxoff(void);
>>> +static bool vmx_mpx_supported(void);
>>> static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
>>> static void vmx_set_segment(struct kvm_vcpu *vcpu,
>>> struct kvm_segment *var, int seg);
>>> @@ -2279,6 +2283,8 @@ static __init void
>>> nested_vmx_setup_ctls_msrs(void)
>>> }
>>> nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>>> VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
>>> + if (vmx_mpx_supported())
>>> + nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
>>>
>>> /* entry controls */
>>> rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>>> @@ -2292,6 +2298,8 @@ static __init void
>>> nested_vmx_setup_ctls_msrs(void)
>>> VM_ENTRY_LOAD_IA32_PAT;
>>> nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
>>> VM_ENTRY_LOAD_IA32_EFER);
>>> + if (vmx_mpx_supported())
>>> + nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
>>>
>>> /* cpu-based controls */
>>> rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>>> @@ -7847,6 +7855,9 @@ static void prepare_vmcs02(struct kvm_vcpu
>>> *vcpu, struct vmcs12 *vmcs12)
>>>
>>> set_cr4_guest_host_mask(vmx);
>>>
>>> + if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)
>>> + vmcs_write64(GUEST_BNDCFGS, vmcs12->guest_bndcfgs);
>>> +
>>> if (vmcs12->cpu_based_vm_exec_control &
>>> CPU_BASED_USE_TSC_OFFSETING)
>>> vmcs_write64(TSC_OFFSET,
>>> vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
>>> @@ -8277,6 +8288,7 @@ static void prepare_vmcs12(struct kvm_vcpu
>>> *vcpu, struct vmcs12 *vmcs12,
>>> vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
>>> vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
>>> vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
>>> + vmcs12->guest_bndcfgs = vmcs_readl(GUEST_BNDCFGS);
>>
>> Can we read this value unconditionally, even when the host does not
>> support the feature?
>
> return -EWRONGPATCH;
>
>>>
>>> /* update exit information fields: */
>>>
>>> @@ -8386,6 +8398,10 @@ static void load_vmcs12_host_state(struct
>>> kvm_vcpu *vcpu,
>>> vmcs_writel(GUEST_IDTR_BASE, vmcs12->host_idtr_base);
>>> vmcs_writel(GUEST_GDTR_BASE, vmcs12->host_gdtr_base);
>>>
>>> + /* If not VM_EXIT_CLEAR_BNDCFGS, the L2 value propagates to L1. */
>>> + if (vmcs12->vm_exit_controls & VM_EXIT_CLEAR_BNDCFGS)
>>> + vmcs_write64(GUEST_BNDCFGS, 0);
>>> +
>>> if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
>>> vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>>> vcpu->arch.pat = vmcs12->host_ia32_pat;
>>>
>>
>> Do we also have a unit test to stress this? Or are we lacking silicon
>> with MPX and corresponding VMX features?
>
> No silicon yet.
>
> There is an emulator, but it is already slow enough without nested
> virtualization... it would be three-level virtualization :)
Cool! :) Maybe it will get faster again when we nest even deeper - no
one was there so far to tell us...
But even if not, the vmx unit tests are pretty short and should not
suffer much.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux