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

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Fix assertion in __virt_to_maddr when starting nested SVM guest
in debug mode. Investigation has shown that svm_vmsave/svm_vmload
make use of __pa() with invalid address.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
Changes in V2:
	- Uses paddr_t instead of uint64_t (suggested by Tim).
	- Rename the nestedsvm_vmxxxx() to svm_vmxxxx_by_paddr() (suggested by Tim).
	- Wrapped svm_vmxxxx_by_paddr() with svm_vmxxxx() (suggested by Jan).

 xen/arch/x86/hvm/svm/svm.c        |    4 ++--
 xen/include/asm-x86/hvm/svm/svm.h |   18 ++++++++++++++----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 4a7aeee..2d01987 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
         goto inject;
     }
 
-    svm_vmload(nv->nv_vvmcx);
+    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
     /* State in L1 VMCB is stale now */
     v->arch.hvm_svm.vmcb_in_sync = 0;
 
@@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
         goto inject;
     }
 
-    svm_vmsave(nv->nv_vvmcx);
+    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
 
     __update_guest_eip(regs, inst_len);
     return;
diff --git a/xen/include/asm-x86/hvm/svm/svm.h
b/xen/include/asm-x86/hvm/svm/svm.h
index 64e7e25..a70bcce 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -41,18 +41,28 @@
 #define SVM_REG_R14 (14)
 #define SVM_REG_R15 (15)
 
-static inline void svm_vmload(void *vmcb)
+static inline void svm_vmload_by_paddr(paddr_t vmcb)
 {
     asm volatile (
         ".byte 0x0f,0x01,0xda" /* vmload */
-        : : "a" (__pa(vmcb)) : "memory" );
+        : : "a" (vmcb) : "memory" );
 }
 
-static inline void svm_vmsave(void *vmcb)
+static inline void svm_vmsave_by_paddr(paddr_t vmcb)
 {
     asm volatile (
         ".byte 0x0f,0x01,0xdb" /* vmsave */
-        : : "a" (__pa(vmcb)) : "memory" );
+        : : "a" (vmcb) : "memory" );
+}
+
+static inline void svm_vmload(void *vmcb)
+{
+     svm_vmload_by_paddr(__pa(vmcb));
+}
+
+static inline void svm_vmsave(void *vmcb)
+{
+     svm_vmsave_by_paddr(__pa(vmcb));
 }
 
 static inline void svm_invlpga(unsigned long vaddr, uint32_t asid)
-- 
1.7.10.4

>>> On 06.07.13 at 00:10, <suravee.suthikulpanit@amd.com> wrote:
> @@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
>          goto inject;
>      }
>  
> -    svm_vmload(nv->nv_vvmcx);
> +    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
>      /* State in L1 VMCB is stale now */
>      v->arch.hvm_svm.vmcb_in_sync = 0;
>  
> @@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>          goto inject;
>      }
>  
> -    svm_vmsave(nv->nv_vvmcx);
> +    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
>  
>      __update_guest_eip(regs, inst_len);
>      return;
As said on the previous version already - from all I can tell these
are GPAs, not PAs, and hence can''t be passed untranslated to
VMLOAD/VMSAVE. If I''m right with this, I also can''t see how
this
would have worked for you...

Apart from that I also dislike the _by_paddr suffix. I''d suggest
either just _pa, or (slightly preferable) prefixing the names with
a double underscore instead.

Jan

On 08.07.13 11:13, Jan Beulich wrote:
>>>> On 06.07.13 at 00:10, <suravee.suthikulpanit@amd.com>
wrote:
>> @@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
>>          goto inject;
>>      }
>>  
>> -    svm_vmload(nv->nv_vvmcx);
>> +    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
>>      /* State in L1 VMCB is stale now */
>>      v->arch.hvm_svm.vmcb_in_sync = 0;
>>  
>> @@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>>          goto inject;
>>      }
>>  
>> -    svm_vmsave(nv->nv_vvmcx);
>> +    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
>>  
>>      __update_guest_eip(regs, inst_len);
>>      return;
> 
> As said on the previous version already - from all I can tell these
> are GPAs, not PAs, and hence can''t be passed untranslated to
> VMLOAD/VMSAVE. If I''m right with this, I also can''t see
how this
> would have worked for you...
You can translate GPA->PA with the hostp2m.
> Apart from that I also dislike the _by_paddr suffix. I''d suggest
> either just _pa, or (slightly preferable) prefixing the names with
> a double underscore instead.
I prefer _pa suffix over the latter.

