On Fri, 2023-02-10 at 22:04 +0800, Chao Gao wrote:
> On Thu, Feb 09, 2023 at 10:40:17AM +0800, Robert Hoo wrote:
> > When calc the new CR3 value, take LAM bits in.
>
> I prefer to merge this one into patch 2 because both are related to
> CR3_LAM_U48/U57 handling. Merging them can give us the whole picture
> of
> how the new LAM bits are handled:
> * strip them from CR3 when allocating/finding a shadow root
> * stitch them with other fields to form a shadow CR3
OK
>
> I have a couple questions:
> 1. in kvm_set_cr3(),
>
> /* PDPTRs are always reloaded for PAE paging. */
> if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu))
> goto handle_tlb_flush;
>
> Shouldn't we strip off CR3_LAM_U48/U57 and do the comparison?
Here is the stringent check, i.e. including LAM bits.
Below we'll check if LAM bits is allowed to check.
if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) &&
(cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57)))
return 1;
The only LAM bits toggling case will be handled in
if (cr3 != old_cr3) {
if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) {
kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 |
X86_CR3_LAM_U57));
} else {
/*
* Though effective addr no change, mark the
* request so that LAM bits will take effect
* when enter guest.
*/
kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu);
}
}
> It depends on whether toggling CR3_LAM_U48/U57 causes a TLB flush.
In v1, Kirill and I discussed about this. He lean toward being
conservative on TLB flush.
>
> 2. also in kvm_set_cr3(),
>
> if (cr3 != kvm_read_cr3(vcpu))
> kvm_mmu_new_pgd(vcpu, cr3);
>
> is it necessary to use a new pgd if only CR3_LAM_U48/U57 were
> changed?
>
Hasn't applied my patch? It isn't like this. It's like below
if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) {
kvm_mmu
_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 |
X86_CR3_LAM_U57));
}
Only if effective pgd addr bits changes, kvm_mmu_new_pgd().
