Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Boris Ostrovsky]

On 2/18/21 6:48 AM, Roger Pau Monné wrote:
>
>> +/* Get the domain's default policy. */
>> +nr_leaves = 0;
>> +rc = xc_get_system_cpu_policy(xch, di.hvm ? 
>> XEN_SYSCTL_cpu_policy_hvm_default
>> +  : 
>> XEN_SYSCTL_cpu_policy_pv_default,
>> +  &nr_leaves, NULL, &nr_msrs, msrs);
>> +if ( rc )
>> +{
>> +PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
>> +rc = -errno;
>> +goto out;
>> +}
> Why not use xc_get_domain_cpu_policy instead so that you can avoid the
> call to xc_domain_getinfo?


Yes, indeed.


-boris


>
> It would also seem safer, as you won't be discarding any adjustments
> made to the default policy by the hypervisor for this specific domain.
>
> Thanks, Roger.

Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Roger Pau Monné]

On Wed, Jan 20, 2021 at 05:49:12PM -0500, Boris Ostrovsky wrote:
> When creating a guest, if ignore_msrs option has been specified,
> apply it to guest's MSR policy.
> 
> Signed-off-by: Boris Ostrovsky 
> ---
>  tools/include/xenctrl.h   |   2 +
>  tools/libs/guest/Makefile |   1 +
>  tools/libs/guest/xg_msrs_x86.c| 110 
> ++
>  tools/libs/light/libxl_dom.c  |   5 +-
>  tools/libs/light/libxl_internal.h |   2 +
>  tools/libs/light/libxl_x86.c  |   7 +++
>  6 files changed, 125 insertions(+), 2 deletions(-)
>  create mode 100644 tools/libs/guest/xg_msrs_x86.c
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 3796425e1eca..1d6a38e73dcf 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1835,6 +1835,8 @@ int xc_cpuid_apply_policy(xc_interface *xch,
>const uint32_t *featureset,
>unsigned int nr_features, bool pae, bool itsc,
>bool nested_virt, const struct xc_xend_cpuid 
> *xend);
> +int xc_msr_apply_policy(xc_interface *xch, uint32_t domid,
> +unsigned int ignore_msr);
>  int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
>  int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
>  xc_cpumap_t cpumap, unsigned int nr_cpus);
> diff --git a/tools/libs/guest/Makefile b/tools/libs/guest/Makefile
> index 1c729040b337..452155ea0385 100644
> --- a/tools/libs/guest/Makefile
> +++ b/tools/libs/guest/Makefile
> @@ -56,6 +56,7 @@ SRCS-y += xg_dom_compat_linux.c
>  
>  SRCS-$(CONFIG_X86) += xg_dom_x86.c
>  SRCS-$(CONFIG_X86) += xg_cpuid_x86.c
> +SRCS-$(CONFIG_X86) += xg_msrs_x86.c
>  SRCS-$(CONFIG_ARM) += xg_dom_arm.c
>  
>  ifeq ($(CONFIG_LIBXC_MINIOS),y)
> diff --git a/tools/libs/guest/xg_msrs_x86.c b/tools/libs/guest/xg_msrs_x86.c
> new file mode 100644
> index ..464ce9292ad8
> --- /dev/null
> +++ b/tools/libs/guest/xg_msrs_x86.c
> @@ -0,0 +1,110 @@
> +/**
> + * xc_msrs_x86.c
> + *
> + * Update MSR policy of a domain.
> + *
> + * Copyright (c) 2021, Oracle and/or its affiliates.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see 
> .
> + */
> +
> +#include "xc_private.h"
> +#include "xen/lib/x86/msr.h"
> +
> +
> +
> +int xc_msr_apply_policy(xc_interface *xch, uint32_t domid, unsigned int 
> ignore_msr)
> +{
> +int rc;
> +unsigned int nr_leaves, nr_msrs;
> +xen_msr_entry_t *msrs = NULL;
> +struct msr_policy *p = NULL;
> +xc_dominfo_t di;
> +unsigned int err_leaf, err_subleaf, err_msr;
> +
> +if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
> + di.domid != domid )
> +{
> +ERROR("Failed to obtain d%d info", domid);
> +rc = -ESRCH;
> +goto out;
> +}
> +
> +rc = xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
> +if ( rc )
> +{
> +PERROR("Failed to obtain policy info size");
> +rc = -errno;
> +goto out;
> +}
> +
> +rc = -ENOMEM;
> +if ( (msrs = calloc(nr_msrs, sizeof(*msrs))) == NULL ||
> + (p = calloc(1, sizeof(*p))) == NULL )
> +goto out;
> +
> +/* Get the domain's default policy. */
> +nr_leaves = 0;
> +rc = xc_get_system_cpu_policy(xch, di.hvm ? 
> XEN_SYSCTL_cpu_policy_hvm_default
> +  : 
> XEN_SYSCTL_cpu_policy_pv_default,
> +  &nr_leaves, NULL, &nr_msrs, msrs);
> +if ( rc )
> +{
> +PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
> +rc = -errno;
> +goto out;
> +}

