[PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Andrew Cooper]

There are no current users which I can locate.  One piece of xend which didn't
move forwards into xl/libxl is this:

  #   Configure host CPUID consistency checks, which must be satisfied for this
  #   VM to be allowed to run on this host's processor type:
  #cpuid_check=[ '1:ecx=xxxxxxxxxxxxxxxxxxxxxxxxxx1xxxxx' ]
  # - Host must have VMX feature flag set

The implementation of xc_cpuid_check() is conceptually broken.  Dom0's view of
CPUID is not the approprite view to check, and will be wrong in the presence
of CPUID masking/faulting, and for HVM-based toolstack domains.

If it turns out that the functionality is required, it should be implemented
in terms of XEN_SYSCTL_get_cpuid_policy to use the proper CPUID view.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: David Scott <dave@recoil.org>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Jim Fehlig <jfehlig@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

RFC initially for feedback, and to see if anyone does expect to be using this
call.  It turns out that Xapi has a library function using it, but that
function is dead so can be removed.
---
 tools/libxc/include/xenctrl.h       |  4 ---
 tools/libxc/xc_cpuid_x86.c          | 57 -------------------------------------
 tools/ocaml/libs/xc/xenctrl.ml      |  2 --
 tools/ocaml/libs/xc/xenctrl.mli     |  3 --
 tools/ocaml/libs/xc/xenctrl_stubs.c | 43 ----------------------------
 tools/python/xen/lowlevel/xc/xc.c   | 34 ----------------------
 6 files changed, 143 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 96df836..acd778c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1793,10 +1793,6 @@ int xc_domain_debug_control(xc_interface *xch,
                             uint32_t vcpu);
 
 #if defined(__i386__) || defined(__x86_64__)