Christoph

>>> On 08.07.13 at 11:18, "Egger, Christoph"
<chegger@amazon.de> wrote:
> On 08.07.13 11:13, Jan Beulich wrote:
>>>>> On 06.07.13 at 00:10, <suravee.suthikulpanit@amd.com>
wrote:
>>> @@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct
*vmcb,
>>>          goto inject;
>>>      }
>>>  
>>> -    svm_vmload(nv->nv_vvmcx);
>>> +    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
>>>      /* State in L1 VMCB is stale now */
>>>      v->arch.hvm_svm.vmcb_in_sync = 0;
>>>  
>>> @@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct
*vmcb,
>>>          goto inject;
>>>      }
>>>  
>>> -    svm_vmsave(nv->nv_vvmcx);
>>> +    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
>>>  
>>>      __update_guest_eip(regs, inst_len);
>>>      return;
>> 
>> As said on the previous version already - from all I can tell these
>> are GPAs, not PAs, and hence can''t be passed untranslated to
>> VMLOAD/VMSAVE. If I''m right with this, I also can''t
see how this
>> would have worked for you...
> 
> You can translate GPA->PA with the hostp2m.
I don''t think I understand what you''re trying to tell me with
this.

Jan

On 08.07.13 11:37, Jan Beulich wrote:
>>>> On 08.07.13 at 11:18, "Egger, Christoph"
<chegger@amazon.de> wrote:
>> On 08.07.13 11:13, Jan Beulich wrote:
>>>>>> On 06.07.13 at 00:10,
<suravee.suthikulpanit@amd.com> wrote:
>>>> @@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct
*vmcb,
>>>>          goto inject;
>>>>      }
>>>>  
>>>> -    svm_vmload(nv->nv_vvmcx);
>>>> +    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
>>>>      /* State in L1 VMCB is stale now */
>>>>      v->arch.hvm_svm.vmcb_in_sync = 0;
>>>>  
>>>> @@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct
*vmcb,
>>>>          goto inject;
>>>>      }
>>>>  
>>>> -    svm_vmsave(nv->nv_vvmcx);
>>>> +    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
>>>>  
>>>>      __update_guest_eip(regs, inst_len);
>>>>      return;
>>>
>>> As said on the previous version already - from all I can tell these
>>> are GPAs, not PAs, and hence can''t be passed untranslated
to
>>> VMLOAD/VMSAVE. If I''m right with this, I also
can''t see how this
>>> would have worked for you...
>>
>> You can translate GPA->PA with the hostp2m.
> 
> I don''t think I understand what you''re trying to tell me
with this.
This information is supposed for suravee. Sorry for being unclear.

Christoph

At 10:13 +0100 on 08 Jul (1373278413), Jan Beulich
wrote:
> >>> On 06.07.13 at 00:10, <suravee.suthikulpanit@amd.com>
wrote:
> > @@ -1816,7 +1816,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
> >          goto inject;
> >      }
> >  
> > -    svm_vmload(nv->nv_vvmcx);
> > +    svm_vmload_by_paddr(nv->nv_vvmcxaddr);
> >      /* State in L1 VMCB is stale now */
> >      v->arch.hvm_svm.vmcb_in_sync = 0;
> >  
> > @@ -1852,7 +1852,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
> >          goto inject;
> >      }
> >  
> > -    svm_vmsave(nv->nv_vvmcx);
> > +    svm_vmsave_by_paddr(nv->nv_vvmcxaddr);
> >  
> >      __update_guest_eip(regs, inst_len);
> >      return;
> 
> As said on the previous version already - from all I can tell these
> are GPAs, not PAs, and hence can''t be passed untranslated to
> VMLOAD/VMSAVE. If I''m right with this, I also can''t see
how this
> would have worked for you...
> 
> Apart from that I also dislike the _by_paddr suffix. I''d suggest
> either just _pa, or (slightly preferable) prefixing the names with
> a double underscore instead.
I prefer the _pa suffix; it carries a reminder of what this version does.

Tim.