Re: [PATCH 4/4] SVM: clean up svm_vmcb_isvalid()

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH 4/4] SVM: clean up svm_vmcb_isvalid()
Date: Thu, 01 Jun 2017 07:06:13 -0600	[thread overview]
Message-ID: <59302D65020000780015EB99@prv-mh.provo.novell.com> (raw)
In-Reply-To: <e2032908-dc19-912b-223e-c56ae567f255@citrix.com>

>>> On 31.05.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
> On 31/05/17 08:23, Jan Beulich wrote:
>> -    if ((vmcb->_cr3 & 0x7) != 0) {
>> -        PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", vmcb->_cr3);
>> -    }
>> -    if ((vmcb->_efer & EFER_LMA) && (vmcb->_cr3 & 0xfe) != 0) {
>> -        PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", vmcb->_cr3);
>> -    }
>> +    if ( (vmcb_get_cr3(vmcb) & 0x7) ||
>> +         ((!(vmcb_get_cr4(vmcb) & X86_CR4_PAE) ||
>> +           (vmcb_get_efer(vmcb) & EFER_LMA)) &&
>> +          (vmcb_get_cr3(vmcb) & 0xfe0)) ||
>> +         ((vmcb_get_efer(vmcb) & EFER_LMA) &&
>> +          (vmcb_get_cr3(vmcb) >> v->domain->arch.cpuid->extd.maxphysaddr)) )
>> +        PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", vmcb_get_cr3(vmcb));
> 
> Is any of this correct if CR0.PG is clear?  It was my understanding that
> outside of paged mode, all bits are software available.
> 
> This is certainly the behaviour of hvm_set_cr3() (which itself has
> further knock-on bugs resulting in vmentry failures, due to insufficient
> CR3 checks when enabling CR0.PG)

I've changed it, but I'm not entirely convinced this is a good idea
for the case when someone means to use this for adhoc
debugging, as generally it is a hint of a problem if any of these
fail.

>> -    if ((vmcb->_efer >> 15U) != 0) {
>> +    if ( vmcb_get_efer(vmcb) >> 15U )
>>          PRINTF("EFER: bits [63:15] are not zero (%#"PRIx64")\n",
>> -                vmcb->_efer);
>> -    }
> 
> I don't see any justification for this particular check (even before
> your modification).  The manual states "Any MBZ bit of EFER is set", so
> I'd recommend using hvm_efer_valid() here, which also subsumes some of
> the other checks.

I can certainly add a call to hvm_efer_valid(), but that won't replace
the existing check, as that function only checks known bits and
ignores all others. Perhaps we could (should) change that, but not
in this patch. I've tightened the existing check though to check for
all undefined bits, not just those upwards from bit 15.

>> -    if (vmcb->_np_enable && vmcb->_h_cr3 == 0) {
>> +    if ( vmcb_get_np_enable(vmcb) && !vmcb_get_h_cr3(vmcb) )
>>          PRINTF("nested paging enabled but host cr3 is 0\n");
> 
> I also can't see anything in the manual about this being invalid.
> 
> The only relevant restriction I can spot is nested paging is not
> permitted if host paging is disabled.  A host cr3 value of 0 can
> legitimately be used for paging suitable PTEs are written into mfn0.

There's no h_cr0 field, so it's not clear to me how host paging
state could be determined (or how one could even talk of it
being disabled). I also couldn't find the statement you say you
have spotted. So best I can do is simply delete this check.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel