On 2/21/2018 2:13 PM, Raj, Ashok wrote:
> On Wed, Feb 21, 2018 at 08:06:11PM +0100, Borislav Petkov wrote:
>>>  arch/x86/kernel/cpu/microcode/core.c  | 113 
>>> +++++++++++++++++++++++++++++-----
>>
>> This is generic so Tom needs to ack whatever we end up doing for the AMD
>> side.
> 
> Yes, i did ping Tom to check if this is ok with them.

I did some testing with these patches and didn't notice any issues on my
EPYC system.  At the moment, I currently don't have access to anything
older on which to test.  But I don't believe there should be any issues
with this approach.  I'll retest when we get closer to the final version
of the patch.

Thanks,
Tom

> >>
>>>  arch/x86/kernel/cpu/microcode/intel.c |   1 +
>>>  2 files changed, 98 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/microcode/core.c 
>>> b/arch/x86/kernel/cpu/microcode/core.c
>>> index aa1b9a4..af0aeb2 100644
>>> --- a/arch/x86/kernel/cpu/microcode/core.c
>>> +++ b/arch/x86/kernel/cpu/microcode/core.c
>>> @@ -31,6 +31,9 @@
>>>  #include <linux/cpu.h>
>>>  #include <linux/fs.h>
>>>  #include <linux/mm.h>
>>> +#include <linux/nmi.h>
>>> +#include <linux/stop_machine.h>
>>> +#include <linux/delay.h>
>>>  
>>>  #include <asm/microcode_intel.h>
>>>  #include <asm/cpu_device_id.h>
>>> @@ -489,19 +492,82 @@ static void __exit microcode_dev_exit(void)
>>>  /* fake device for request_firmware */
>>>  static struct platform_device      *microcode_pdev;
>>>  
>>> -static enum ucode_state reload_for_cpu(int cpu)
>>> +static struct ucode_update_param {
>>> +   spinlock_t ucode_lock;
>>> +   atomic_t   count;
>>> +   atomic_t   errors;
>>> +   atomic_t   enter;
>>> +   int        timeout;
>>> +} uc_data;
>>> +
>>> +static void do_ucode_update(int cpu, struct ucode_update_param *ucd)
>>>  {
>>> -   struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>>> -   enum ucode_state ustate;
>>> +   enum ucode_state retval = 0;
>>>  
>>> -   if (!uci->valid)
>>> -           return UCODE_OK;
>>> +   spin_lock(&ucd->ucode_lock);
>>> +   retval = microcode_ops->apply_microcode(cpu);
>>> +   spin_unlock(&ucd->ucode_lock);
>>
>> What's the spinlock protecting against?
> 
> This is ensuring no 2 cpus do ucode update at the same time.
> 
> Since all cpus wait for all the online cpus to arrive in stop_machine handler.
> Once we let go, every cpu tries to update. This just serializes against that.
> 
>>
>> We hold the hotplug lock and the microcode mutex. And yet interrupts are
>> still enabled. So what's up?
> 
> hotplug lock/microcode mutex are at global level, these are 
> protecting individual cpus in stop machine trying to update microcode.
> 
> these are called while in stop_machine() so i think interrupts are disabled 
> IRC.
> 
>>
>>
>>> +   if (retval > UCODE_NFOUND) {
>>> +           atomic_inc(&ucd->errors);
>>
>> You don't need ->errors. Simply propagate retval from do_ucode_update().
>> Or compare ucd->count to the number of CPUs. Or something like that.
> 
> That's what we are doing here, but simply returning number of cpus
> that encountered failure instead of a per-cpu retval
> like before.
> 
> I use ucd->count to use as an exit rendezvous.. to make sure we leave only
> after all cpus have done updating ucode.
> 
>>> +           pr_warn("microcode update to cpu %d failed\n", cpu);
>>> +   }
>>> +   atomic_inc(&ucd->count);
>>> +}
>>> +
>>> +/*
>>> + * Wait for upto 1sec for all cpus
>>> + * to show up in the rendezvous function
>>> + */
>>> +#define MAX_UCODE_RENDEZVOUS       1000000000 /* nanosec */
>>
>>                              1 * NSEC_PER_SEC
>>
>>> +#define SPINUNIT           100        /* 100ns */
>>> +
>>> +/*
>>> + * Each cpu waits for 1sec max.
>>> + */
>>> +static int ucode_wait_timedout(int *time_out, void *data)
>>> +{
>>> +   struct ucode_update_param *ucd = data;
>>> +   if (*time_out < SPINUNIT) {
>>> +           pr_err("Not all cpus entered ucode update handler %d cpus 
>>> missing\n",
>>> +                   (num_online_cpus() - atomic_read(&ucd->enter)));
>>> +           return 1;
>>> +   }
>>> +   *time_out -= SPINUNIT;
>>> +   touch_nmi_watchdog();
>>> +   return 0;
>>> +}
>>> +
>>> +/*
>>> + * All cpus enter here before a ucode load upto 1 sec.
>>> + * If not all cpus showed up, we abort the ucode update
>>> + * and return. ucode update is serialized with the spinlock
>>
>> ... and yet you don't check stop_machine()'s retval and issue an error
>> message that it failed.
>>
> 
> Will add that 
> 
>>> + */
>>> +static int ucode_load_rendezvous(void *data)
>>
>> The correct prefix is "microcode_"
>>
>>> +{
>>> +   int cpu = smp_processor_id();
>>> +   struct ucode_update_param *ucd = data;
>>> +   int timeout = MAX_UCODE_RENDEZVOUS;
>>> +   int total_cpus = num_online_cpus();
>>>  
>>> -   ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, 
>>> true);
>>> -   if (ustate != UCODE_OK)
>>> -           return ustate;
>>> +   /*
>>> +    * Wait for all cpu's to arrive
>>> +    */
>>> +   atomic_dec(&ucd->enter);
>>> +   while(atomic_read(&ucd->enter)) {
>>> +           if (ucode_wait_timedout(&timeout, ucd))
>>> +                   return 1;
>>> +           ndelay(SPINUNIT);
>>> +   }
>>> +
>>> +   do_ucode_update(cpu, ucd);
>>>  
>>> -   return apply_microcode_on_target(cpu);
>>> +   /*
>>> +    * Wait for all cpu's to complete
>>> +    * ucode update
>>> +    */
>>> +   while (atomic_read(&ucd->count) != total_cpus)
>>> +           cpu_relax();
>>> +   return 0;
>>>  }
>>>  
>>>  static ssize_t reload_store(struct device *dev,
>>> @@ -509,7 +575,6 @@ static ssize_t reload_store(struct device *dev,
>>>                         const char *buf, size_t size)
>>>  {
>>>     enum ucode_state tmp_ret = UCODE_OK;
>>> -   bool do_callback = false;
>>>     unsigned long val;
>>>     ssize_t ret = 0;
>>>     int cpu;
>>> @@ -523,21 +588,37 @@ static ssize_t reload_store(struct device *dev,
>>>  
>>>     get_online_cpus();
>>>     mutex_lock(&microcode_mutex);
>>> +   /*
>>> +    * First load the microcode file for all cpu's
>>> +    */
>>
>> It is always "CPU" or "CPUs". Fix all misspelled places.
>>
>>>     for_each_online_cpu(cpu) {
>>
>> You need to fail loading and not even try when not all cores are online.
>> And issue an error message.
>>
> 
> When we online any of the offline cpu's we do a microcode load again right? 
> 
> I did check with offlining 2 threads of the same core offline, then reload 
> with a 
> new version of microcode. Online the new cpus i did find the microcode was 
> updated 
> during online process.
> 
> Since offline ones don't participate in any OS activity  thought its ok to 
> update everything that is available and visitible to the OS.
> 
> If BIOS has turned off cores due to some failures and didn't expose
> in MADT during boot, we will never get a chance to update online.
> 
>>> -           tmp_ret = reload_for_cpu(cpu);
>>> +           tmp_ret = microcode_ops->request_microcode_fw(cpu,
>>> +                           &microcode_pdev->dev, true);
>>
>> This needs to happen only once - not per CPU. Let's simply forget
>> heterogeneous microcode revisions.
> 
> Sounds good.. let me take a look at this.
> 
>>
>>>             if (tmp_ret > UCODE_NFOUND) {
>>> -                   pr_warn("Error reloading microcode on CPU %d\n", cpu);
>>> +                   pr_warn("Error reloading microcode file for CPU %d\n", 
>>> cpu);
>>>  
>>>                     /* set retval for the first encountered reload error */
>>>                     if (!ret)
>>>                             ret = -EINVAL;
>>
>> You can't continue loading here if you've encountered an error.
> 
> Sounds good.
>>
>>>             }
>>> -
>>> -           if (tmp_ret == UCODE_UPDATED)
>>> -                   do_callback = true;
>>>     }
>>> +   pr_debug("Done loading microcode file for all cpus\n");
>>>  
>>> -   if (!ret && do_callback)
>>> +   memset(&uc_data, 0, sizeof(struct ucode_update_param));
>>> +   spin_lock_init(&uc_data.ucode_lock);
>>> +   atomic_set(&uc_data.enter, num_online_cpus());
>>> +   /*
>>> +    * Wait for a 1 sec
>>> +    */
>>> +   uc_data.timeout = USEC_PER_SEC;
>>> +   stop_machine(ucode_load_rendezvous, &uc_data, cpu_online_mask);
>>> +
>>> +   pr_debug("Total CPUS = %d uperrors = %d\n",
>>> +           atomic_read(&uc_data.count), atomic_read(&uc_data.errors));
>>> +
>>> +   if (atomic_read(&uc_data.errors))
>>> +           pr_warn("Update failed for %d cpus\n", 
>>> atomic_read(&uc_data.errors));
>>> +   else
>>>             microcode_check();
>>
>> This whole jumping through hoops needs to be extracted away in a
>> separate function.
> 
> Not sure what you mean by jumping through hoops need to be extracted away.. 
> 
>>
>> Ok, that should be enough review for now. More with v2.
>>
>> -- 
>> Regards/Gruss,
>>     Boris.
>>
>> SUSE Linux GmbH, GF: Felix Imend├Ârffer, Jane Smithard, Graham Norton, HRB 
>> 21284 (AG N├╝rnberg)
>> -- 

Reply via email to