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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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.