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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 13.04.18 at 20:56, <simon@invisiblethingslab.com> wrote:
> Simon Gaiser:
>> 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@suse.com>
>>>
>>> --- 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.
> 
> Meh, also a backport failure from me. Since e34bc403c3c7 this problem
> should not appear since it does not assume a null terminated string.

NP - it's good to be aware of such issues in case we as well decide to
backport this.

Thanks for the feedback,
Jan



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