> 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/
