Mailing List Archive

[PATCH v2 4/4] tools/libs: Apply MSR policy to a guest
When creating a guest, if ignore_msrs option has been specified,
apply it to guest's MSR policy.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
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 000000000000..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 <http://www.gnu.org/licenses/>.
+ */
+
+#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: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
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);
+ }
return rc;
}

diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c79523ba9248..4f369e6a6f14 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2054,6 +2054,8 @@ _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,

_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
libxl_domain_build_info *info);
+_hidden void libxl__msr_policy(libxl_ctx *ctx, uint32_t domid,
+ libxl_domain_build_info *info);

/* Calls poll() again - useful to check whether a signaled condition
* is still true. Cannot fail. Returns currently-true revents. */
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 86d272999d67..92ec1da77139 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -1,5 +1,6 @@
#include "libxl_internal.h"
#include "libxl_arch.h"
+#include "xen/lib/x86/msr.h"

int libxl__arch_domain_prepare_config(libxl__gc *gc,
libxl_domain_config *d_config,
@@ -838,6 +839,12 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
return rc;
}

+void libxl__msr_policy(libxl_ctx *ctx, uint32_t domid,
+ libxl_domain_build_info *info)
+{
+ if (info->ignore_msrs != LIBXL_IGNORE_MSRS_NEVER)
+ xc_msr_apply_policy(ctx->xch, domid, info->ignore_msrs);
+}

/*
* Local variables:
--
1.8.3.1
Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest [ In reply to ]
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 <boris.ostrovsky@oracle.com>

Acked-by: Wei Liu <wl@xen.org>
Re: [PATCH v2 4/4] tools/libs: Apply MSR policy to a guest [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 <boris.ostrovsky@oracle.com>
> ---
> 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 000000000000..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 <http://www.gnu.org/licenses/>.
> + */
> +
> +#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 [ In reply to ]
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.