[Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations
c/s 9383de210 "x86/altp2m: support for setting restrictions for an array of
pages" introduced this logic, but do_hvm_op() was already capable of handling
-ERESTART correctly.
More problematic however is a continuation from compat_altp2m_op(). The arg
written back into register state points into the hypercall XLAT area, not at
the original parameter passed by the guest. It may be truncated by the
vmentry, but definitely won't be correct on the next invocation.
Delete the hypercall_create_continuation() call, and return -ERESTART, which
will cause the compat case to start working correctly.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Razvan Cojocaru
CC: Petre Pircalabu
---
xen/arch/x86/hvm/hvm.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e798b49..cc89ee7 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4729,12 +4729,10 @@ static int do_altp2m_op(
if ( rc > 0 )
{
a.u.set_mem_access_multi.opaque = rc;
+rc = -ERESTART;
if ( __copy_field_to_guest(guest_handle_cast(arg,
xen_hvm_altp2m_op_t),
&a, u.set_mem_access_multi.opaque) )
rc = -EFAULT;
-else
-rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
- HVMOP_altp2m, arg);
}
break;
@@ -4853,14 +4851,8 @@ static int compat_altp2m_op(
switch ( a.cmd )
{
case HVMOP_altp2m_set_mem_access_multi:
-/*
- * The return code can be positive only if it is the return value
- * of hypercall_create_continuation. In this case, the opaque value
- * must be copied back to the guest.
- */
-if ( rc > 0 )
+if ( rc == -ERESTART )
{
-ASSERT(rc == __HYPERVISOR_hvm_op);
a.u.set_mem_access_multi.opaque =
nat.altp2m_op->u.set_mem_access_multi.opaque;
if ( __copy_field_to_guest(guest_handle_cast(arg,
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations
On 4/8/19 8:39 PM, Andrew Cooper wrote:
> c/s 9383de210 "x86/altp2m: support for setting restrictions for an array of
> pages" introduced this logic, but do_hvm_op() was already capable of handling
> -ERESTART correctly.
>
> More problematic however is a continuation from compat_altp2m_op(). The arg
> written back into register state points into the hypercall XLAT area, not at
> the original parameter passed by the guest. It may be truncated by the
> vmentry, but definitely won't be correct on the next invocation.
>
> Delete the hypercall_create_continuation() call, and return -ERESTART, which
> will cause the compat case to start working correctly.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Razvan Cojocaru
> CC: Petre Pircalabu
> ---
> xen/arch/x86/hvm/hvm.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e798b49..cc89ee7 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4729,12 +4729,10 @@ static int do_altp2m_op(
> if ( rc > 0 )
> {
> a.u.set_mem_access_multi.opaque = rc;
> +rc = -ERESTART;
> if ( __copy_field_to_guest(guest_handle_cast(arg,
> xen_hvm_altp2m_op_t),
> &a, u.set_mem_access_multi.opaque) )
> rc = -EFAULT;
> -else
> -rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
> - HVMOP_altp2m, arg);
> }
> break;
Right, that part was taken from the XENMEM_access_op_set_access_multi
code and plopped in. Didn't follow the call chain all the way through
and so missed the simpler way of doing this.
The changes certainly look correct. I'll run some tests tomorrow as well.
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations
On 08/04/2019 19:13, Razvan Cojocaru wrote:
> On 4/8/19 8:39 PM, Andrew Cooper wrote:
>> c/s 9383de210 "x86/altp2m: support for setting restrictions for an array of
>> pages" introduced this logic, but do_hvm_op() was already capable of handling
>> -ERESTART correctly.
>>
>> More problematic however is a continuation from compat_altp2m_op(). The arg
>> written back into register state points into the hypercall XLAT area, not at
>> the original parameter passed by the guest. It may be truncated by the
>> vmentry, but definitely won't be correct on the next invocation.
>>
>> Delete the hypercall_create_continuation() call, and return -ERESTART, which
>> will cause the compat case to start working correctly.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Wei Liu
>> CC: Roger Pau Monné
>> CC: Razvan Cojocaru
>> CC: Petre Pircalabu
>> ---
>> xen/arch/x86/hvm/hvm.c | 12 ++--
>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index e798b49..cc89ee7 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4729,12 +4729,10 @@ static int do_altp2m_op(
>> if ( rc > 0 )
>> {
>> a.u.set_mem_access_multi.opaque = rc;
>> +rc = -ERESTART;
>> if ( __copy_field_to_guest(guest_handle_cast(arg,
>> xen_hvm_altp2m_op_t),
>> &a, u.set_mem_access_multi.opaque) )
>> rc = -EFAULT;
>> -else
>> -rc = hypercall_create_continuation(__HYPERVISOR_hvm_op,
>> "lh",
>> - HVMOP_altp2m, arg);
>> }
>> break;
> Right, that part was taken from the XENMEM_access_op_set_access_multi
> code and plopped in. Didn't follow the call chain all the way through
> and so missed the simpler way of doing this.
do_memory_op() is the other way around. The compat wrapper is entered
first, and calls into the native path, and any set-up continuations are
then adjusted by hypercall_xlat_continuation() on the way back out.
All of this parameter handling is chronically complicated, and in
serious need of a more simple way to be found.
> The changes certainly look correct. I'll run some tests tomorrow as well.
Thanks,
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations
>>> On 08.04.19 at 19:39, wrote: > c/s 9383de210 "x86/altp2m: support for setting restrictions for an array of > pages" introduced this logic, but do_hvm_op() was already capable of handling > -ERESTART correctly. > > More problematic however is a continuation from compat_altp2m_op(). The arg > written back into register state points into the hypercall XLAT area, not at > the original parameter passed by the guest. It may be truncated by the > vmentry, but definitely won't be correct on the next invocation. > > Delete the hypercall_create_continuation() call, and return -ERESTART, which > will cause the compat case to start working correctly. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Fix altp2m_op hypercall continuations
On 4/8/19 9:34 PM, Andrew Cooper wrote: On 08/04/2019 19:13, Razvan Cojocaru wrote: The changes certainly look correct. I'll run some tests tomorrow as well. Thanks, Continuation appears to work correctly with our tests (64-bit PV domain calling 64-bit Xen / dom0). Since we're only using the external altp2m model, HVMOPs from a 32-bit guest never occur, so the issues this patch fixes are not a problem for our introspection agent, at least. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
