diff mbox series

[v3] x86/altp2m: Add a new hypercall to get the active altp2m index

Message ID 20190607105449.28167-1-aisaila@bitdefender.com (mailing list archive)
State New, archived
Headers show
Series [v3] x86/altp2m: Add a new hypercall to get the active altp2m index | expand

Commit Message

Alexandru Stefan ISAILA June 7, 2019, 10:55 a.m. UTC
The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that
uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m
index from a given vcpu.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V2:
	- Update comment and title
	- Remove redundant max_vcpu check.
---
 tools/libxc/include/xenctrl.h   |  2 ++
 tools/libxc/xc_altp2m.c         | 25 +++++++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 23 +++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h |  8 ++++++++
 4 files changed, 58 insertions(+)

Comments

Jan Beulich June 7, 2019, 11:50 a.m. UTC | #1
>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote:
> @@ -4735,6 +4736,28 @@ static int do_altp2m_op(
>                      _gfn(a.u.change_gfn.old_gfn),
>                      _gfn(a.u.change_gfn.new_gfn));
>          break;
> +
> +    case HVMOP_altp2m_get_p2m_idx:
> +    {
> +        struct vcpu *v;
> +
> +        if ( !altp2m_active(d) )
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
> +
> +        if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }

The order of return values was the other way around before, but
I suppose this doesn't matter much?

> +        a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v);
> +        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;

Just as remark for the future (I didn't pay attention before,
and the difference isn't overly meaningful for operations that
don't occur frequently) - __copy_field_to_guest() would be
less overhead here. Oh, right - this is once again not possible
because of arg (still) being a handle of void.

Jan
Alexandru Stefan ISAILA June 25, 2019, 8:01 a.m. UTC | #2
Ping,

Any thoughts on this matter are appreciated.

Thanks,
Alex

On 07.06.2019 13:55, Alexandru Stefan ISAILA wrote:
> The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that
> uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m
> index from a given vcpu.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V2:
> 	- Update comment and title
> 	- Remove redundant max_vcpu check.
> ---
>   tools/libxc/include/xenctrl.h   |  2 ++
>   tools/libxc/xc_altp2m.c         | 25 +++++++++++++++++++++++++
>   xen/arch/x86/hvm/hvm.c          | 23 +++++++++++++++++++++++
>   xen/include/public/hvm/hvm_op.h |  8 ++++++++
>   4 files changed, 58 insertions(+)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 538007a6dc..87526af4b4 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1942,6 +1942,8 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
>   int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>                            uint16_t view_id, xen_pfn_t old_gfn,
>                            xen_pfn_t new_gfn);
> +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> +                               uint32_t vcpuid, uint16_t *p2midx);
>   
>   /**
>    * Mem paging operations.
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index a86520c232..09dad0355e 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -352,3 +352,28 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
>       xc_hypercall_buffer_free(handle, arg);
>       return rc;
>   }
> +
> +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
> +                               uint32_t vcpuid, uint16_t *altp2m_idx)
> +{
> +    int rc;
> +
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_get_p2m_idx;
> +    arg->domain = domid;
> +    arg->u.get_vcpu_p2m_idx.vcpu_id = vcpuid;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                 HYPERCALL_BUFFER_AS_ARG(arg));
> +    if ( !rc )
> +        *altp2m_idx = arg->u.get_vcpu_p2m_idx.altp2m_idx;
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 029eea3b85..4ee7e6ce47 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4500,6 +4500,7 @@ static int do_altp2m_op(
>       case HVMOP_altp2m_set_mem_access_multi:
>       case HVMOP_altp2m_get_mem_access:
>       case HVMOP_altp2m_change_gfn:
> +    case HVMOP_altp2m_get_p2m_idx:
>           break;
>   
>       default:
> @@ -4735,6 +4736,28 @@ static int do_altp2m_op(
>                       _gfn(a.u.change_gfn.old_gfn),
>                       _gfn(a.u.change_gfn.new_gfn));
>           break;
> +
> +    case HVMOP_altp2m_get_p2m_idx:
> +    {
> +        struct vcpu *v;
> +
> +        if ( !altp2m_active(d) )
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
> +
> +        if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v);
> +        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +        break;
> +    }
> +
>       default:
>           ASSERT_UNREACHABLE();
>       }
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index c6cd12f596..353f8034d9 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -304,6 +304,11 @@ struct xen_hvm_altp2m_change_gfn {
>   typedef struct xen_hvm_altp2m_change_gfn xen_hvm_altp2m_change_gfn_t;
>   DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t);
>   
> +struct xen_hvm_altp2m_get_vcpu_p2m_idx {
> +    uint32_t vcpu_id;
> +    uint16_t altp2m_idx;
> +};
> +
>   struct xen_hvm_altp2m_op {
>       uint32_t version;   /* HVMOP_ALTP2M_INTERFACE_VERSION */
>       uint32_t cmd;
> @@ -332,6 +337,8 @@ struct xen_hvm_altp2m_op {
>   #define HVMOP_altp2m_get_mem_access       12
>   /* Disable altp2m event notifications for a given VCPU */
>   #define HVMOP_altp2m_vcpu_disable_notify  13
> +/* Get the active vcpu p2m index */
> +#define HVMOP_altp2m_get_p2m_idx          14
>       domid_t domain;
>       uint16_t pad1;
>       uint32_t pad2;
> @@ -347,6 +354,7 @@ struct xen_hvm_altp2m_op {
>           struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
>           struct xen_hvm_altp2m_suppress_ve          suppress_ve;
>           struct xen_hvm_altp2m_vcpu_disable_notify  disable_notify;
> +        struct xen_hvm_altp2m_get_vcpu_p2m_idx     get_vcpu_p2m_idx;
>           uint8_t pad[64];
>       } u;
>   };
>
Alexandru Stefan ISAILA July 22, 2019, 12:39 p.m. UTC | #3
Ping,

