Re: [PATCH v4 7/9] KVM: x86: When guest set CR3, handle LAM bits semantics

[Chao Gao <chao.gao@intel.com>]

On Tue, Feb 14, 2023 at 01:28:33PM +0800, Robert Hoo wrote:
>> > +
>> > 	/*
>> > 	 * Do not condition the GPA check on long mode, this helper is
>> > used to
>> > 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee
>> > that
>> > @@ -1268,8 +1272,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
>> > unsigned long cr3)
>> > 	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
>> > 		return 1;
>> > 
>> > -	if (cr3 != kvm_read_cr3(vcpu))
>> > -		kvm_mmu_new_pgd(vcpu, cr3);
>> > +	old_cr3 = kvm_read_cr3(vcpu);
>> > +	if (cr3 != old_cr3) {
>> > +		if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) {
>> 
>This means those effective addr bits changes, then no matter LAM bits
>toggled or not, it needs new pgd.
>
>> Does this check against CR3_ADDR_MASK necessarily mean LAM bits are
>> toggled, i.e., CR3_ADDR_MASK == ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57)?
>> 
>> Why not check if LAM bits are changed? This way the patch only
>> changes
>> cases related to LAM, keeping other cases intact.
>
>Yes, I can better to add check in "else" that LAM bits changes.
>But in fact above kvm_is_valid_cr3() has guaranteed no other high order
>bits changed.
>Emm, now you might ask to melt LAM bits into vcpu-
>>arch.reserved_gpa_bits? ;)

no. I am not asking for that.

My point is for example, bit X isn't in CR3_ADDR_MASK. then toggling
the bit X will go into the else{} branch, which is particularly for LAM
bits. So, the change is correct iff

	CR3_ADDR_MASK = ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57).

I didn't check if that is true on your code base. If it isn't, replace
CR3_ADDR_MASK with ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57).

>> 
>> > +			kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 |
>> > +					X86_CR3_LAM_U57));
>> 
>> Do you need to touch kvm_mmu_new_pgd() in nested_vmx_load_cr3()?
>
>Didn't scope nested LAM case in this patch set.

Is there any justificaiton for not considering nested virtualization?
Won't nested virtualization be broken by this series?