Re: [PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Wed, Sep 30, 2020 at 05:29:24PM -0700, Krish Sadhukhan wrote:
> 
> On 9/28/20 8:11 PM, Sean Christopherson wrote:
> >On Mon, Sep 28, 2020 at 07:20:42AM +0000, Krish Sadhukhan wrote:
> >>According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
> >>need to be set by software as follows:
> >>
> >>	"Reserved Bits. Reserved fields should be cleared to 0 by software
> >>	when writing CR3."
> >Nothing in the shortlog or changelog actually states what this patch does.
> >"Test non-MBZ reserved bits in CR3 in long mode" is rather ambiguous, and
> >IIUC, the changelog is straight up misleading.
> >
> >Based on the discussion from v1, I _think_ this test verifies that KVM does
> >_not_ fail nested VMRUN if non-MBZ bits are set, correct?
> 
> Not KVM, hardware rather.  Hardware doesn't consider it as an invalid guest
> state if non-MBZ reserved bits are set.
> >
> >If so, then something like:
> >
> >   KVM: nSVM: Verify non-MBZ CR3 reserved bits can be set in long mode
> >
> >with further explanation in the changelog would be very helpful.
> 
> Even though the non-MBZ reserved bits are ignored by the consistency checks
> in hardware, eventually page-table walks fail. So, I am wondering whether it

Page table walks fail how?  Are you referring to forcing the #NPF, or does
the CPU puke on the non-MBZ reserved bits at some point?

> is appropriate to say,
> 
>             "Verify non-MBZ CR3 reserved bits can be set in long mode"
> 
> because the test is inducing an artificial failure even before any guest
> instruction is executed. We are not entering the guest with these bits set.

Yes we are, unless I'm misunderstanding how SVM handles VMRUN.  "entering" the
guest does not mean successfully executing guest code, it means loading guest
state and completing the world switch.  I don't think I'm misunderstanding,
because the test explicitly clears the NPT PML4[0]'s present bit to induce a
#NPF.  That means the CPU is fetching instructions, and again unless there's
details about NPT that I'm missing, the fact that the test sees a #NPF means
that the CPU successfully completed the GVA->GPA translation using the "bad"
CR3.

> I prefer to keep the commit header as is and rather expand the commit
> message to explain what I have described here. How about that ?

That's fine, so long as it documents both what the test is actually verifying
and what is/isn't legal.