[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/1 V5] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr

On 8/8/2013 1:47 AM, Jan Beulich wrote:
On 08.08.13 at 00:18, Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx> 
On 8/7/2013 8:17 AM, Jan Beulich wrote:
On 05.08.13 at 10:31, <suravee.suthikulpanit@xxxxxxx> wrote:
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1779,15 +1779,15 @@ static void
   svm_vmexit_do_vmrun(struct cpu_user_regs *regs,
                       struct vcpu *v, uint64_t vmcbaddr)
-    if (!nestedhvm_enabled(v->domain)) {
+    if ( !nestedhvm_enabled(v->domain) || !hvm_svm_enabled(v) ) {
Suravee, why is this change needed (here and further down)?
Can we really get here when hvm_svm_enabled(v) returns false?
I don't recall this having been there in earlier versions.
Basically, I double checked the logic for all the svm_vmexit_do_vmxxx to
make sure
that the proper exception has been raised.  We had a discussion whether it
returned #GP or #UD.  In this case, if the L1 vcpu does not have SVME
bit in the EFER set, it should return #UD. Otherewise, it should return #GP.

Here the hvm_svm_enabled(v) is return true when L1 guest enabled SVM in

#define hvm_svm_enabled(v) (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))

So, I decided to add the check here as well.  Unless you think it is not
I don't know enough about the nested HVM state handling to be
certain it's not needed. The change just looks bogus in the
context of the subject of the patch, along with the need for it not
being explained in the patch description.
I'll separate the patch to make it more clear.
And of course it doesn't help things that no prior uses of hvm_svm_enabled() 
exist in the
tree (which I didn't realize before, as the name doesn't even
suggest this is a nested-only construct)...
This macro was used in the past. However, the code was removed, but the macro still exist. I'll update the macro name.
So either you fully explain in the patch description why the change
is necessary/correct _here_, or (better imo) you break it out into a
separate patch (making it obvious that then patch title and/or
description will make clear why the change is needed).
I'll update the description.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.