[Xen-devel] [PATCH] xen/hvm: Fix handling of obsolete HVM_PARAMs

[Xen-devel] [PATCH] xen/hvm: Fix handling of obsolete HVM_PARAMs

[Andrew Cooper]

The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's
behaviour for the MEMORY_EVENT params, but is wrong for the get side, where
Xen would return 0 (which is also a bug).  Delete the helper.

In Xen, perform the checks in hvm_allow_set_param(), rather than
hvm_set_param(), and actually implement checks on the get side so the
hypercall doesn't return successfully with 0 as an answer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>

Tamas: You introduced xc_hvm_param_deprecated_check() in 0a246b41ca while
introducing XEN_DOMCTL_monitor_op.  Do you recall why?
---
 tools/libxc/xc_domain.c         | 28 ++--------------------------
 xen/arch/x86/hvm/hvm.c          | 25 ++++++++++++++-----------
 xen/include/public/hvm/params.h | 18 ++++++++----------
 3 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index e544218d2e..71829c2bce 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1441,31 +1441,10 @@ int xc_domain_send_trigger(xc_interface *xch,
     return do_domctl(xch, &domctl);
 }
 
-static inline int xc_hvm_param_deprecated_check(uint32_t param)
-{
-    switch ( param )
-    {
-        case HVM_PARAM_MEMORY_EVENT_CR0:
-        case HVM_PARAM_MEMORY_EVENT_CR3:
-        case HVM_PARAM_MEMORY_EVENT_CR4:
-        case HVM_PARAM_MEMORY_EVENT_INT3:
-        case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-        case HVM_PARAM_MEMORY_EVENT_MSR:
-            return -EOPNOTSUPP;
-        default:
-            break;
-    };
-
-    return 0;
-}
-
 int xc_hvm_param_set(xc_interface *handle, uint32_t dom, uint32_t param, uint64_t value)
 {
     DECLARE_HYPERCALL_BUFFER(xen_hvm_param_t, arg);
-    int rc = xc_hvm_param_deprecated_check(param);
-
-    if ( rc )
-        return rc;
+    int rc;
 
     arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
     if ( arg == NULL )
@@ -1484,10 +1463,7 @@ int xc_hvm_param_set(xc_interface *handle, uint32_t dom, uint32_t param, uint64_
 int xc_hvm_param_get(xc_interface *handle, uint32_t dom, uint32_t param, uint64_t *value)
 {
     DECLARE_HYPERCALL_BUFFER(xen_hvm_param_t, arg);
-    int rc = xc_hvm_param_deprecated_check(param);
-
-    if ( rc )
-        return rc;
+    int rc;
 
     arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
     if ( arg == NULL )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 00a9e70b7c..93795dab92 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4105,8 +4105,14 @@ static int hvm_allow_set_param(struct domain *d,
         break;
     /* The following parameters are deprecated. */
     case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_MEMORY_EVENT_CR0:
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+    case HVM_PARAM_MEMORY_EVENT_CR4:
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-        rc = -EPERM;
+    case HVM_PARAM_MEMORY_EVENT_MSR:
+        rc = -EINVAL;
         break;
     /*
      * The following parameters must not be set by the guest
@@ -4221,15 +4227,6 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     case HVM_PARAM_ACPI_IOPORTS_LOCATION:
         rc = pmtimer_change_ioport(d, value);
         break;
-    case HVM_PARAM_MEMORY_EVENT_CR0:
-    case HVM_PARAM_MEMORY_EVENT_CR3:
-    case HVM_PARAM_MEMORY_EVENT_CR4:
-    case HVM_PARAM_MEMORY_EVENT_INT3:
-    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-    case HVM_PARAM_MEMORY_EVENT_MSR:
-        /* Deprecated */
-        rc = -EOPNOTSUPP;
-        break;
     case HVM_PARAM_NESTEDHVM:
         rc = xsm_hvm_param_nested(XSM_PRIV, d);
         if ( rc )
@@ -4411,8 +4408,14 @@ static int hvm_allow_get_param(struct domain *d,
         break;
     /* The following parameters are deprecated. */
     case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_MEMORY_EVENT_CR0:
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+    case HVM_PARAM_MEMORY_EVENT_CR4:
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-        rc = -ENODATA;
+    case HVM_PARAM_MEMORY_EVENT_MSR:
+        rc = -EINVAL;
         break;
     /* The remaining parameters should not be read by the guest. */
     default:
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 36832e4b94..68293e314e 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -28,8 +28,14 @@
 /* These parameters are deprecated and their meaning is undefined. */
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
-#define HVM_PARAM_DM_DOMAIN 13
-#define HVM_PARAM_BUFIOREQ_EVTCHN 26
+#define HVM_PARAM_DM_DOMAIN                 13
+#define HVM_PARAM_MEMORY_EVENT_CR0          20
+#define HVM_PARAM_MEMORY_EVENT_CR3          21
+#define HVM_PARAM_MEMORY_EVENT_CR4          22
+#define HVM_PARAM_MEMORY_EVENT_INT3         23
+#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
+#define HVM_PARAM_BUFIOREQ_EVTCHN           26
+#define HVM_PARAM_MEMORY_EVENT_MSR          30
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
@@ -227,14 +233,6 @@
  */
 #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
 
-/* Deprecated */
-#define HVM_PARAM_MEMORY_EVENT_CR0          20
-#define HVM_PARAM_MEMORY_EVENT_CR3          21
-#define HVM_PARAM_MEMORY_EVENT_CR4          22
-#define HVM_PARAM_MEMORY_EVENT_INT3         23
-#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
-#define HVM_PARAM_MEMORY_EVENT_MSR          30
-
 /* Boolean: Enable nestedhvm (hvm only) */
 #define HVM_PARAM_NESTEDHVM    24
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/hvm: Fix handling of obsolete HVM_PARAMs

[Jan Beulich]

On 06.02.2020 14:24, Andrew Cooper wrote:
> The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's
> behaviour for the MEMORY_EVENT params, but is wrong for the get side, where
> Xen would return 0 (which is also a bug).  Delete the helper.
> 
> In Xen, perform the checks in hvm_allow_set_param(), rather than
> hvm_set_param(), and actually implement checks on the get side so the
> hypercall doesn't return successfully with 0 as an answer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Hypervisor parts
Acked-by: Jan Beulich <jbeulich@suse.com>
with one remark:

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4105,8 +4105,14 @@ static int hvm_allow_set_param(struct domain *d,
>          break;
>      /* The following parameters are deprecated. */
>      case HVM_PARAM_DM_DOMAIN:
> +    case HVM_PARAM_MEMORY_EVENT_CR0:
> +    case HVM_PARAM_MEMORY_EVENT_CR3:
> +    case HVM_PARAM_MEMORY_EVENT_CR4:
> +    case HVM_PARAM_MEMORY_EVENT_INT3:
> +    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -        rc = -EPERM;
> +    case HVM_PARAM_MEMORY_EVENT_MSR:
> +        rc = -EINVAL;
>          break;

Other than in the header, where sorting by numeric value makes
(more) sense, I think it would be better to sort case labels
alphabetically. Otherwise when wanting to add to it, one needs
to go look up (or memorize, from doing the header file change
first) the insertion point.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/hvm: Fix handling of obsolete HVM_PARAMs

[Tamas K Lengyel]

On Thu, Feb 6, 2020 at 6:24 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's
> behaviour for the MEMORY_EVENT params, but is wrong for the get side, where
> Xen would return 0 (which is also a bug).  Delete the helper.
>
> In Xen, perform the checks in hvm_allow_set_param(), rather than
> hvm_set_param(), and actually implement checks on the get side so the
> hypercall doesn't return successfully with 0 as an answer.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
>
> Tamas: You introduced xc_hvm_param_deprecated_check() in 0a246b41ca while
> introducing XEN_DOMCTL_monitor_op.  Do you recall why?

Well, from the looks of it the intent was to tell any remaining user
of the deprecated HVM params that these are no longer supported since
we now have a separate domctl, the monitor_op, to set these options.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/hvm: Fix handling of obsolete HVM_PARAMs

[Wei Liu]

On Thu, Feb 06, 2020 at 01:24:52PM +0000, Andrew Cooper wrote:
> The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's
> behaviour for the MEMORY_EVENT params, but is wrong for the get side, where
> Xen would return 0 (which is also a bug).  Delete the helper.
> 
> In Xen, perform the checks in hvm_allow_set_param(), rather than
> hvm_set_param(), and actually implement checks on the get side so the
> hypercall doesn't return successfully with 0 as an answer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

OK. Now the check is pushed down to hypervisor. I'm certainly happy to
have less code in libxc.

Acked-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel