[Xen-devel] [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
The current hvm_msr_{read,write}_intercept() infrastructure calls
hvm_inject_hw_exception() directly to latch a fault, and returns
X86EMUL_EXCEPTION to its caller.
This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the
fault is raised behind the back of the x86 emulator.
Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns
X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Paul Durrant
CC: Jun Nakajima
CC: Kevin Tian
CC: Boris Ostrovsky
CC: Suravee Suthikulpanit
---
xen/arch/x86/hvm/emulate.c| 14 --
xen/arch/x86/hvm/hvm.c| 8 +---
xen/arch/x86/hvm/svm/svm.c| 4 ++--
xen/arch/x86/hvm/vmx/vmx.c| 32 +---
xen/arch/x86/hvm/vmx/vvmx.c | 19 ++-
xen/include/asm-x86/hvm/support.h | 11 ---
6 files changed, 62 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index d0a043b..b182d57 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1531,7 +1531,12 @@ static int hvmemul_read_msr(
uint64_t *val,
struct x86_emulate_ctxt *ctxt)
{
-return hvm_msr_read_intercept(reg, val);
+int rc = hvm_msr_read_intercept(reg, val);
+
+if ( rc == X86EMUL_EXCEPTION )
+x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+
+return rc;
}
static int hvmemul_write_msr(
@@ -1539,7 +1544,12 @@ static int hvmemul_write_msr(
uint64_t val,
struct x86_emulate_ctxt *ctxt)
{
-return hvm_msr_write_intercept(reg, val, 1);
+int rc = hvm_msr_write_intercept(reg, val, 1);
+
+if ( rc == X86EMUL_EXCEPTION )
+x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+
+return rc;
}
static int hvmemul_wbinvd(
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ac207e4..863adfc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
if ( w->do_write.msr )
{
-hvm_msr_write_intercept(w->msr, w->value, 0);
+int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
+
+if ( rc == X86EMUL_EXCEPTION )
+hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
w->do_write.msr = 0;
}
@@ -3896,7 +3900,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t
*msr_content)
return ret;
gp_fault:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
ret = X86EMUL_EXCEPTION;
*msr_content = -1ull;
goto out;
@@ -4054,7 +4057,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
msr_content,
return ret;
gp_fault:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
return X86EMUL_EXCEPTION;
}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1588b2f..810b0d4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr,
uint64_t *msr_content)
return X86EMUL_OKAY;
gpf:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
return X86EMUL_EXCEPTION;
}
@@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr,
uint64_t msr_content)
return result;
gpf:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
return X86EMUL_EXCEPTION;
}
@@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
if ( rc == X86EMUL_OKAY )
__update_guest_eip(regs, inst_len);
+else if ( rc == X86EMUL_EXCEPTION )
+hvm_inject_hw_exception(TRAP_gp_fault, 0);
}
static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index afde634..ddfb410 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2691,7 +2691,6 @@ static int vmx_msr_read_intercept(unsigned int msr,
uint64_t *msr_content)
return X86EMUL_OKAY;
gp_fault:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
return X86EMUL_EXCEPTION;
}
@@ -2920,7 +2919,6 @@ static int vmx_msr_write_intercept(unsigned int msr,
uint64_t msr_content)
return X86EMUL_OKAY;
gp_fault:
-hvm_inject_hw_exception(TRAP_gp_fault, 0);
return X86EMUL_EXCEPTION;
}
@@ -3632,23 +3630,35 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
break;
case EXIT_REASON_MSR_READ:
{
-uint64_t msr_content;
-if ( hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY )
+uint64_t msr_content = 0;
+
+switch ( hvm_msr_read_intercept(regs->_ecx, &msr_content) )
{
-regs->eax = (uint32_t)msr_content;
-regs->edx = (uint32_t)(msr_content >> 32);
+case X86EMUL_OKAY:
+regs->rax = (uint32_t)msr_content;
+regs->rdx = (uint32_t)(msr_content >> 32);
update_guest_eip(); /* Safe: RDMSR */
Re: [Xen-devel] [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
>>> On 05.12.16 at 11:09, wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>
> if ( w->do_write.msr )
> {
> -hvm_msr_write_intercept(w->msr, w->value, 0);
> +int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
> +
> +if ( rc == X86EMUL_EXCEPTION )
> +hvm_inject_hw_exception(TRAP_gp_fault, 0);
The use of a local variable looks kind of pointless here.
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr,
> uint64_t *msr_content)
> return X86EMUL_OKAY;
>
> gpf:
> -hvm_inject_hw_exception(TRAP_gp_fault, 0);
> return X86EMUL_EXCEPTION;
> }
>
> @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr,
> uint64_t msr_content)
> return result;
>
> gpf:
> -hvm_inject_hw_exception(TRAP_gp_fault, 0);
> return X86EMUL_EXCEPTION;
> }
In cases like these it would certainly be nice to get rid of the now
rather pointless goto-s, but of course we can equally well do this
in a later patch.
> @@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs
> *regs)
>
> if ( rc == X86EMUL_OKAY )
> __update_guest_eip(regs, inst_len);
> +else if ( rc == X86EMUL_EXCEPTION )
> +hvm_inject_hw_exception(TRAP_gp_fault, 0);
else
ASSERT_UNREACHABLE();
? (And then similarly for VMX.)
> +/*
> + * May return X86EMUL_EXCEPTION, at which point the caller is responsible for
> + * injecting a #GP fault. Used to support speculative reads.
> + */
> +int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
> +int hvm_msr_write_intercept(
> +unsigned int msr, uint64_t msr_content, bool_t may_defer);
Please add __must_check to both. With at least this one taken care of
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
On 05/12/16 12:10, Jan Beulich wrote:
On 05.12.16 at 11:09, wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>>
>> if ( w->do_write.msr )
>> {
>> -hvm_msr_write_intercept(w->msr, w->value, 0);
>> +int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
>> +
>> +if ( rc == X86EMUL_EXCEPTION )
>> +hvm_inject_hw_exception(TRAP_gp_fault, 0);
> The use of a local variable looks kind of pointless here.
The first version had
if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
X86EMUL_EXCEPTION )
but this looked rather ugly to read. I prefer the version as submitted,
but am not too fussed if you insist for the latter?
>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr,
>> uint64_t *msr_content)
>> return X86EMUL_OKAY;
>>
>> gpf:
>> -hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> return X86EMUL_EXCEPTION;
>> }
>>
>> @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr,
>> uint64_t msr_content)
>> return result;
>>
>> gpf:
>> -hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> return X86EMUL_EXCEPTION;
>> }
> In cases like these it would certainly be nice to get rid of the now
> rather pointless goto-s, but of course we can equally well do this
> in a later patch.
I will do a cleanup patch and add it to v2.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
>>> On 05.12.16 at 17:29, wrote:
> On 05/12/16 12:10, Jan Beulich wrote:
> On 05.12.16 at 11:09, wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>>>
>>> if ( w->do_write.msr )
>>> {
>>> -hvm_msr_write_intercept(w->msr, w->value, 0);
>>> +int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
>>> +
>>> +if ( rc == X86EMUL_EXCEPTION )
>>> +hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> The use of a local variable looks kind of pointless here.
>
> The first version had
>
> if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
> X86EMUL_EXCEPTION )
>
> but this looked rather ugly to read. I prefer the version as submitted,
> but am not too fussed if you insist for the latter?
I won't insist, it was just a suggestion to make the code look better
to my eyes. If you like it better as is, keep it.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Monday, December 05, 2016 6:09 PM
>
> The current hvm_msr_{read,write}_intercept() infrastructure calls
> hvm_inject_hw_exception() directly to latch a fault, and returns
> X86EMUL_EXCEPTION to its caller.
>
> This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the
> fault is raised behind the back of the x86 emulator.
>
> Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns
> X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault.
>
> Signed-off-by: Andrew Cooper
Acked-by: Kevin Tian
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
