diff mbox series

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

Message ID 20230209024022.3371768-3-robert.hu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Linear Address Masking (LAM) KVM Enabling | expand

Commit Message

Robert Hoo Feb. 9, 2023, 2:40 a.m. UTC
mmu->get_guest_pgd()'s implementation is get_cr3(), clear the LAM bits for
root_pgd, which needs a pure address, plus (possible) PCID info (low 12
bits).

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/mmu/mmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Chao Gao Feb. 9, 2023, 9:55 a.m. UTC | #1
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.

>+#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? And what's
the value of the #ifdef?

> 	root_gfn = root_pgd >> PAGE_SHIFT;
> 
> 	if (mmu_check_root(vcpu, root_gfn))
>-- 
>2.31.1
>
Robert Hoo Feb. 9, 2023, 1:02 p.m. UTC | #2
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.

> And what's
> the value of the #ifdef?

LAM is only available in 64 bit mode.
> 
> > 	root_gfn = root_pgd >> PAGE_SHIFT;
> > 
> > 	if (mmu_check_root(vcpu, root_gfn))
> > -- 
> > 2.31.1
> >
Yang, Weijiang Feb. 10, 2023, 3:38 a.m. UTC | #3
On 2/9/2023 10:40 AM, Robert Hoo wrote:

[...]


> -
> +#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

I prefer using:

root_pgd = mmu->get_guest_pgd(vcpu);

if (IS_ENABLED(CONFIG_X86_64))

     root_pgd &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);

I looks more structured.


>   	root_gfn = root_pgd >> PAGE_SHIFT;
>   
>   	if (mmu_check_root(vcpu, root_gfn))
Robert Hoo Feb. 11, 2023, 3:12 a.m. UTC | #4
On Fri, 2023-02-10 at 11:38 +0800, Yang, Weijiang wrote:
> On 2/9/2023 10:40 AM, Robert Hoo wrote:
> 
> [...]
> 
> 
> > -
> > +#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
> 
> I prefer using:
> 
> root_pgd = mmu->get_guest_pgd(vcpu);
> 
> if (IS_ENABLED(CONFIG_X86_64))
> 
>      root_pgd &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
> 
> I looks more structured.
> 
OK, thanks, both look all right to me.
Just don't quite understand the "structured" meaning, would you
elaborate more? Same as 8d5265b1016 ("KVM: x86/mmu: Use IS_ENABLED() to
avoid RETPOLINE for TDP page faults")?
Binbin Wu Feb. 14, 2023, 2:55 a.m. UTC | #5
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
>>>
Robert Hoo Feb. 15, 2023, 1:17 a.m. UTC | #6
On Tue, 2023-02-14 at 10:55 +0800, Binbin Wu wrote:
> > 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?
> 
I'm looking into the async PF. Anyone who's familiar with async PF can
shed some light?

If kvm_arch_setup_async_pf()/kvm_arch_async_page_ready() needs whole
CR3, would you agree we have 2 methods: get_cr3() and get_pgd()?
Robert Hoo Feb. 16, 2023, 2:14 a.m. UTC | #7
On Tue, 2023-02-14 at 10:55 +0800, Binbin Wu wrote:
> 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?
> 
Took a rough look at the code, looks like
kvm_arch_setup_sync_fp() preserves the temporal cr3
kvm_arch_async_page_ready() confirms the ready (resolved) PF does
correspond to current vcpu context.
To be conservative, I prefer keep
kvm_arch_setup_async_pf()/kvm_arch_async_page_ready() as is, i.e. LAM
bits is contained in the checking.

As for FNAME(walk_addr_generic)()
	pte           = mmu->get_guest_pgd(vcpu);
I think it's like mmu_alloc_shadow_roots(), i.e. LAM bits should be
cleared. 

If no objection, I'll update in next version.
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 835426254e76..1d61dfe37c77 100644
--- 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;
-
+#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
 	root_gfn = root_pgd >> PAGE_SHIFT;
 
 	if (mmu_check_root(vcpu, root_gfn))