On Mon, 2023-02-13 at 10:01 +0800, Chao Gao wrote:
> On Thu, Feb 09, 2023 at 10:40:19AM +0800, Robert Hoo wrote:
> > Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's
> > valid.
>
> I prefer to squash this patch into patch 2 because it is also related
> to
> CR3 LAM bits handling.
>
Though all surround CR3, I would prefer split into pieces, so that
easier for review and accept. I can change their order to group
together. Is is all right for you?
> >
> >
> > +static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long
> > cr3)
>
> Since this function takes a "vcpu" argument, probably
> kvm_vcpu_is_valid_cr3() is slightly better.
OK, to align with kvm_vcpu_is_legal_gpa().
>
> > +{
> > + if (guest_cpuid_has(vcpu, X86_FEATURE_LAM))
>
> check if the vcpu is in the 64 bit long mode?
Emm, looks necessary.
(guest_cpuid_has(vcpu, X86_FEATURE_LAM) && is_long_mode(vcpu))
Let me ponder more, e.g. when guest has LAM feature but not in long
mode...
>
> > + cr3 &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
> > +
> > + return kvm_vcpu_is_legal_gpa(vcpu, cr3);
> > +}
> > +
> > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> > {
> > bool skip_tlb_flush = false;
> > @@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
> > unsigned long cr3)
> > * stuff CR3, e.g. for RSM emulation, and there is no guarantee
> > that
> > * the current vCPU mode is accurate.
> > */
> > - if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
> > + if (!kvm_is_valid_cr3(vcpu, cr3))
>
> There are other call sites of kvm_vcpu_is_illegal_gpa() to validate
> cr3.
> Do you need to modify them?
I don't think so. Others are for gpa validation, no need to change.
Here is for CR3.
>
> > return 1;
> >
> > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
> > --
> > 2.31.1
> >
