|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 5/8] microcode: split out apply_microcode() from cpu_request_microcode()
On Tue, Jan 29, 2019 at 10:58:11AM +0100, Roger Pau Monné wrote:
>On Mon, Jan 28, 2019 at 03:06:47PM +0800, Chao Gao wrote:
>> During late microcode update, apply_microcode() is invoked in
>> cpu_request_microcode(). To make late microcode update more reliable,
>> we want to put the apply_microcode() into stop_machine context. So
>> we split out it from cpu_request_microcode(). As a consequence,
>> apply_microcode() should be invoked explicitly in the common code.
>>
>> Also with the global ucode cache, microcode parsing only needs
>> to be done once; cpu_request_microcode() is also moved out of
>> microcode_update_cpu().
>>
>> On AMD side, svm_host_osvw_init() is supposed to be called after
>> microcode update. As apply_micrcode() won't be called by
>> cpu_request_microcode() now, svm_host_osvw_init() is also moved to the
>> end of apply_microcode().
>>
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> ---
>> xen/arch/x86/microcode.c | 80
>> +++++++++++++++++++++++++-----------------
>> xen/arch/x86/microcode_amd.c | 23 +++++++-----
>> xen/arch/x86/microcode_intel.c | 20 ++---------
>> 3 files changed, 64 insertions(+), 59 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 0c77e90..936f0b8 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -254,47 +254,42 @@ struct microcode_patch *find_patch(unsigned int cpu)
>> return NULL;
>> }
>>
>> -int microcode_resume_cpu(unsigned int cpu)
>> +/*
>> + * Return the number of ucode patch inserted to the global cache.
>> + * Return negtive value on error.
>> + */
>> +static int parse_microcode_blob(const void *buffer, size_t len)
>> {
>> - int err;
>> + unsigned int cpu = smp_processor_id();
>> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> -
>> - if ( !microcode_ops )
>> - return 0;
>> + int ret;
>>
>> spin_lock(µcode_mutex);
>> -
>> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> - if ( err )
>> - {
>> + ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> + if ( likely(!ret) )
>> + ret = microcode_ops->cpu_request_microcode(cpu, buffer, len);
>> + else
>> microcode_fini_cpu(cpu);
>> - spin_unlock(µcode_mutex);
>> - return err;
>> - }
>> -
>> - err = microcode_ops->apply_microcode(cpu);
>> spin_unlock(µcode_mutex);
>>
>> - return err;
>> + return ret;
>> }
>>
>> -static int microcode_update_cpu(const void *buf, size_t size)
>> +static int microcode_update_cpu(void)
>> {
>> - int err;
>> - unsigned int cpu = smp_processor_id();
>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> + int ret;
>>
>> spin_lock(µcode_mutex);
>> -
>> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> - if ( likely(!err) )
>> - err = microcode_ops->cpu_request_microcode(cpu, buf, size);
>> - else
>> - microcode_fini_cpu(cpu);
>> -
>> + ret = microcode_ops->apply_microcode(smp_processor_id());
>
>I've realized that most of this helpers take a cpu id parameter
>(apply_microcode, collect_cpu_info and cpu_request_microcode), but
>that at least on Intel they are required to be called on the current
>CPU, at which point I wonder about their purpose. IMO they just make
>the interface more messy, without adding any functionality.
I agree with you and will remove the 'cpu' parameter from the interfaces
you mentioned above.
>
>> spin_unlock(µcode_mutex);
>>
>> - return err;
>> + return ret;
>> +}
>> +
>> +int microcode_resume_cpu(unsigned int cpu)
>> +{
>> + BUG_ON(cpu != smp_processor_id());
>
>Same here, what's the point of passing a cpu id as parameter when
>there's only one valid value?
>
>> + return microcode_ops ? microcode_update_cpu() : 0;
>> }
>>
>> static long do_microcode_update(void *_info)
>> @@ -304,7 +299,7 @@ static long do_microcode_update(void *_info)
>>
>> BUG_ON(info->cpu != smp_processor_id());
>>
>> - error = microcode_update_cpu(info->buffer, info->buffer_size);
>> + error = microcode_update_cpu();
>> if ( error )
>> info->error = error;
>>
>> @@ -339,10 +334,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void)
>> buf, unsigned long len)
>> return ret;
>> }
>>
>> - info->buffer_size = len;
>> - info->error = 0;
>> - info->cpu = cpumask_first(&cpu_online_map);
>> -
>> if ( microcode_ops->start_update )
>> {
>> ret = microcode_ops->start_update();
>> @@ -353,6 +344,18 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void)
>> buf, unsigned long len)
>> }
>> }
>>
>> + ret = parse_microcode_blob(info->buffer, len);
>> + if ( ret <= 0 )
>> + {
>> + printk(XENLOG_ERR "No valid or newer ucode found. Update abort!\n");
>> + xfree(info);
>> + return -EINVAL;
>> + }
>> +
>> + info->buffer_size = len;
>> + info->error = 0;
>> + info->cpu = cpumask_first(&cpu_online_map);
>
>It looks like you can also get rid of the cpu field in microcode_info,
>since it's always smp_processor_id?
Yes. Will remove the 'cpu' field.
>
>> +
>> return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>> }
>>
>> @@ -396,13 +399,24 @@ int __init early_microcode_update_cpu(bool
>> start_update)
>> }
>> if ( data )
>> {
>> + static bool parsed = false;
>> +
>> if ( start_update && microcode_ops->start_update )
>> rc = microcode_ops->start_update();
>>
>> if ( rc )
>> return rc;
>>
>> - return microcode_update_cpu(data, len);
>> + if ( !parsed )
>> + {
>> + rc = parse_microcode_blob(data, len);
>> + parsed = true;
>> +
>> + if ( rc <= 0 )
>> + return -EINVAL;
>> + }
>> +
>> + return microcode_update_cpu();
>
>Hm, the usage of parsed here is kind of dangerous. I assume this works
>fine because early_microcode_update_cpu is called from the BSP in
>early_microcode_init, and then concurrent calls done by the APs always
>see parsed as true.
I think APs are woken up one-by-one. See the call site of cpu_up in
__start_xen.
>
>I would however recommend that you move the parsing to
>early_microcode_init, and that early_microcode_update_cpu always
>assume the blob has been parsed.
>
>If that doesn't work for some reason, I would then recommend that you
>gate the parsing based on cpu_id, so "smp_processor_id() == 0"
Do you think it is better:
remove the parsed flag and check whether current cpu is bsp in
early_microcode_init(); bsp calls the early_microcode_update_cpu(). APs
just call microcode_update_cpu().
>
>> }
>> else
>> return -ENOMEM;
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index d86a596..80e274e 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -297,6 +297,10 @@ static int apply_microcode(unsigned int cpu)
>>
>> uci->cpu_sig.rev = rev;
>>
>> +#if CONFIG_HVM
>> + svm_host_osvw_init();
>> +#endif
>> +
>> return 0;
>> }
>>
>> @@ -463,6 +467,7 @@ static int cpu_request_microcode(unsigned int cpu, const
>> void *buf,
>> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> unsigned int current_cpu_id;
>> unsigned int equiv_cpu_id;
>> + unsigned int matched_cnt = 0;
>>
>> /* We should bind the task to the CPU */
>> BUG_ON(cpu != raw_smp_processor_id());
>> @@ -564,11 +569,15 @@ static int cpu_request_microcode(unsigned int cpu,
>> const void *buf,
>> * this ucode patch before checking whether it matches with
>> * current CPU.
>> */
>> - if ( save_patch(microcode_patch) && microcode_fits(mc_amd, cpu) )
>> + if ( save_patch(microcode_patch) )
>> {
>> - error = apply_microcode(cpu);
>> - if ( error )
>> - break;
>> + matched_cnt++;
>> + if ( microcode_fits(mc_amd, cpu) )
>> + {
>> + error = apply_microcode(cpu);
>> + if ( error )
>> + break;
>> + }
>
>In the commit message you mention that apply_microcode won't be called
>by cpu_request_microcode, yet it seems it's called?
lines below "matched_cnt++" should be removed. They slipped in when I
did a rebasing.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |