| Message ID | 20170824202447.61553-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/08/2017 22:24, Jim Mattson wrote: > According to the SDM, if the "use TPR shadow" VM-execution control is > 1, bits 11:0 of the virtual-APIC address must be 0 and the address > should set any bits beyond the processor's physical-address width. > > Signed-off-by: Jim Mattson <jmattson@google.com> Applied, thanks. Paolo > --- > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 19aa69af7c2d..abe8caf9756a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) > + return 0; > + > + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) > + return -EINVAL; > + > + return 0; > +} > + > /* > * Merge L0's and L1's MSR bitmap, return false to indicate that > * we do not use the hardware. > @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > >
2017-08-25 4:24 GMT+08:00 Jim Mattson <jmattson@google.com>: > According to the SDM, if the "use TPR shadow" VM-execution control is > 1, bits 11:0 of the virtual-APIC address must be 0 and the address > should set any bits beyond the processor's physical-address width. s/should set/should not set > > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 19aa69af7c2d..abe8caf9756a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) > + return 0; > + > + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) > + return -EINVAL; > + > + return 0; > +} > + > /* > * Merge L0's and L1's MSR bitmap, return false to indicate that > * we do not use the hardware. > @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > -- > 2.14.1.342.g6490525c54-goog >
On 24.08.2017 22:24, Jim Mattson wrote: > According to the SDM, if the "use TPR shadow" VM-execution control is > 1, bits 11:0 of the virtual-APIC address must be 0 and the address > should set any bits beyond the processor's physical-address width. > > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 19aa69af7c2d..abe8caf9756a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) > + return 0; > + > + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) > + return -EINVAL; > + > + return 0; > +} > + > /* > * Merge L0's and L1's MSR bitmap, return false to indicate that > * we do not use the hardware. > @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > Late but still Reviewed-by: David Hildenbrand <david@redhat.com>
On 24.08.2017 22:24, Jim Mattson wrote: > According to the SDM, if the "use TPR shadow" VM-execution control is > 1, bits 11:0 of the virtual-APIC address must be 0 and the address > should set any bits beyond the processor's physical-address width. > > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 19aa69af7c2d..abe8caf9756a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) > + return 0; > + > + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) > + return -EINVAL; > + > + return 0; > +} > + > /* > * Merge L0's and L1's MSR bitmap, return false to indicate that > * we do not use the hardware. > @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > Should we also test for If the “use TPR shadow” VM-execution control is 1 and the “virtual-interrupt delivery” VM-execution control is 0, bits 31:4 of the TPR threshold VM-execution control field must be 0. 5 (I guess HW will implicitly check this for us?)
Yes, hardware will check that condition. However, that exposes a more fundamental defect in the kvm implementation: it is possible for an explicitly checked guest state error to take priority over an implicitly checked control field error. On Mon, Aug 28, 2017 at 8:27 AM, David Hildenbrand <david@redhat.com> wrote: > On 24.08.2017 22:24, Jim Mattson wrote: >> According to the SDM, if the "use TPR shadow" VM-execution control is >> 1, bits 11:0 of the virtual-APIC address must be 0 and the address >> should set any bits beyond the processor's physical-address width. >> >> Signed-off-by: Jim Mattson <jmattson@google.com> >> --- >> arch/x86/kvm/vmx.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 19aa69af7c2d..abe8caf9756a 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, >> return 0; >> } >> >> +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, >> + struct vmcs12 *vmcs12) >> +{ >> + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) >> + return 0; >> + >> + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> /* >> * Merge L0's and L1's MSR bitmap, return false to indicate that >> * we do not use the hardware. >> @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) >> if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) >> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> >> + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) >> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> + >> if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) >> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> >> > > Should we also test for > > If the “use TPR shadow” VM-execution control is 1 and the > “virtual-interrupt delivery” VM-execution control is > 0, bits 31:4 of the TPR threshold VM-execution control field must be 0. 5 > > (I guess HW will implicitly check this for us?) > > -- > > Thanks, > > David
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 19aa69af7c2d..abe8caf9756a 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -9915,6 +9915,18 @@ static int nested_vmx_check_msr_bitmap_controls(struct kvm_vcpu *vcpu, return 0; } +static int nested_vmx_check_tpr_shadow_controls(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + if (!nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) + return 0; + + if (!page_address_valid(vcpu, vmcs12->virtual_apic_page_addr)) + return -EINVAL; + + return 0; +} + /* * Merge L0's and L1's MSR bitmap, return false to indicate that * we do not use the hardware. @@ -10601,6 +10613,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_apicv_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
According to the SDM, if the "use TPR shadow" VM-execution control is 1, bits 11:0 of the virtual-APIC address must be 0 and the address should set any bits beyond the processor's physical-address width. Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/vmx.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)