Why not use xc_get_domain_cpu_policy instead so that you can avoid the
call to xc_domain_getinfo?

It would also seem safer, as you won't be discarding any adjustments
made to the default policy by the hypervisor for this specific domain.

Thanks, Roger.

Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Boris Ostrovsky]

On 1/22/21 4:56 AM, Julien Grall wrote:
> Hi Boris,
> 
> On 20/01/2021 22:49, Boris Ostrovsky wrote:
>> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
>> index 19168572fd3e..1f2abf6679d7 100644
>> --- a/tools/libs/light/libxl_dom.c
>> +++ b/tools/libs/light/libxl_dom.c
>> @@ -383,9 +383,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>   /* Construct a CPUID policy, but only for brand new domains.  Domains
>>    * being migrated-in/restored have CPUID handled during the
>>    * static_data_done() callback. */
>> -    if (!state->restore)
>> +    if (!state->restore) {
>>   libxl__cpuid_legacy(ctx, domid, false, info);
>> -
>> +    libxl__msr_policy(ctx, domid, info);
> 
> AFAICT, this is going to break compilation of the toolst on Arm because 
> libxl__msr_policy().

Yes, it will ;-(

> 
> However, I am a bit unsure whether we should define a stub for this on Arm. 
> It feels to me it would be better to pass an extra boolean (restore) to 
> libxl__arch_domain_create() and directly implement it there.


Yes. And move libxl__cpuid_legacy call there too then.


-boris

Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Julien Grall]

Hi Boris,

On 20/01/2021 22:49, Boris Ostrovsky wrote:

diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 19168572fd3e..1f2abf6679d7 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -383,9 +383,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
  /* Construct a CPUID policy, but only for brand new domains.  Domains
   * being migrated-in/restored have CPUID handled during the
   * static_data_done() callback. */
-if (!state->restore)
+if (!state->restore) {
  libxl__cpuid_legacy(ctx, domid, false, info);
-
+libxl__msr_policy(ctx, domid, info);


AFAICT, this is going to break compilation of the toolst on Arm because 
libxl__msr_policy().


However, I am a bit unsure whether we should define a stub for this on 
Arm. It feels to me it would be better to pass an extra boolean 
(restore) to libxl__arch_domain_create() and directly implement it there.


Cheers,

--
Julien Grall

Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Wei Liu]

On Wed, Jan 20, 2021 at 05:49:12PM -0500, Boris Ostrovsky wrote:
> When creating a guest, if ignore_msrs option has been specified,
> apply it to guest's MSR policy.
> 
> Signed-off-by: Boris Ostrovsky 

Acked-by: Wei Liu 

[PATCH v2 4/4] tools/libs: Apply MSR policy to a guest

[Boris Ostrovsky]

When creating a guest, if ignore_msrs option has been specified,
apply it to guest's MSR policy.

Signed-off-by: Boris Ostrovsky 
---
 tools/include/xenctrl.h   |   2 +
 tools/libs/guest/Makefile |   1 +
 tools/libs/guest/xg_msrs_x86.c| 110 ++
 tools/libs/light/libxl_dom.c  |   5 +-
 tools/libs/light/libxl_internal.h |   2 +
 tools/libs/light/libxl_x86.c  |   7 +++
 6 files changed, 125 insertions(+), 2 deletions(-)
 create mode 100644 tools/libs/guest/xg_msrs_x86.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3796425e1eca..1d6a38e73dcf 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1835,6 +1835,8 @@ int xc_cpuid_apply_policy(xc_interface *xch,
   const uint32_t *featureset,
   unsigned int nr_features, bool pae, bool itsc,
   bool nested_virt, const struct xc_xend_cpuid *xend);
+int xc_msr_apply_policy(xc_interface *xch, uint32_t domid,
+unsigned int ignore_msr);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
 xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/Makefile b/tools/libs/guest/Makefile
index 1c729040b337..452155ea0385 100644
--- a/tools/libs/guest/Makefile
+++ b/tools/libs/guest/Makefile
@@ -56,6 +56,7 @@ SRCS-y += xg_dom_compat_linux.c
 
 SRCS-$(CONFIG_X86) += xg_dom_x86.c
 SRCS-$(CONFIG_X86) += xg_cpuid_x86.c
+SRCS-$(CONFIG_X86) += xg_msrs_x86.c
 SRCS-$(CONFIG_ARM) += xg_dom_arm.c
 
 ifeq ($(CONFIG_LIBXC_MINIOS),y)
diff --git a/tools/libs/guest/xg_msrs_x86.c b/tools/libs/guest/xg_msrs_x86.c
new file mode 100644
index ..464ce9292ad8
--- /dev/null
+++ b/tools/libs/guest/xg_msrs_x86.c
@@ -0,0 +1,110 @@
+/**
+ * xc_msrs_x86.c
+ *
+ * Update MSR policy of a domain.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see .
+ */
+
+#include "xc_private.h"
+#include "xen/lib/x86/msr.h"
+
+
+
+int xc_msr_apply_policy(xc_interface *xch, uint32_t domid, unsigned int 
ignore_msr)
+{
+int rc;
+unsigned int nr_leaves, nr_msrs;
+xen_msr_entry_t *msrs = NULL;
+struct msr_policy *p = NULL;
+xc_dominfo_t di;
+unsigned int err_leaf, err_subleaf, err_msr;
+
+if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
+ di.domid != domid )
+{
+ERROR("Failed to obtain d%d info", domid);
+rc = -ESRCH;
+goto out;
+}
+
+rc = xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
+if ( rc )
+{
+PERROR("Failed to obtain policy info size");
+rc = -errno;
+goto out;
+}
+
+rc = -ENOMEM;
+if ( (msrs = calloc(nr_msrs, sizeof(*msrs))) == NULL ||
+ (p = calloc(1, sizeof(*p))) == NULL )
+goto out;
+
+/* Get the domain's default policy. */
+nr_leaves = 0;
+rc = xc_get_system_cpu_policy(xch, di.hvm ? 
XEN_SYSCTL_cpu_policy_hvm_default
+  : 
XEN_SYSCTL_cpu_policy_pv_default,
+  &nr_leaves, NULL, &nr_msrs, msrs);
+if ( rc )
+{
+PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
+rc = -errno;
+goto out;
+}
+
+rc = x86_msr_copy_from_buffer(p, msrs, nr_msrs, &err_msr);
+if ( rc )
+{
+ERROR("Failed to deserialise MSR (err msr %#x) (%d = %s)",
+  err_msr, -rc, strerror(-rc));
+goto out;
+}
+
+p->ignore_msrs = ignore_msr;
+
+rc = x86_msr_copy_to_buffer(p, msrs, &nr_msrs);
+if ( rc )
+{
+ERROR("Failed to serialise MSR (%d = %s)", -rc, strerror(-rc));
+goto out;
+}
+
+nr_leaves = 0;
+rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, NULL, nr_msrs, msrs,
+  &err_leaf, &err_subleaf, &err_msr);
+if ( rc )
+{
+PERROR("Failed to set d%d's MSR policy (err leaf %#x, subleaf %#x, msr 
%#x)",
+   domid, err_leaf, err_subleaf, err_msr);
+rc = -errno;
+}
+
+out:
+free(msrs);
+free(p);
+
+return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: