Re: [PATCH v4 2/9] KVM: x86: MMU: Clear CR3 LAM bits when allocate shadow root

Binbin Wu
2023-02-14T02:56:59Z

On 2/9/2023 9:02 PM, Robert Hoo wrote:
> On Thu, 2023-02-09 at 17:55 +0800, Chao Gao wrote:
>> On Thu, Feb 09, 2023 at 10:40:15AM +0800, Robert Hoo wrote:
>>> --- a/arch/x86/kvm/mmu/mmu.c
>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>> @@ -3698,8 +3698,11 @@ static int mmu_alloc_shadow_roots(struct
>>> kvm_vcpu *vcpu)
>>> gfn_t root_gfn, root_pgd;
>>> int quadrant, i, r;
>>> hpa_t root;
>>> -
>> The blank line should be kept.
> OK
>>> +#ifdef CONFIG_X86_64
>>> + root_pgd = mmu->get_guest_pgd(vcpu) & ~(X86_CR3_LAM_U48 |
>>> X86_CR3_LAM_U57);
>>> +#else
>>> root_pgd = mmu->get_guest_pgd(vcpu);
>>> +#endif
>> Why are other call sites of mmu->get_guest_pgd() not changed?
> Emm, the other 3 are
> FNAME(walk_addr_generic)()
> kvm_arch_setup_async_pf()
> kvm_arch_async_page_ready
>
> In former version, I clear CR3.LAM bits for guest_pgd inside mmu-
>> get_guest_pgd(). I think this is generic. Perhaps I should still do it
> in that way. Let's wait for other's comments on this.
> Thanks for pointing out.

I also prefer to handle it inside get_guest_pdg,
but in kvm_arch_setup_async_pf()/kvm_arch_async_page_ready(), the value
is assigned to
cr3 of struct kvm_arch_async_pf, does it requires all fileds of cr3?

>
>> And what's
>> the value of the #ifdef?
> LAM is only available in 64 bit mode.

In other modes, the two bits are either ignored or not defined, seems
safe to clear the two bits unconditionally.

>>> root_gfn = root_pgd >> PAGE_SHIFT;
>>>
>>> if (mmu_check_root(vcpu, root_gfn))
>>> --
>>> 2.31.1
>>>