Re: [Xen-devel] [PATCH v1 1/5] x86/msr: introduce struct msr_domain_policy

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Sergey Dyasli [mailto:sergey.dyasli@xxxxxxxxxx]
> Sent: Wednesday, August 30, 2017 6:34 PM
> 
> The new structure contains information about guest's MSRs that are
> shared between all domain's vCPUs. It starts with only 1 MSR:
> 
>     MSR_INTEL_PLATFORM_INFO
> 
> Which currently has only 1 usable bit: cpuid_faulting.
> 
> Add 2 global policy objects: hvm_max and pv_max that are inited during
> boot up. It's always possible to emulate CPUID faulting for HVM guests
> while for PV guests the H/W support is required.
> 
> Add init_domain_msr_policy() which sets initial MSR policy during
> domain creation with a special case for Dom0.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
>  xen/arch/x86/Makefile        |  1 +
>  xen/arch/x86/domain.c        |  6 +++
>  xen/arch/x86/msr.c           | 95
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/setup.c         |  1 +
>  xen/include/asm-x86/domain.h |  3 +-
>  xen/include/asm-x86/msr.h    | 13 ++++++
>  6 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/x86/msr.c
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 93ead6e5dd..d5d58a205e 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -35,6 +35,7 @@ obj-y += i8259.o
>  obj-y += io_apic.o
>  obj-$(CONFIG_LIVEPATCH) += alternative.o livepatch.o
>  obj-y += msi.o
> +obj-y += msr.o
>  obj-y += ioport_emulate.o
>  obj-y += irq.o
>  obj-$(CONFIG_KEXEC) += machine_kexec.o
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index baaf8151d2..620666b33a 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -425,6 +425,7 @@ int arch_domain_create(struct domain *d,
> unsigned int domcr_flags,
>      {
>          d->arch.emulation_flags = 0;
>          d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
> +        d->arch.msr = ZERO_BLOCK_PTR;
>      }
>      else
>      {
> @@ -470,6 +471,9 @@ int arch_domain_create(struct domain *d,
> unsigned int domcr_flags,
>          if ( (rc = init_domain_cpuid_policy(d)) )
>              goto fail;
> 
> +        if ( (rc = init_domain_msr_policy(d)) )
> +            goto fail;
> +
>          d->arch.ioport_caps =
>              rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
>          rc = -ENOMEM;
> @@ -540,6 +544,7 @@ int arch_domain_create(struct domain *d,
> unsigned int domcr_flags,
>      cleanup_domain_irq_mapping(d);
>      free_xenheap_page(d->shared_info);
>      xfree(d->arch.cpuid);
> +    xfree(d->arch.msr);
>      if ( paging_initialised )
>          paging_final_teardown(d);
>      free_perdomain_mappings(d);
> @@ -554,6 +559,7 @@ void arch_domain_destroy(struct domain *d)
> 
>      xfree(d->arch.e820);
>      xfree(d->arch.cpuid);
> +    xfree(d->arch.msr);
> 
>      free_domain_pirqs(d);
>      if ( !is_idle_domain(d) )
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> new file mode 100644
> index 0000000000..eac50ec897
> --- /dev/null
> +++ b/xen/arch/x86/msr.c
> @@ -0,0 +1,95 @@
> +/************************************************************
> ******************
> + * arch/x86/msr.c
> + *
> + * Policy objects for Model-Specific Registers.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2017 Citrix Systems Ltd.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +#include <asm/msr.h>
> +
> +struct msr_domain_policy __read_mostly hvm_max_msr_domain_policy,
> +                         __read_mostly  pv_max_msr_domain_policy;
> +
> +static void __init calculate_hvm_max_policy(void)
> +{
> +    struct msr_domain_policy *dp = &hvm_max_msr_domain_policy;
> +
> +    if ( !hvm_enabled )
> +        return;
> +
> +    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */

not needed since the code is straightforward?

> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +    {
> +        dp->plaform_info.available = true;
> +        dp->plaform_info.cpuid_faulting = true;
> +    }
> +}
> +
> +static void __init calculate_pv_max_policy(void)
> +{
> +    struct msr_domain_policy *dp = &pv_max_msr_domain_policy;
> +
> +    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
> +    if ( cpu_has_cpuid_faulting )
> +    {
> +        dp->plaform_info.available = true;
> +        dp->plaform_info.cpuid_faulting = true;
> +    }
> +}
> +
> +void __init init_guest_msr_policy(void)
> +{
> +    calculate_hvm_max_policy();
> +    calculate_pv_max_policy();
> +}
> +
> +int init_domain_msr_policy(struct domain *d)
> +{
> +    struct msr_domain_policy *dp;
> +
> +    dp = xmalloc(struct msr_domain_policy);
> +
> +    if ( !dp )
> +        return -ENOMEM;
> +
> +    *dp = is_pv_domain(d) ? pv_max_msr_domain_policy :
> +                            hvm_max_msr_domain_policy;
> +
> +    /* See comment in intel_ctxt_switch_levelling() */

I'd prefer to a brief description here, e.g. "CPUID faulting
has to be disabled for control domain due to the limitation
described in intel_ctxt_switch_levelling()"

> +    if ( is_control_domain(d) )
> +    {
> +        dp->plaform_info.available = false;
> +        dp->plaform_info.cpuid_faulting = false;
> +    }
> +
> +    d->arch.msr = dp;
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index db5df6956d..b9a769d92c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1560,6 +1560,7 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
>          panic("Could not protect TXT memory regions");
> 
>      init_guest_cpuid();
> +    init_guest_msr_policy();
> 
>      if ( dom0_pvh )
>      {
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-
> x86/domain.h
> index c10522b7f5..f08ede3a05 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -356,8 +356,9 @@ struct arch_domain
>       */
>      uint8_t x87_fip_width;
> 
> -    /* CPUID Policy. */
> +    /* CPUID and MSR policy objects. */
>      struct cpuid_policy *cpuid;
> +    struct msr_domain_policy *msr;
> 
>      struct PITState vpit;
> 
> diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> index 7004b6f398..5cf7be1821 100644
> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -202,6 +202,19 @@ void write_efer(u64 val);
> 
>  DECLARE_PER_CPU(u32, ler_msr);
> 
> +/* MSR policy object for shared per-domain MSRs */
> +struct msr_domain_policy
> +{
> +    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
> +    struct {
> +        bool available; /* This MSR is non-architectural */

does "non-architectural" matter in future policy control? If not
no need to mention it.

> +        bool cpuid_faulting;
> +    } plaform_info;
> +};
> +
> +void init_guest_msr_policy(void);
> +int init_domain_msr_policy(struct domain *d);
> +
>  #endif /* !__ASSEMBLY__ */
> 
>  #endif /* __ASM_MSR_H */
> --
> 2.11.0

Above are all cosmetic comments. Here is my:

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

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