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

From: Robert Hoo <robert.hu@linux.intel.com>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org
Subject: Re: [PATCH 8/9] KVM: x86: When guest set CR3, handle LAM bits semantics
Date: Tue, 01 Nov 2022 10:26:46 +0800	[thread overview]
Message-ID: <c2031f91e80fe24ed466db738ffb611f14bbad4f.camel@linux.intel.com> (raw)
In-Reply-To: <20221101020416.yh53bvpt3v5gwvcj@box.shutemov.name>

On Tue, 2022-11-01 at 05:04 +0300, Kirill A. Shutemov wrote:
> > > > @@ -1254,8 +1258,22 @@ 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) {
> > > > +			kvm_mmu_new_pgd(vcpu, cr3 &
> > > > ~(X86_CR3_LAM_U48 |
> > > > +					X86_CR3_LAM_U57));
> > > > +		} else {
> > > > +			/* Only LAM conf changes, no tlb flush
> > > > needed
> > > > */
> > > > +			skip_tlb_flush = true;
> > > 
> > > I'm not sure about this.
> > > 
> > > Consider case when LAM_U48 gets enabled on 5-level paging
> > > machines.
> > > We may
> > > have valid TLB entries for addresses above 47-bit. It's kinda
> > > broken
> > > case,
> > > but seems valid from architectural PoV, no?
> > 
> > You're right, thanks Kirill.
> > 
> > I noticed in your Kernel enabling, because of this LAM_U48 and
> > LA_57
> > overlapping, you enabled LAM_U57 only for simplicity at this
> > moment. I
> > thought at that time, that this trickiness will be contained in
> > Kernel
> > layer, but now it turns out at least non-EPT KVM MMU is not spared.
> > > 
> > > I guess after enabling LAM, these entries will never match. But
> > > if
> > > LAM
> > > gets disabled again they will become active. Hm?
> > > 
> > > Maybe just flush?
> > 
> > Now we have 2 options
> > 1. as you suggested, just flush
> > 2. more precisely identify the case Guest.LA57 && (CR3.bit[62:61]
> > 00
> > -->10 switching), flush. (LAM_U57 bit take precedence over LAM_U48,
> > from spec.)
> > 
> > Considering CR3 change is relatively hot path, and tlb flush is
> > heavy,
> > I lean towards option 2. Your opinion? 
> 
> 11 in bits [62:61] is also considered LAM_U57. So your option 2 is
> broken.

I don't get you here. Yes, 11 is also LAM_U57, but LAM_U57 is safe (6
bit).
Anyway, flush, as below hot path doesn't hold.
> 
> And I don't buy argument about hot path: the case we talking about is
> about enabling/disabling LAM with constant PGD. 

Right.

> It's not hot path by any
> mean.
> 
> Let's not be fancy. Just flush TLB.
> 