RE: [PATCH v5 1/2] KVM: VMX: Cleanup VMX basic information defines and usages

[Li, Xin3]

> Please send cover letters for series with more than one patch, even if there are
> only two patches.  At the very least, cover letters are a convenient location to
> provide feedback/communication for the series as a whole.

Kai also said so...  I'll take it as a standard practice.

>  Instead, I need to put it here:
> 
> I'll send a v6 with all of my suggestions incorporated.

Perfect!

>  I like the cleanups, but
> there are too many process issues to fixup when applying, a few things that I
> straight up disagree with, and more aggressive memtype related changes that can
> be done in the context of this series.
> 
> > @@ -505,8 +521,6 @@ enum vmcs_field {
> >  #define VMX_EPTP_PWL_5				0x20ull
> >  #define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
> >  #define VMX_EPTP_MT_MASK			0x7ull
> > -#define VMX_EPTP_MT_WB				0x6ull
> > -#define VMX_EPTP_MT_UC				0x0ull
> 
> I would strongly prefer to keep the VMX_EPTP_MT_WB and VMX_EPTP_MT_UC
> defines,
> at least so long as KVM is open coding reads and writes to the EPTP.  E.g if
> someone wants to do a follow-up series that adds wrappers to decode/encode
> the
> memtype (and other fiels) from/to EPTP values, then I'd be fine dropping these.
> 
> But this:
> 
> 
> 	/* Check for memory type validity */
> 	switch (new_eptp & VMX_EPTP_MT_MASK) {
> 	case MEM_TYPE_UC:
> 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_UC_BIT)))
> 			return false;
> 		break;
> 	case MEM_TYPE_WB:
> 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_WB_BIT)))
> 			return false;
> 		break;
> 	default:
> 		return false;
> 	}
> 
> looks wrong and is actively confusing, especially when the code below it does:
> 
> 	/* Page-walk levels validity. */
> 	switch (new_eptp & VMX_EPTP_PWL_MASK) {
> 	case VMX_EPTP_PWL_5:
> 		if (CC(!(vmx->nested.msrs.ept_caps &
> VMX_EPT_PAGE_WALK_5_BIT)))
> 			return false;
> 		break;
> 	case VMX_EPTP_PWL_4:
> 		if (CC(!(vmx->nested.msrs.ept_caps &
> VMX_EPT_PAGE_WALK_4_BIT)))
> 			return false;
> 		break;
> 	default:
> 		return false;
> 	}
>

I see your point here.  But "#define VMX_EPTP_MT_WB	0x6ull" seems to define
its own memory type 0x6.  I think what we want is:

/* in a pat/mtrr header */
#define MEM_TYPE_WB 0x6

/* vmx.h */
#define VMX_EPTP_MT_WB	MEM_TYPE_WB

if it's not regarded as another layer of indirect.

> >  static inline bool cpu_has_virtual_nmis(void)
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 994e014f8a50..80fea1875948 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -1226,23 +1226,29 @@ static bool is_bitwise_subset(u64 superset, u64
> subset, u64 mask)
> >  	return (superset | subset) == superset;
> >  }
> >
> > +#define VMX_BASIC_FEATURES_MASK			\
> > +	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
> > +	 VMX_BASIC_INOUT |			\
> > +	 VMX_BASIC_TRUE_CTLS)
> > +
> > +#define VMX_BASIC_RESERVED_BITS			\
> > +	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))
> 
> Looking at this with fresh eyes, I think #defines are overkill.  There is zero
> chance anything other than vmx_restore_vmx_basic() will use these, and the
> feature
> bits mask is rather weird.  It's not a mask of features that KVM supports, it's
> a mask of feature *bits* that KVM knows about.
> 
> So rather than add #defines, I think we can keep "const u64" variables, but split
> into feature_bits and reserved_bits (the latter will have open coded
> GENMASK_ULL()
> usage, whereas the former will not).
> 
> BUILD_BUG_ON() is fancy enough that it can detect overlap.

Sounds reasonable to me.

> 
> > +#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
> 
> Typo.

Sigh!

> 
> > +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50)
> 
> I don't see any value in either of these.  In fact, I find them both to be far
> more confusing, and much more likely to be incorrectly used.
> 
> Back in v1, when I said "don't bother with shift #defines", I was very specifically
> talking about feature bits where defining the bit shift is an extra, pointless
> layer.  I even (tried) to clarify that.

Another review comment got me lost here:
https://lore.kernel.org/kvm/2158ef3c5ce2de96c970b49802b7e1dba8b704d6.camel@xxxxxxxxx/