Re: [Xen-devel] [PATCH 3/3] x86: check feature flags after resume

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

Jan Beulich:
> Make sure no previously present features are missing after resume (and
> the re-loading of microcode), to avoid later crashes or (likely silent)
> hangs / live locks. This doesn't go beyond checking x86_capability[],
> but this should be good enough for the immediate need of making sure
> that the BIT mitigation MSRs are still available.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -254,6 +254,9 @@ static int enter_state(u32 state)
>  
>      microcode_resume_cpu(0);
>  
> +    if ( !recheck_cpu_features(0) )
> +        panic("Missing previously available feature(s).");
> +
>      ci->bti_ist_info = default_bti_ist_info;
>      asm volatile (ALTERNATIVE("", "wrmsr", X86_FEATURE_XEN_IBRS_SET)
>                    :: "a" (SPEC_CTRL_IBRS), "c" (MSR_SPEC_CTRL), "d" (0)
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -501,6 +501,9 @@ void identify_cpu(struct cpuinfo_x86 *c)
>       printk("\n");
>  #endif
>  
> +     if (system_state == SYS_STATE_resume)
> +             return;
> +
>       /*
>        * On SMP, boot_cpu_data holds the common feature set between
>        * all CPUs; so make sure that we indicate which features are
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -473,6 +473,28 @@ void __init init_guest_cpuid(void)
>      calculate_hvm_max_policy();
>  }
>  
> +bool recheck_cpu_features(unsigned int cpu)
> +{
> +    bool okay = true;
> +    struct cpuinfo_x86 c;
> +    const struct cpuinfo_x86 *bsp = &boot_cpu_data;
> +    unsigned int i;
> +
> +    identify_cpu(&c);

This runs into a bug in identify_cpu(). x86_vendor_id does not get
zeroed, so the x86_vendor_id is not null terminated and the vendor
identification fails.

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4feaa2ceb6..5750d26216 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -366,8 +366,8 @@ void identify_cpu(struct cpuinfo_x86 *c)
        c->x86_vendor = X86_VENDOR_UNKNOWN;
        c->cpuid_level = -1;    /* CPUID not detected */
        c->x86_model = c->x86_mask = 0; /* So far unknown... */
-       c->x86_vendor_id[0] = '\0'; /* Unset */
-       c->x86_model_id[0] = '\0';  /* Unset */
+       memset(&c->x86_vendor_id, 0, sizeof(c->x86_vendor_id));
+       memset(&c->x86_model_id, 0, sizeof(c->x86_model_id));
        c->x86_max_cores = 1;
        c->x86_num_siblings = 1;
        c->x86_clflush_size = 0;

With this patch it works for me.

> +
> +    for ( i = 0; i < NCAPINTS; ++i )
> +    {
> +        if ( !(~c.x86_capability[i] & bsp->x86_capability[i]) )
> +            continue;
> +
> +        printk(XENLOG_ERR "CPU%u: cap[%2u] is %08x (expected %08x)\n",
> +               cpu, i, c.x86_capability[i], bsp->x86_capability[i]);
> +        okay = false;
> +    }
> +
> +    return okay;
> +}
> +
>  const uint32_t *lookup_deep_deps(uint32_t feature)
>  {
>      static const struct {
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -90,11 +90,14 @@ void initialize_cpu_data(unsigned int cp
>      cpu_data[cpu] = boot_cpu_data;
>  }
>  
> -static void smp_store_cpu_info(int id)
> +static bool smp_store_cpu_info(unsigned int id)
>  {
>      unsigned int socket;
>  
> -    identify_cpu(&cpu_data[id]);
> +    if ( system_state != SYS_STATE_resume )
> +        identify_cpu(&cpu_data[id]);
> +    else if ( !recheck_cpu_features(id) )
> +        return false;
>  
>      socket = cpu_to_socket(id);
>      if ( !socket_cpumask[socket] )
> @@ -102,6 +105,8 @@ static void smp_store_cpu_info(int id)
>          socket_cpumask[socket] = secondary_socket_cpumask;
>          secondary_socket_cpumask = NULL;
>      }
> +
> +    return true;
>  }
>  
>  /*
> @@ -187,12 +192,19 @@ static void smp_callin(void)
>      setup_local_APIC();
>  
>      /* Save our processor parameters. */
> -    smp_store_cpu_info(cpu);
> +    if ( !smp_store_cpu_info(cpu) )
> +    {
> +        printk("CPU%u: Failed to validate features - not coming back 
> online\n",
> +               cpu);
> +        cpu_error = -ENXIO;
> +        goto halt;
> +    }
>  
>      if ( (rc = hvm_cpu_up()) != 0 )
>      {
>          printk("CPU%d: Failed to initialise HVM. Not coming online.\n", cpu);
>          cpu_error = rc;
> +    halt:
>          clear_local_APIC();
>          spin_debug_enable();
>          cpu_exit_clear(cpu);
> --- a/xen/include/asm-x86/cpuid.h
> +++ b/xen/include/asm-x86/cpuid.h
> @@ -253,6 +253,9 @@ static inline void cpuid_featureset_to_p
>  extern struct cpuid_policy raw_cpuid_policy, host_cpuid_policy,
>      pv_max_cpuid_policy, hvm_max_cpuid_policy;
>  
> +/* Check that all previously present features are still available. */
> +bool recheck_cpu_features(unsigned int cpu);
> +
>  /* Allocate and initialise a CPUID policy suitable for the domain. */
>  int init_domain_cpuid_policy(struct domain *d);
>

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel