Re: [Xen-devel] [PATCH v6 11/12] x86/microcode: Synchronize late microcode loading

[Chao Gao <chao.gao@xxxxxxxxx>]

On Tue, Mar 12, 2019 at 05:07:51PM -0700, Raj, Ashok wrote:
>On Mon, Mar 11, 2019 at 03:57:35PM +0800, Chao Gao wrote:
>> This patch ports microcode improvement patches from linux kernel.
>> 
>> Before you read any further: the early loading method is still the
>> preferred one and you should always do that. The following patch is
>> improving the late loading mechanism for long running jobs and cloud use
>> cases.
>> 
>> Gather all cores and serialize the microcode update on them by doing it
>> one-by-one to make the late update process as reliable as possible and
>> avoid potential issues caused by the microcode update.
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> Tested-by: Chao Gao <chao.gao@xxxxxxxxx>
>> [linux commit: a5321aec6412b20b5ad15db2d6b916c05349dbff]
>> [linux commit: bb8c13d61a629276a162c1d2b1a20a815cbcfbb7]
>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
>> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Changes in v6:
>>  - Use one timeout period for rendezvous stage and another for update stage.
>>  - scale time to wait by the number of remaining cpus to respond.
>>    It helps to find something wrong earlier and thus we can reboot the
>>    system earlier.
>> ---
>>  xen/arch/x86/microcode.c | 149 
>> ++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 136 insertions(+), 13 deletions(-)
>> 
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index c510808..96bcef6 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -22,6 +22,7 @@
>>   */
>>  
>>  #include <xen/cpu.h>
>> +#include <xen/cpumask.h>
>>  #include <xen/lib.h>
>>  #include <xen/kernel.h>
>>  #include <xen/init.h>
>> @@ -30,15 +31,34 @@
>>  #include <xen/smp.h>
>>  #include <xen/softirq.h>
>>  #include <xen/spinlock.h>
>> +#include <xen/stop_machine.h>
>>  #include <xen/tasklet.h>
>>  #include <xen/guest_access.h>
>>  #include <xen/earlycpio.h>
>> +#include <xen/watchdog.h>
>>  
>> +#include <asm/delay.h>
>>  #include <asm/msr.h>
>>  #include <asm/processor.h>
>>  #include <asm/setup.h>
>>  #include <asm/microcode.h>
>>  
>> +/*
>> + * Before performing a late microcode update on any thread, we
>> + * rendezvous all cpus in stop_machine context. The timeout for
>> + * waiting for cpu rendezvous is 30ms. It is the timeout used by
>> + * live patching
>> + */
>> +#define MICROCODE_CALLIN_TIMEOUT_US 30000
>> +
>> +/*
>> + * Timeout for each thread to complete update is set to 1s. It is a
>> + * conservative choice considering all possible interference (for
>> + * instance, sometimes wbinvd takes relative long time). And a perfect
>> + * timeout doesn't help a lot except an early shutdown.
>> + */
>> +#define MICROCODE_UPDATE_TIMEOUT_US 1000000
>> +
>>  static module_t __initdata ucode_mod;
>>  static signed int __initdata ucode_mod_idx;
>>  static bool_t __initdata ucode_mod_forced;
>> @@ -189,6 +209,12 @@ static DEFINE_SPINLOCK(microcode_mutex);
>>  DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
>>  
>>  /*
>> + * Count the CPUs that have entered and exited the rendezvous
>> + * during late microcode update.
>> + */
>> +static atomic_t cpu_in, cpu_out;
>> +
>> +/*
>>   * Save an ucode patch to the global cache list.
>>   *
>>   * Return true if a patch is added to the global cache or it replaces
>> @@ -284,25 +310,86 @@ static int microcode_update_cpu(void)
>>      return ret;
>>  }
>>  
>> -static long do_microcode_update(void *unused)
>> +/* Wait for CPUs to rendezvous with a timeout (us) */
>> +static int wait_for_cpus(atomic_t *cnt, unsigned int expect,
>> +                         unsigned int timeout)
>>  {
>> -    int error, cpu;
>> +    while ( atomic_read(cnt) < expect )
>> +    {
>> +        if ( timeout <= 0 )
>
>timeout is unsigned int.. it will never be < 0.. flip it to read better maybe?

Will do.

>
>> +        {
>> +            printk("CPU%d: Timeout when waiting for CPUs calling in\n",
>> +                   smp_processor_id());
>> +            return -EBUSY;
>> +        }
>> +        udelay(1);
>> +        timeout--;
>> +    }
>> +
>> +    return 0;
>> +}
>>  
>> -    error = microcode_update_cpu();
>> -    if ( error )
>> -        return error;
>> +static int do_microcode_update(void *unused)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +    unsigned int cpu_nr = num_online_cpus();
>> +    unsigned int finished;
>> +    int ret;
>> +    static bool error;
>>  
>> -    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
>> -    if ( cpu < nr_cpu_ids )
>> -        return continue_hypercall_on_cpu(cpu, do_microcode_update, NULL);
>>  
>> -    return error;
>> +    atomic_inc(&cpu_in);
>> +    ret = wait_for_cpus(&cpu_in, cpu_nr, MICROCODE_CALLIN_TIMEOUT_US);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    /*
>> +     * Initiate an update on all processors which don't have an online 
>> sibling
>> +     * thread with a lower thread id. Other sibling threads just await the
>> +     * completion of microcode update.
>> +     */
>
>The above comment isn't clear. Looks like you are doing the update just
>to the first cpu in the sibling map? 

Yes. Will refine it.

>
>> +    if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>> +        ret = microcode_update_cpu();
>
>Does ret have any useful things on if the update failed? Doesn't seem 
>to be used before you overwrite later in collect_cpu_info()?

It has the reason of failure on error. Actally, there are two reasons:
one is no patch of newer revision, the other is we tried to update but
the microcode revision didn't change. I can check this return value and
print more informative message to admin. And furthermore, for the
latter, we can remove the ucode patch from caches to address Roger's
concern expressed in comments to patch 4 & 5.

>
>
>> +    /*
>> +     * Increase the wait timeout to a safe value here since we're 
>> serializing
>> +     * the microcode update and that could take a while on a large number of
>> +     * CPUs. And that is fine as the *actual* timeout will be determined by
>> +     * the last CPU finished updating and thus cut short
>> +     */
>> +    atomic_inc(&cpu_out);
>> +    finished = atomic_read(&cpu_out);
>> +    while ( !error && finished != cpu_nr )
>
>Maybe i'm reading this wrong.. is "error" used uninitialized?

"error" is a static local variable. So it should be initialized to 0.
And if we got an error, the system will panic; there is no need to
re-initialize "error" at the begining of this function.

Thanks
Chao

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