On Fri, 2023-02-10 at 23:04 +0800, Chao Gao wrote:
> Why emit a WARN()? the behavior is undefined or something KVM cannot
> emulated?
>
Oh, it should have been removed per Yuan's comment on last version;
sorry forgot to.
Originally, I wrote an WARN_ON() (perhaps WARN_ON_ONCE() is better)
here, as an assert this should never happen to KVM. But now considering
emulation part, it's possible.
> >
[...]
>
> This is a MSR write, so LAM masking isn't performed on this write
> according to LAM spec.
Right.
> Am I misunderstanding something?
>
> > +
[...]
> > @@ -1809,6 +1809,10 @@ static int __kvm_set_msr(struct kvm_vcpu
> > *vcpu, u32 index, u64 data,
> > case MSR_KERNEL_GS_BASE:
> > case MSR_CSTAR:
> > case MSR_LSTAR:
> > + /*
> > + * The strict canonical checking still applies to MSR
> > + * writing even LAM is enabled.
> > + */
> > if (is_noncanonical_address(data, vcpu))
>
> LAM spec says:
>
> Processors that support LAM continue to require the addresses
> written to
> control registers or MSRs be 57-bit canonical if the processor
> supports
> 5-level paging or 48-bit canonical if it supports only 4-level
> paging
>
> My understanding is 57-bit canonical checking is performed if the
> processor
> __supports__ 5-level paging. Then the is_noncanonical_address() here
> is
> arguably wrong. Could you double-confirm and fix it?
>
Emm, condition of "support" V.S. "enabled", you mean
vcpu_virt_addr_bits(), right?
{
return kvm_read_cr4_bits(vcpu, X86_CR4_LA57) ? 57 : 48;
}
This is worth double-confirm. Let me check with Yu. Thanks.
> > return 1;
> > break;
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index 8ec5cc983062..7228895d4a6f 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -201,6 +201,38 @@ static inline bool is_noncanonical_address(u64
> > la, struct kvm_vcpu *vcpu)
> > return !__is_canonical_address(la, vcpu_virt_addr_bits(vcpu));
> > }
> >
> > +#ifdef CONFIG_X86_64
>
> I don't get the purpose of the #ifdef. Shouldn't you check if the
> vcpu
> is in 64-bit long mode?
>
Good question, thanks. It inspires me:
1) can 32-bit host/kvm support 64-bit guest kernel?
2) if !64-bit mode (guest), should CPUID enumeration gone? or #GP on
guest attempts to set CR3/CR4 lam bits?
> > +/* untag addr for guest, according to vCPU CR3 and CR4 settings */
> > +static inline u64 kvm_untagged_addr(u64 addr, struct kvm_vcpu
> > *vcpu)
> > +{
> > + if (addr >> 63 == 0) {
> > + /* User pointers */
> > + if (kvm_read_cr3(vcpu) & X86_CR3_LAM_U57)
> > + addr = __canonical_address(addr, 57);
>
> braces are missing.
Careful review, thanks.
Curious this escaped checkpatch.pl.
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
> > + else if (kvm_read_cr3(vcpu) & X86_CR3_LAM_U48) {
> > + /*
> > + * If guest enabled 5-level paging and LAM_U48,
> > + * bit 47 should be 0, bit 48:56 contains meta
> > data
> > + * although bit 47:56 are valid 5-level address
> > + * bits.
> > + * If LAM_U48 and 4-level paging, bit47 is 0.
> > + */
> > + WARN_ON(addr & _BITUL(47));
> > + addr = __canonical_address(addr, 48);
> > + }
> > + } else if (kvm_read_cr4(vcpu) & X86_CR4_LAM_SUP) { /*
> > Supervisor pointers */
> > + if (kvm_read_cr4(vcpu) & X86_CR4_LA57)
>
> use kvm_read_cr4_bits here to save potential VMCS_READs.
Right, thanks.
