* [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
@ 2013-07-05 22:10 suravee.suthikulpanit
2013-07-08 9:13 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: suravee.suthikulpanit @ 2013-07-05 22:10 UTC (permalink / raw)
To: xen-devel, JBeulich, chegger; +Cc: Suravee Suthikulpanit
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
2013-07-05 22:10 [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr suravee.suthikulpanit
@ 2013-07-08 9:13 ` Jan Beulich
2013-07-08 9:18 ` Egger, Christoph
2013-07-08 16:21 ` Tim Deegan
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2013-07-08 9:13 UTC (permalink / raw)
To: suravee.suthikulpanit; +Cc: chegger, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
2013-07-08 9:13 ` Jan Beulich
@ 2013-07-08 9:18 ` Egger, Christoph
2013-07-08 9:37 ` Jan Beulich
2013-07-08 16:21 ` Tim Deegan
1 sibling, 1 reply; 6+ messages in thread
From: Egger, Christoph @ 2013-07-08 9:18 UTC (permalink / raw)
To: Jan Beulich; +Cc: suravee.suthikulpanit, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
2013-07-08 9:18 ` Egger, Christoph
@ 2013-07-08 9:37 ` Jan Beulich
2013-07-08 9:59 ` Egger, Christoph
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-07-08 9:37 UTC (permalink / raw)
To: Christoph Egger; +Cc: suravee.suthikulpanit, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
2013-07-08 9:37 ` Jan Beulich
@ 2013-07-08 9:59 ` Egger, Christoph
0 siblings, 0 replies; 6+ messages in thread
From: Egger, Christoph @ 2013-07-08 9:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: suravee.suthikulpanit, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr
2013-07-08 9:13 ` Jan Beulich
2013-07-08 9:18 ` Egger, Christoph
@ 2013-07-08 16:21 ` Tim Deegan
1 sibling, 0 replies; 6+ messages in thread
From: Tim Deegan @ 2013-07-08 16:21 UTC (permalink / raw)
To: Jan Beulich; +Cc: chegger, suravee.suthikulpanit, xen-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-08 16:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-05 22:10 [PATCH 1/1 V2] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr suravee.suthikulpanit
2013-07-08 9:13 ` Jan Beulich
2013-07-08 9:18 ` Egger, Christoph
2013-07-08 9:37 ` Jan Beulich
2013-07-08 9:59 ` Egger, Christoph
2013-07-08 16:21 ` Tim Deegan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).