On 2013-03-04 14:22, Gleb Natapov wrote:
> On Thu, Feb 28, 2013 at 10:44:47AM +0100, Jan Kiszka wrote:
>> The logic for calculating the value with which we call kvm_set_cr0/4 was
>> broken (will definitely be visible with nested unrestricted guest mode
>> support). Also, we performed the check regarding CR0_ALWAYSON too early
>> when in guest mode.
>>
>> What really needs to be done on both CR0 and CR4 is to mask out L1-owned
>> bits and merge them in from GUEST_CR0/4. In contrast, arch.cr0/4 and
>> arch.cr0/4_guest_owned_bits contain the mangled L0+L1 state and, thus,
>> are not suited as input.
>>
>> For both CRs, we can then apply the check against VMXON_CRx_ALWAYSON and
>> refuse the update if it fails. To be fully consistent, we implement this
>> check now also for CR4.
>>
>> Finally, we have to set the shadow to the value L2 wanted to write
>> originally.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>> ---
>>
>> Found while making unrestricted guest mode working. Not sure what impact
>> the bugs had on current feature level, if any.
>>
>> For interested folks, I've pushed my nEPT environment here:
>>
>> git://git.kiszka.org/linux-kvm.git nept-hacking
>>
>> arch/x86/kvm/vmx.c | 49 ++++++++++++++++++++++++++++++-------------------
>> 1 files changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 7cc566b..d1dac08 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4605,37 +4605,48 @@ vmx_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall)
>> /* called to set cr0 as appropriate for a mov-to-cr0 exit. */
>> static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val)
>> {
>> - if (to_vmx(vcpu)->nested.vmxon &&
>> - ((val & VMXON_CR0_ALWAYSON) != VMXON_CR0_ALWAYSON))
>> - return 1;
>> -
>> if (is_guest_mode(vcpu)) {
>> - /*
>> - * We get here when L2 changed cr0 in a way that did not change
>> - * any of L1's shadowed bits (see nested_vmx_exit_handled_cr),
>> - * but did change L0 shadowed bits. This can currently happen
>> - * with the TS bit: L0 may want to leave TS on (for lazy fpu
>> - * loading) while pretending to allow the guest to change it.
>> - */
> Can't say I understand this patch yet, but it looks like the comment is
> still valid. Why have you removed it?
L0 allows L1 or L2 at most to own TS, the rest is host-owned. I think
the comment was always misleading.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