-int xc_cpuid_check(xc_interface *xch,
-                   const unsigned int *input,
-                   const char **config,
-                   char **config_transformed);
 int xc_cpuid_set(xc_interface *xch,
                  domid_t domid,
                  const unsigned int *input,
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 6f82277..d1d0b51 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -855,63 +855,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid,
 }
 
 /*
- * Check whether a VM is allowed to launch on this host's processor type.
- *
- * @config format is similar to that of xc_cpuid_set():
- *  '1' -> the bit must be set to 1
- *  '0' -> must be 0
- *  'x' -> we don't care
- *  's' -> (same) must be the same
- */
-int xc_cpuid_check(
-    xc_interface *xch, const unsigned int *input,
-    const char **config,
-    char **config_transformed)
-{
-    int i, j, rc;
-    unsigned int regs[4];
-
-    memset(config_transformed, 0, 4 * sizeof(*config_transformed));
-
-    cpuid(input, regs);
-
-    for ( i = 0; i < 4; i++ )
-    {
-        if ( config[i] == NULL )
-            continue;
-        config_transformed[i] = alloc_str();
-        if ( config_transformed[i] == NULL )
-        {
-            rc = -ENOMEM;
-            goto fail_rc;
-        }
-        for ( j = 0; j < 32; j++ )
-        {
-            unsigned char val = !!((regs[i] & (1U << (31 - j))));
-            if ( !strchr("10xs", config[i][j]) ||
-                 ((config[i][j] == '1') && !val) ||
-                 ((config[i][j] == '0') && val) )
-                goto fail;
-            config_transformed[i][j] = config[i][j];
-            if ( config[i][j] == 's' )
-                config_transformed[i][j] = '0' + val;
-        }
-    }
-
-    return 0;
-
- fail:
-    rc = -EPERM;
- fail_rc:
-    for ( i = 0; i < 4; i++ )
-    {
-        free(config_transformed[i]);
-        config_transformed[i] = NULL;
-    }
-    return rc;
-}
-
-/*
  * Configure a single input with the informatiom from config.
  *
  * Config is an array of strings:
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 75006e7..70a325b 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -218,8 +218,6 @@ external domain_cpuid_set: handle -> domid -> (int64 * (int64 option))
        = "stub_xc_domain_cpuid_set"
 external domain_cpuid_apply_policy: handle -> domid -> unit
        = "stub_xc_domain_cpuid_apply_policy"
-external cpuid_check: handle -> (int64 * (int64 option)) -> string option array -> (bool * string option array)
-       = "stub_xc_cpuid_check"
 
 external map_foreign_range: handle -> domid -> int
                          -> nativeint -> Xenmmap.mmap_interface
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 720e4b2..702d8a7 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -179,6 +179,3 @@ external domain_cpuid_set: handle -> domid -> (int64 * (int64 option))
        = "stub_xc_domain_cpuid_set"
 external domain_cpuid_apply_policy: handle -> domid -> unit
        = "stub_xc_domain_cpuid_apply_policy"
-external cpuid_check: handle -> (int64 * (int64 option)) -> string option array -> (bool * string option array)
-       = "stub_xc_cpuid_check"
-
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index f1b28db..c66732f 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -805,49 +805,6 @@ CAMLprim value stub_xc_domain_cpuid_apply_policy(value xch, value domid)
 	CAMLreturn(Val_unit);
 }
 
-CAMLprim value stub_xc_cpuid_check(value xch, value input, value config)
-{
-	CAMLparam3(xch, input, config);
-	CAMLlocal3(ret, array, tmp);
-#if defined(__i386__) || defined(__x86_64__)
-	int r;
-	unsigned int c_input[2];
-	char *c_config[4], *out_config[4];
-
-	c_config[0] = string_of_option_array(config, 0);
-	c_config[1] = string_of_option_array(config, 1);
-	c_config[2] = string_of_option_array(config, 2);
-	c_config[3] = string_of_option_array(config, 3);
-
-	cpuid_input_of_val(c_input[0], c_input[1], input);
-
-	array = caml_alloc(4, 0);
-	for (r = 0; r < 4; r++) {
-		tmp = Val_none;
-		if (c_config[r]) {
-			tmp = caml_alloc_small(1, 0);
-			Field(tmp, 0) = caml_alloc_string(32);
-		}
-		Store_field(array, r, tmp);
-	}
-
-	for (r = 0; r < 4; r++)
-		out_config[r] = (c_config[r]) ? String_val(Field(Field(array, r), 0)) : NULL;
-
-	r = xc_cpuid_check(_H(xch), c_input, (const char **)c_config, out_config);
-	if (r < 0)
-		failwith_xc(_H(xch));
-
-	ret = caml_alloc_tuple(2);
-	Store_field(ret, 0, Val_bool(r));
-	Store_field(ret, 1, array);
-
-#else
-	caml_failwith("xc_domain_cpuid_check: not implemented");
-#endif
-	CAMLreturn(ret);
-}
-
 CAMLprim value stub_xc_version_version(value xch)
 {
 	CAMLparam1(xch);
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 5d112af..aa9f8e4 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -711,29 +711,6 @@ static PyObject *pyxc_create_cpuid_dict(char **regs)
    return dict;
 }
 
-static PyObject *pyxc_dom_check_cpuid(XcObject *self,
-                                      PyObject *args)
-{
-    PyObject *sub_input, *config;
-    unsigned int input[2];
-    char *regs[4], *regs_transform[4];
-
-    if ( !PyArg_ParseTuple(args, "iOO", &input[0], &sub_input, &config) )
-        return NULL;
-
-    pyxc_dom_extract_cpuid(config, regs);
-
-    input[1] = XEN_CPUID_INPUT_UNUSED;
-    if ( PyLong_Check(sub_input) )
-        input[1] = PyLong_AsUnsignedLong(sub_input);
-
-    if ( xc_cpuid_check(self->xc_handle, input,
-                        (const char **)regs, regs_transform) )
-        return pyxc_error_to_exception(self->xc_handle);
-
-    return pyxc_create_cpuid_dict(regs_transform);
-}
-
 static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self,
                                            PyObject *args)
 {
@@ -2467,17 +2444,6 @@ static PyMethodDef pyxc_methods[] = {
       " keys    [str]: String of keys to inject.\n" },
 
 #if defined(__i386__) || defined(__x86_64__)
-    { "domain_check_cpuid", 
-      (PyCFunction)pyxc_dom_check_cpuid, 
-      METH_VARARGS, "\n"
-      "Apply checks to host CPUID.\n"
-      " input [long]: Input for cpuid instruction (eax)\n"
-      " sub_input [long]: Second input (optional, may be None) for cpuid "
-      "                     instruction (ecx)\n"
-      " config [dict]: Dictionary of register\n"
-      " config [dict]: Dictionary of register, use for checking\n\n"
-      "Returns: [int] 0 on success; exception on error.\n" },
-    
     { "domain_set_cpuid", 
       (PyCFunction)pyxc_dom_set_cpuid, 
       METH_VARARGS, "\n"
-- 
2.1.4


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

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Christian Lindig]

> On 17. Jul 2017, at 13:38, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> It turns out that Xapi has a library function using it, but that
> function is dead so can be removed.

I am fine with the removal of the OCaml bindings and the patch for the OCaml code. If the code is fundamentally broken it should be removed in any case but like you already said, we are not aware of any clients.

— Christian
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

On Mon, Jul 17, 2017 at 01:54:19PM +0000, Christian Lindig wrote:
> 
> > On 17. Jul 2017, at 13:38, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> > 
> > It turns out that Xapi has a library function using it, but that
> > function is dead so can be removed.
> 
> I am fine with the removal of the OCaml bindings and the patch for the OCaml code. If the code is fundamentally broken it should be removed in any case but like you already said, we are not aware of any clients.

Same for python bindings.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Wei Liu]

On Mon, Jul 17, 2017 at 01:38:03PM +0100, Andrew Cooper wrote:
> There are no current users which I can locate.  One piece of xend which didn't
> move forwards into xl/libxl is this:
> 
>   #   Configure host CPUID consistency checks, which must be satisfied for this
>   #   VM to be allowed to run on this host's processor type:
>   #cpuid_check=[ '1:ecx=xxxxxxxxxxxxxxxxxxxxxxxxxx1xxxxx' ]
>   # - Host must have VMX feature flag set
> 
> The implementation of xc_cpuid_check() is conceptually broken.  Dom0's view of
> CPUID is not the approprite view to check, and will be wrong in the presence
> of CPUID masking/faulting, and for HVM-based toolstack domains.
> 
> If it turns out that the functionality is required, it should be implemented
> in terms of XEN_SYSCTL_get_cpuid_policy to use the proper CPUID view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: David Scott <dave@recoil.org>
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> RFC initially for feedback, and to see if anyone does expect to be using this
> call.  It turns out that Xapi has a library function using it, but that
> function is dead so can be removed.

FAOD I am still waiting for Oracle and Suse folks to express their
opinions.

I will wait until August 4th. If I don't hear objection by then I will
just apply this patch.

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

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Juergen Gross]

On 19/07/17 12:32, Wei Liu wrote:
> On Mon, Jul 17, 2017 at 01:38:03PM +0100, Andrew Cooper wrote:
>> There are no current users which I can locate.  One piece of xend which didn't
>> move forwards into xl/libxl is this:
>>
>>   #   Configure host CPUID consistency checks, which must be satisfied for this
>>   #   VM to be allowed to run on this host's processor type:
>>   #cpuid_check=[ '1:ecx=xxxxxxxxxxxxxxxxxxxxxxxxxx1xxxxx' ]
>>   # - Host must have VMX feature flag set
>>
>> The implementation of xc_cpuid_check() is conceptually broken.  Dom0's view of
>> CPUID is not the approprite view to check, and will be wrong in the presence
>> of CPUID masking/faulting, and for HVM-based toolstack domains.
>>
>> If it turns out that the functionality is required, it should be implemented
>> in terms of XEN_SYSCTL_get_cpuid_policy to use the proper CPUID view.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> CC: David Scott <dave@recoil.org>
>> CC: Christian Lindig <christian.lindig@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Jim Fehlig <jfehlig@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> RFC initially for feedback, and to see if anyone does expect to be using this
>> call.  It turns out that Xapi has a library function using it, but that
>> function is dead so can be removed.
> 
> FAOD I am still waiting for Oracle and Suse folks to express their
> opinions.

No objection from me.


Juergen

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

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Boris Ostrovsky]

On 07/19/2017 06:43 AM, Juergen Gross wrote:
> On 19/07/17 12:32, Wei Liu wrote:
>> On Mon, Jul 17, 2017 at 01:38:03PM +0100, Andrew Cooper wrote:
>>> There are no current users which I can locate.  One piece of xend which didn't
>>> move forwards into xl/libxl is this:
>>>
>>>   #   Configure host CPUID consistency checks, which must be satisfied for this
>>>   #   VM to be allowed to run on this host's processor type:
>>>   #cpuid_check=[ '1:ecx=xxxxxxxxxxxxxxxxxxxxxxxxxx1xxxxx' ]
>>>   # - Host must have VMX feature flag set
>>>
>>> The implementation of xc_cpuid_check() is conceptually broken.  Dom0's view of
>>> CPUID is not the approprite view to check, and will be wrong in the presence
>>> of CPUID masking/faulting, and for HVM-based toolstack domains.
>>>
>>> If it turns out that the functionality is required, it should be implemented
>>> in terms of XEN_SYSCTL_get_cpuid_policy to use the proper CPUID view.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>> CC: David Scott <dave@recoil.org>
>>> CC: Christian Lindig <christian.lindig@citrix.com>
>>> CC: Juergen Gross <jgross@suse.com>
>>> CC: Jim Fehlig <jfehlig@suse.com>
>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> RFC initially for feedback, and to see if anyone does expect to be using this
>>> call.  It turns out that Xapi has a library function using it, but that
>>> function is dead so can be removed.
>> FAOD I am still waiting for Oracle and Suse folks to express their
>> opinions.
> No objection from me.
>


Or from me.

-boris

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

Re: [PATCH RFC] tools: Drop xc_cpuid_check() and bindings

[Wei Liu]

On Wed, Jul 19, 2017 at 08:50:07AM -0400, Boris Ostrovsky wrote:
> On 07/19/2017 06:43 AM, Juergen Gross wrote:
> > On 19/07/17 12:32, Wei Liu wrote:
> >> On Mon, Jul 17, 2017 at 01:38:03PM +0100, Andrew Cooper wrote:
> >>> There are no current users which I can locate.  One piece of xend which didn't
> >>> move forwards into xl/libxl is this:
> >>>
> >>>   #   Configure host CPUID consistency checks, which must be satisfied for this
> >>>   #   VM to be allowed to run on this host's processor type:
> >>>   #cpuid_check=[ '1:ecx=xxxxxxxxxxxxxxxxxxxxxxxxxx1xxxxx' ]
> >>>   # - Host must have VMX feature flag set
> >>>
> >>> The implementation of xc_cpuid_check() is conceptually broken.  Dom0's view of
> >>> CPUID is not the approprite view to check, and will be wrong in the presence
> >>> of CPUID masking/faulting, and for HVM-based toolstack domains.
> >>>
> >>> If it turns out that the functionality is required, it should be implemented
> >>> in terms of XEN_SYSCTL_get_cpuid_policy to use the proper CPUID view.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> ---
> >>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> >>> CC: David Scott <dave@recoil.org>
> >>> CC: Christian Lindig <christian.lindig@citrix.com>
> >>> CC: Juergen Gross <jgross@suse.com>
> >>> CC: Jim Fehlig <jfehlig@suse.com>
> >>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>
> >>> RFC initially for feedback, and to see if anyone does expect to be using this
> >>> call.  It turns out that Xapi has a library function using it, but that
> >>> function is dead so can be removed.
> >> FAOD I am still waiting for Oracle and Suse folks to express their
> >> opinions.
> > No objection from me.
> >
> 
> 
> Or from me.
> 

In that case -- queued.

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