Any reviews on this patch are appreciated.

Regards,
Alex

On 07.06.2019 14:50, Jan Beulich wrote:
>>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote:
>> @@ -4735,6 +4736,28 @@ static int do_altp2m_op(
>>                       _gfn(a.u.change_gfn.old_gfn),
>>                       _gfn(a.u.change_gfn.new_gfn));
>>           break;
>> +
>> +    case HVMOP_altp2m_get_p2m_idx:
>> +    {
>> +        struct vcpu *v;
>> +
>> +        if ( !altp2m_active(d) )
>> +        {
>> +            rc = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>> +        if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
> 
> The order of return values was the other way around before, but
> I suppose this doesn't matter much?
> 
>> +        a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v);
>> +        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> 
> Just as remark for the future (I didn't pay attention before,
> and the difference isn't overly meaningful for operations that
> don't occur frequently) - __copy_field_to_guest() would be
> less overhead here. Oh, right - this is once again not possible
> because of arg (still) being a handle of void.
> 
> Jan
> 
> 
> 
> ________________________
> This email was scanned by Bitdefender
>
Jan Beulich July 22, 2019, 12:56 p.m. UTC | #4
On 22.07.2019 14:39, Alexandru Stefan ISAILA wrote:
> Ping,
> 
> Any reviews on this patch are appreciated.

FAOD I think I've provided all the feedback I have. The patch doesn't
seem to have a tool stack maintainer ack yet, and I think I had made
clear previously that I'm willing to look at changes to do_altp2m_op(),
but that I'm not willing to ack new additions to this interface as long
as it's (optionally or not) exposed to guests. It effectively being
part of altp2m, strictly speaking it shouldn't be a general x86
maintainer ack that's needed here anyway. As a hint - it certainly
would help if the function was moved suitably.

Jan

> On 07.06.2019 14:50, Jan Beulich wrote:
>>>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote:
>>> @@ -4735,6 +4736,28 @@ static int do_altp2m_op(
>>>                        _gfn(a.u.change_gfn.old_gfn),
>>>                        _gfn(a.u.change_gfn.new_gfn));
>>>            break;
>>> +
>>> +    case HVMOP_altp2m_get_p2m_idx:
>>> +    {
>>> +        struct vcpu *v;
>>> +
>>> +        if ( !altp2m_active(d) )
>>> +        {
>>> +            rc = -EOPNOTSUPP;
>>> +            break;
>>> +        }
>>> +
>>> +        if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL )
>>> +        {
>>> +            rc = -EINVAL;
>>> +            break;
>>> +        }
>>
>> The order of return values was the other way around before, but
>> I suppose this doesn't matter much?
>>
>>> +        a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v);
>>> +        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>
>> Just as remark for the future (I didn't pay attention before,
>> and the difference isn't overly meaningful for operations that
>> don't occur frequently) - __copy_field_to_guest() would be
>> less overhead here. Oh, right - this is once again not possible
>> because of arg (still) being a handle of void.
>>
>> Jan
>>
>>
>>
>> ________________________
>> This email was scanned by Bitdefender
>>
George Dunlap Sept. 4, 2019, 3:16 p.m. UTC | #5
On 6/7/19 11:55 AM, Alexandru Stefan ISAILA wrote:
> The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that
> uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m
> index from a given vcpu.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

This looks good to me.  Sorry it took so long to get to it:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
diff mbox series

Patch

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 538007a6dc..87526af4b4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1942,6 +1942,8 @@  int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
 int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn);
+int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
+                               uint32_t vcpuid, uint16_t *p2midx);
 
 /** 
  * Mem paging operations.
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index a86520c232..09dad0355e 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -352,3 +352,28 @@  int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
     xc_hypercall_buffer_free(handle, arg);
     return rc;
 }
+
+int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid,
+                               uint32_t vcpuid, uint16_t *altp2m_idx)
+{
+    int rc;
+
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_get_p2m_idx;
+    arg->domain = domid;
+    arg->u.get_vcpu_p2m_idx.vcpu_id = vcpuid;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+                 HYPERCALL_BUFFER_AS_ARG(arg));
+    if ( !rc )
+        *altp2m_idx = arg->u.get_vcpu_p2m_idx.altp2m_idx;
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 029eea3b85..4ee7e6ce47 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4500,6 +4500,7 @@  static int do_altp2m_op(
     case HVMOP_altp2m_set_mem_access_multi:
     case HVMOP_altp2m_get_mem_access:
     case HVMOP_altp2m_change_gfn:
+    case HVMOP_altp2m_get_p2m_idx:
         break;
 
     default:
@@ -4735,6 +4736,28 @@  static int do_altp2m_op(
                     _gfn(a.u.change_gfn.old_gfn),
                     _gfn(a.u.change_gfn.new_gfn));
         break;
+
+    case HVMOP_altp2m_get_p2m_idx:
+    {
+        struct vcpu *v;
+
+        if ( !altp2m_active(d) )
+        {
+            rc = -EOPNOTSUPP;
+            break;
+        }
+
+        if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL )
+        {
+            rc = -EINVAL;
+            break;
+        }
+
+        a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v);
+        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+        break;
+    }
+
     default:
         ASSERT_UNREACHABLE();
     }
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index c6cd12f596..353f8034d9 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -304,6 +304,11 @@  struct xen_hvm_altp2m_change_gfn {
 typedef struct xen_hvm_altp2m_change_gfn xen_hvm_altp2m_change_gfn_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t);
 
+struct xen_hvm_altp2m_get_vcpu_p2m_idx {
+    uint32_t vcpu_id;
+    uint16_t altp2m_idx;
+};
+
 struct xen_hvm_altp2m_op {
     uint32_t version;   /* HVMOP_ALTP2M_INTERFACE_VERSION */
     uint32_t cmd;
@@ -332,6 +337,8 @@  struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_get_mem_access       12
 /* Disable altp2m event notifications for a given VCPU */
 #define HVMOP_altp2m_vcpu_disable_notify  13
+/* Get the active vcpu p2m index */
+#define HVMOP_altp2m_get_p2m_idx          14
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -347,6 +354,7 @@  struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
         struct xen_hvm_altp2m_suppress_ve          suppress_ve;
         struct xen_hvm_altp2m_vcpu_disable_notify  disable_notify;
+        struct xen_hvm_altp2m_get_vcpu_p2m_idx     get_vcpu_p2m_idx;
         uint8_t pad[64];
     } u;
 };