[PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan]

Each family has a stipulated max patch_size. Use this as
additional sanity check before we apply it.

Also, if we find current patch level equal or higher, then we
return early from microcode_fits, but we don't do anything about it.
This means we waste a bit of boot time needlessly parsing remainder
of firmware image.

This patch returns EEXIST when above situation occurs so that-
If we do apply patch during presmp_init, then we save bit of time
when these routines are invoked during microcode_init.
If there is nothing to apply during presmp_init, then we flush
out ucode_blob.size and ucode_blob.data anyway since we are
already at required level.

Since 'microcode_fits' returns int now, modify error machinery
to return EINVAL for error and '0' for success.

While at it, fix comment at very top to indicate we support ucode
patch loading from fam10h and higher.

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/x86/microcode_amd.c |   70 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index b227173..6fafe85 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -8,7 +8,7 @@
  *  Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
  *
  *  This driver allows to upgrade microcode on AMD
- *  family 0x10 and 0x11 processors.
+ *  family 0x10 and later.
  *
  *  Licensed unter the terms of the GNU General Public
  *  License version 2. See file COPYING for details.
@@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
     return 0;
 }
 
-static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
+static bool_t verify_patch_size(uint32_t patch_size)
+{
+    uint8_t family;
+    uint32_t max_size;
+
+#define F1XH_MPB_MAX_SIZE 2048
+#define F14H_MPB_MAX_SIZE 1824
+#define F15H_MPB_MAX_SIZE 4096
+#define F16H_MPB_MAX_SIZE 3458
+
+    family = boot_cpu_data.x86;
+    switch (family)
+    {
+    case 0x14:
+        max_size = F14H_MPB_MAX_SIZE;
+        break;
+    case 0x15:
+        max_size = F15H_MPB_MAX_SIZE;
+        break;
+    case 0x16:
+        max_size = F16H_MPB_MAX_SIZE;
+        break;
+    default:
+        max_size = F1XH_MPB_MAX_SIZE;
+        break;
+    }
+
+    if ( patch_size > max_size )
+        return 0;
+
+    return 1;
+}
+
+static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     const struct microcode_header_amd *mc_header = mc_amd->mpb;
@@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
     }
 
     if ( !equiv_cpu_id )
-        return 0;
+        return -EINVAL;
 
     if ( (mc_header->processor_rev_id) != equiv_cpu_id )
-        return 0;
+        return -EINVAL;
+
+    if ( !verify_patch_size(mc_amd->mpb_size) )
+    {
+        printk(KERN_ERR "microcode: patch size mismatch\n");
+        return -EINVAL;
+    }
 
     if ( mc_header->patch_id <= uci->cpu_sig.rev )
-        return 0;
+        return -EEXIST;
 
     printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
            "update with version %#x (current=%#x)\n",
            cpu, mc_header->patch_id, uci->cpu_sig.rev);
 
-    return 1;
+    return 0;
 }
 
 static int apply_microcode(int cpu)
@@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
     while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
                                                &offset)) == 0 )
     {
-        if ( microcode_fits(mc_amd, cpu) )
+        error = microcode_fits(mc_amd, cpu);
+        if ( !error )
         {
             error = apply_microcode(cpu);
             if ( error )
@@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
 
         last_offset = offset;
 
+        if ( error == -EEXIST ) {
+            printk(KERN_DEBUG "microcode: patch is already at required level or greater.\n");
+            break;
+        }
+
         if ( offset >= bufsize )
             break;
     }
@@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
     const struct microcode_amd *src = mc;
+    int error;
 
-    if ( !microcode_fits(src, cpu) )
-        return 0;
+    error = microcode_fits(src, cpu);
+    if ( error )
+        return error;
 
     if ( src != mc_amd )
     {
@@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
             xfree(mc_amd);
         }
 
+        error = -ENOMEM;
         mc_amd = xmalloc(struct microcode_amd);
         uci->mc.mc_amd = mc_amd;
         if ( !mc_amd )
-            return -ENOMEM;
+            return error;
         mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
         if ( !mc_amd->equiv_cpu_table )
             goto err1;
@@ -408,7 +456,7 @@ err2:
 err1:
     xfree(mc_amd);
     uci->mc.mc_amd = NULL;
-    return -ENOMEM;
+    return error;
 }
 
 static int start_update(void)
-- 
1.7.9.5

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Andrew Cooper]

On 24/04/14 20:54, Aravind Gopalakrishnan wrote:
> Each family has a stipulated max patch_size. Use this as
> additional sanity check before we apply it.
>
> Also, if we find current patch level equal or higher, then we
> return early from microcode_fits, but we don't do anything about it.
> This means we waste a bit of boot time needlessly parsing remainder
> of firmware image.
>
> This patch returns EEXIST when above situation occurs so that-
> If we do apply patch during presmp_init, then we save bit of time
> when these routines are invoked during microcode_init.
> If there is nothing to apply during presmp_init, then we flush
> out ucode_blob.size and ucode_blob.data anyway since we are
> already at required level.
>
> Since 'microcode_fits' returns int now, modify error machinery
> to return EINVAL for error and '0' for success.
>
> While at it, fix comment at very top to indicate we support ucode
> patch loading from fam10h and higher.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
> Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  xen/arch/x86/microcode_amd.c |   70 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 59 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index b227173..6fafe85 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -8,7 +8,7 @@
>   *  Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
>   *
>   *  This driver allows to upgrade microcode on AMD
> - *  family 0x10 and 0x11 processors.
> + *  family 0x10 and later.
>   *
>   *  Licensed unter the terms of the GNU General Public
>   *  License version 2. See file COPYING for details.
> @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
>      return 0;
>  }
>  
> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +static bool_t verify_patch_size(uint32_t patch_size)
> +{
> +    uint8_t family;
> +    uint32_t max_size;
> +
> +#define F1XH_MPB_MAX_SIZE 2048
> +#define F14H_MPB_MAX_SIZE 1824
> +#define F15H_MPB_MAX_SIZE 4096
> +#define F16H_MPB_MAX_SIZE 3458
> +
> +    family = boot_cpu_data.x86;
> +    switch (family)

You can drop the family variable and just switch on boot_cpu_data.x86

> +    {
> +    case 0x14:
> +        max_size = F14H_MPB_MAX_SIZE;
> +        break;
> +    case 0x15:
> +        max_size = F15H_MPB_MAX_SIZE;
> +        break;
> +    case 0x16:
> +        max_size = F16H_MPB_MAX_SIZE;
> +        break;
> +    default:
> +        max_size = F1XH_MPB_MAX_SIZE;
> +        break;
> +    }
> +
> +    if ( patch_size > max_size )
> +        return 0;
> +
> +    return 1;
> +}
> +
> +static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>  {
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      const struct microcode_header_amd *mc_header = mc_amd->mpb;
> @@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>      }
>  
>      if ( !equiv_cpu_id )
> -        return 0;
> +        return -EINVAL;
>  
>      if ( (mc_header->processor_rev_id) != equiv_cpu_id )
> -        return 0;
> +        return -EINVAL;
> +
> +    if ( !verify_patch_size(mc_amd->mpb_size) )
> +    {
> +        printk(KERN_ERR "microcode: patch size mismatch\n");
> +        return -EINVAL;

XENLOG_ERR "microcode patch size too large"
return -E2BIG;

And is this really worth being an error as opposed to a warning, or
frankly even just debug? It is presumably one of N possible blobs in the
hypercall.

> +    }
>  
>      if ( mc_header->patch_id <= uci->cpu_sig.rev )
> -        return 0;
> +        return -EEXIST;
>  
>      printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
>             "update with version %#x (current=%#x)\n",
>             cpu, mc_header->patch_id, uci->cpu_sig.rev);
>  
> -    return 1;
> +    return 0;
>  }
>  
>  static int apply_microcode(int cpu)
> @@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>                                                 &offset)) == 0 )
>      {
> -        if ( microcode_fits(mc_amd, cpu) )
> +        error = microcode_fits(mc_amd, cpu);
> +        if ( !error )
>          {
>              error = apply_microcode(cpu);
>              if ( error )
> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>  
>          last_offset = offset;
>  
> +        if ( error == -EEXIST ) {
> +            printk(KERN_DEBUG "microcode: patch is already at required level or greater.\n");
> +            break;

You can't break from the loop here.  What if a subsequent blob in the
buffer contains a newer piece of microcode?

~Andrew

> +        }
> +
>          if ( offset >= bufsize )
>              break;
>      }
> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>      const struct microcode_amd *src = mc;
> +    int error;
>  
> -    if ( !microcode_fits(src, cpu) )
> -        return 0;
> +    error = microcode_fits(src, cpu);
> +    if ( error )
> +        return error;
>  
>      if ( src != mc_amd )
>      {
> @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>              xfree(mc_amd);
>          }
>  
> +        error = -ENOMEM;
>          mc_amd = xmalloc(struct microcode_amd);
>          uci->mc.mc_amd = mc_amd;
>          if ( !mc_amd )
> -            return -ENOMEM;
> +            return error;
>          mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
>          if ( !mc_amd->equiv_cpu_table )
>              goto err1;
> @@ -408,7 +456,7 @@ err2:
>  err1:
>      xfree(mc_amd);
>      uci->mc.mc_amd = NULL;
> -    return -ENOMEM;
> +    return error;
>  }
>  
>  static int start_update(void)

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan]

On 4/24/2014 3:26 PM, Andrew Cooper wrote:
> On 24/04/14 20:54, Aravind Gopalakrishnan wrote:
>>   
>> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>> +static bool_t verify_patch_size(uint32_t patch_size)
>> +{
>> +    uint8_t family;
>> +    uint32_t max_size;
>> +
>> +#define F1XH_MPB_MAX_SIZE 2048
>> +#define F14H_MPB_MAX_SIZE 1824
>> +#define F15H_MPB_MAX_SIZE 4096
>> +#define F16H_MPB_MAX_SIZE 3458
>> +
>> +    family = boot_cpu_data.x86;
>> +    switch (family)
> You can drop the family variable and just switch on boot_cpu_data.x86

Noted.
>> +static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>>   {
>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>       const struct microcode_header_amd *mc_header = mc_amd->mpb;
>> @@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>>       }
>>   
>>       if ( !equiv_cpu_id )
>> -        return 0;
>> +        return -EINVAL;
>>   
>>       if ( (mc_header->processor_rev_id) != equiv_cpu_id )
>> -        return 0;
>> +        return -EINVAL;
>> +
>> +    if ( !verify_patch_size(mc_amd->mpb_size) )
>> +    {
>> +        printk(KERN_ERR "microcode: patch size mismatch\n");
>> +        return -EINVAL;
> XENLOG_ERR "microcode patch size too large"
> return -E2BIG;
>
> And is this really worth being an error as opposed to a warning, or
> frankly even just debug? It is presumably one of N possible blobs in the
> hypercall.

Right. Modified it to use XENLOG_DEBUG instead.
>>   
>>   static int apply_microcode(int cpu)
>> @@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>>       while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>                                                  &offset)) == 0 )
>>       {
>> -        if ( microcode_fits(mc_amd, cpu) )
>> +        error = microcode_fits(mc_amd, cpu);
>> +        if ( !error )
>>           {
>>               error = apply_microcode(cpu);
>>               if ( error )
>> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>>   
>>           last_offset = offset;
>>   
>> +        if ( error == -EEXIST ) {
>> +            printk(KERN_DEBUG "microcode: patch is already at required level or greater.\n");
>> +            break;
> You can't break from the loop here.  What if a subsequent blob in the
> buffer contains a newer piece of microcode?
>     

Right. Didn't think about that. Sorry..

But I ran into larger issues here:
Since we don't break on the first match and continue to parse the entire 
fw image till the very end;
*and* since I modified the error handling around 'microcode_fits' thus:

if ( (mc_header->processor_rev_id) != equiv_cpu_id )

-return 0;

+return -EINVAL;

+

The last returned error val is '-22' which is bubbled up to microcode.c. 
'microcode_presmp_init' as a result
flushes out ucode_blob or NULL-ifies ucode_mod_map.

Which means 'microcode_init' returns early as it can't validate 
ucode_mod.mod_end or ucode_blob.size
Now, this breaks original functionality (sorry for catching this late), 
*but*
doesn't cause any problem to updating(,applying) ucode patches to cpus 
as we apply the patches during
'microcode_resume_cpu' anyway.  (which is why I thought at first all is 
fine)

Could someone please help me understand why there are two initcalls - 
'microcode_presmp_init' && 'microcode_init'
that perform the same tasks?

It results in these functions - 'collect_cpu_info', 
'cpu_request_microcode' to run a second time through 'microcode_init'
when we have already accomplished the job of updating cpu with microcode 
patches
through 'microcode_presmp_init' and 'microcode_resume_cpu'

If there is particular reason for 'microcode_init''s existence, then 
I'll modify the patch so that the error handling around 'microcode_fits'
is not altered. But if not, then I simply have to just remove the 
'break' statement.

Thanks,
-Aravind.

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Andrew Cooper]

On 25/04/14 20:48, Aravind Gopalakrishnan wrote:
> On 4/24/2014 3:26 PM, Andrew Cooper wrote:
>> On 24/04/14 20:54, Aravind Gopalakrishnan wrote:
>>>   -static bool_t microcode_fits(const struct microcode_amd *mc_amd,
>>> int cpu)
>>> +static bool_t verify_patch_size(uint32_t patch_size)
>>> +{
>>> +    uint8_t family;
>>> +    uint32_t max_size;
>>> +
>>> +#define F1XH_MPB_MAX_SIZE 2048
>>> +#define F14H_MPB_MAX_SIZE 1824
>>> +#define F15H_MPB_MAX_SIZE 4096
>>> +#define F16H_MPB_MAX_SIZE 3458
>>> +
>>> +    family = boot_cpu_data.x86;
>>> +    switch (family)
>> You can drop the family variable and just switch on boot_cpu_data.x86
>
> Noted.
>>> +static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>>>   {
>>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>>       const struct microcode_header_amd *mc_header = mc_amd->mpb;
>>> @@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct
>>> microcode_amd *mc_amd, int cpu)
>>>       }
>>>         if ( !equiv_cpu_id )
>>> -        return 0;
>>> +        return -EINVAL;
>>>         if ( (mc_header->processor_rev_id) != equiv_cpu_id )
>>> -        return 0;
>>> +        return -EINVAL;
>>> +
>>> +    if ( !verify_patch_size(mc_amd->mpb_size) )
>>> +    {
>>> +        printk(KERN_ERR "microcode: patch size mismatch\n");
>>> +        return -EINVAL;
>> XENLOG_ERR "microcode patch size too large"
>> return -E2BIG;
>>
>> And is this really worth being an error as opposed to a warning, or
>> frankly even just debug? It is presumably one of N possible blobs in the
>> hypercall.
>
> Right. Modified it to use XENLOG_DEBUG instead.
>>>     static int apply_microcode(int cpu)
>>> @@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const
>>> void *buf, size_t bufsize)
>>>       while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>>                                                  &offset)) == 0 )
>>>       {
>>> -        if ( microcode_fits(mc_amd, cpu) )
>>> +        error = microcode_fits(mc_amd, cpu);
>>> +        if ( !error )
>>>           {
>>>               error = apply_microcode(cpu);
>>>               if ( error )
>>> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const
>>> void *buf, size_t bufsize)
>>>             last_offset = offset;
>>>   +        if ( error == -EEXIST ) {
>>> +            printk(KERN_DEBUG "microcode: patch is already at
>>> required level or greater.\n");
>>> +            break;
>> You can't break from the loop here.  What if a subsequent blob in the
>> buffer contains a newer piece of microcode?
>>     
>
> Right. Didn't think about that. Sorry..
>
> But I ran into larger issues here:
> Since we don't break on the first match and continue to parse the
> entire fw image till the very end;
> *and* since I modified the error handling around 'microcode_fits' thus:
>
> if ( (mc_header->processor_rev_id) != equiv_cpu_id )
>
> -return 0;
>
> +return -EINVAL;
>
> +
>
> The last returned error val is '-22' which is bubbled up to
> microcode.c. 'microcode_presmp_init' as a result
> flushes out ucode_blob or NULL-ifies ucode_mod_map.
>
> Which means 'microcode_init' returns early as it can't validate
> ucode_mod.mod_end or ucode_blob.size
> Now, this breaks original functionality (sorry for catching this
> late), *but*
> doesn't cause any problem to updating(,applying) ucode patches to cpus
> as we apply the patches during
> 'microcode_resume_cpu' anyway.  (which is why I thought at first all
> is fine)
>
> Could someone please help me understand why there are two initcalls -
> 'microcode_presmp_init' && 'microcode_init'
> that perform the same tasks?
>
> It results in these functions - 'collect_cpu_info',
> 'cpu_request_microcode' to run a second time through 'microcode_init'
> when we have already accomplished the job of updating cpu with
> microcode patches
> through 'microcode_presmp_init' and 'microcode_resume_cpu'
>
> If there is particular reason for 'microcode_init''s existence, then
> I'll modify the patch so that the error handling around 'microcode_fits'
> is not altered. But if not, then I simply have to just remove the
> 'break' statement.

Good grief this looks mad.  Ignoring for a moment what the code actually
does, lets consider what it should do.

The administrator has an option of providing a microcode blob in a
multiboot module, and instructing Xen where to look.  In this case if a
valid blob is found, Xen will want to load the microcode ASAP and cache
it for the other processors.  This can be done as early as is suitable
on the BSP.

The administrator has an option of providing a microcode blob by
hypercall (either automatically as part of their kernel microcode
patching code, or perhaps from a userspace tool in dom0).  For a valid
blob, Xen will want to cache it and apply to all active processors. 
This can be done by whichever cpu processes the hypercall.

At any point, a new cpu coming online (AP boot, S3 etc) should look at
the cached microcode and try to load it.


So it would occur to me to have something such as as presmp initcall
which sets up a cpunotifier for any cpu coming online.  It checks to see
whether there are any blobs provided at boot, tries to load them and
caches the best result.  The hypercall tries to apply the blob from
userspace and if successful, caches the new microcode and instructs all
other cpus to load this new microcode.

I dont see a need for two initcalls in the slightest, nor do I see a
need for using tasklets on boot.  If the earlier initcall hasn't found
any microcode in a multiboot module then there will be nothing for the
latter one to do.  If the earlier did find microcode, then it should be
applied automatically whenever a new cpu comes up, rather than when
explicitly prodded by the later initcall.

Does this make sense?

~Andrew

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Jan Beulich]

>>> On 25.04.14 at 22:30, <andrew.cooper3@citrix.com> wrote:
> On 25/04/14 20:48, Aravind Gopalakrishnan wrote:
>> Could someone please help me understand why there are two initcalls -
>> 'microcode_presmp_init' && 'microcode_init'
>> that perform the same tasks?
>>
>> It results in these functions - 'collect_cpu_info',
>> 'cpu_request_microcode' to run a second time through 'microcode_init'
>> when we have already accomplished the job of updating cpu with
>> microcode patches
>> through 'microcode_presmp_init' and 'microcode_resume_cpu'
>>
>> If there is particular reason for 'microcode_init''s existence, then
>> I'll modify the patch so that the error handling around 'microcode_fits'
>> is not altered. But if not, then I simply have to just remove the
>> 'break' statement.
> 
> Good grief this looks mad.  Ignoring for a moment what the code actually
> does, lets consider what it should do.
> 
> The administrator has an option of providing a microcode blob in a
> multiboot module, and instructing Xen where to look.  In this case if a
> valid blob is found, Xen will want to load the microcode ASAP and cache
> it for the other processors.  This can be done as early as is suitable
> on the BSP.
> 
> The administrator has an option of providing a microcode blob by
> hypercall (either automatically as part of their kernel microcode
> patching code, or perhaps from a userspace tool in dom0).  For a valid
> blob, Xen will want to cache it and apply to all active processors. 
> This can be done by whichever cpu processes the hypercall.
> 
> At any point, a new cpu coming online (AP boot, S3 etc) should look at
> the cached microcode and try to load it.
> 
> 
> So it would occur to me to have something such as as presmp initcall
> which sets up a cpunotifier for any cpu coming online.  It checks to see
> whether there are any blobs provided at boot, tries to load them and
> caches the best result.  The hypercall tries to apply the blob from
> userspace and if successful, caches the new microcode and instructs all
> other cpus to load this new microcode.
> 
> I dont see a need for two initcalls in the slightest, nor do I see a
> need for using tasklets on boot.  If the earlier initcall hasn't found
> any microcode in a multiboot module then there will be nothing for the
> latter one to do.  If the earlier did find microcode, then it should be
> applied automatically whenever a new cpu comes up, rather than when
> explicitly prodded by the later initcall.

The code indeed looks more complicated than one would think it
needs to be, but simplifying it would likely require adding another,
later CPU notification that is being issued _on_ the CPU being
brought up, or an IPI issued from e.g. the CPU_ONLINE callback
(but I'm rather uncertain about it being a good idea to do the ucode
update in an interrupt handler). The whole purpose of the later
initcall and the use of a tasklet there is to have a vehicle to get code
executed _on_ the new CPU, with CPU_STARTING being too early to
hook onto.

Jan

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan]

On 4/28/2014 3:49 AM, Jan Beulich wrote:
>>>> On 25.04.14 at 22:30, <andrew.cooper3@citrix.com> wrote:
>> On 25/04/14 20:48, Aravind Gopalakrishnan wrote:
>>> Could someone please help me understand why there are two initcalls -
>>> 'microcode_presmp_init' && 'microcode_init'
>>> that perform the same tasks?
>>>
>>> It results in these functions - 'collect_cpu_info',
>>> 'cpu_request_microcode' to run a second time through 'microcode_init'
>>> when we have already accomplished the job of updating cpu with
>>> microcode patches
>>> through 'microcode_presmp_init' and 'microcode_resume_cpu'
>>>
>>> If there is particular reason for 'microcode_init''s existence, then
>>> I'll modify the patch so that the error handling around 'microcode_fits'
>>> is not altered. But if not, then I simply have to just remove the
>>> 'break' statement.
>> Good grief this looks mad.  Ignoring for a moment what the code actually
>> does, lets consider what it should do.
>>
>> The administrator has an option of providing a microcode blob in a
>> multiboot module, and instructing Xen where to look.  In this case if a
>> valid blob is found, Xen will want to load the microcode ASAP and cache
>> it for the other processors.  This can be done as early as is suitable
>> on the BSP.
>>
>> The administrator has an option of providing a microcode blob by
>> hypercall (either automatically as part of their kernel microcode
>> patching code, or perhaps from a userspace tool in dom0).  For a valid
>> blob, Xen will want to cache it and apply to all active processors.
>> This can be done by whichever cpu processes the hypercall.
>>
>> At any point, a new cpu coming online (AP boot, S3 etc) should look at
>> the cached microcode and try to load it.
>>
>>
>> So it would occur to me to have something such as as presmp initcall
>> which sets up a cpunotifier for any cpu coming online.  It checks to see
>> whether there are any blobs provided at boot, tries to load them and
>> caches the best result.  The hypercall tries to apply the blob from
>> userspace and if successful, caches the new microcode and instructs all
>> other cpus to load this new microcode.
>>
>> I dont see a need for two initcalls in the slightest, nor do I see a
>> need for using tasklets on boot.  If the earlier initcall hasn't found
>> any microcode in a multiboot module then there will be nothing for the
>> latter one to do.  If the earlier did find microcode, then it should be
>> applied automatically whenever a new cpu comes up, rather than when
>> explicitly prodded by the later initcall.
> The code indeed looks more complicated than one would think it
> needs to be, but simplifying it would likely require adding another,
> later CPU notification that is being issued _on_ the CPU being
> brought up, or an IPI issued from e.g. the CPU_ONLINE callback
> (but I'm rather uncertain about it being a good idea to do the ucode
> update in an interrupt handler). The whole purpose of the later
> initcall and the use of a tasklet there is to have a vehicle to get code
> executed _on_ the new CPU, with CPU_STARTING being too early to
> hook onto.
>
>
Okay, So let me just modify the patch to make sure I don't change 
original behavior.
and worry about simplifying this code at a later time..

Will send out a V2..

Thanks Jan, Andrew
-Aravind

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Jan Beulich]

>>> On 24.04.14 at 21:54, <aravind.gopalakrishnan@amd.com> wrote:
> Each family has a stipulated max patch_size. Use this as
> additional sanity check before we apply it.

And iirc there was a size limit check earlier, and it got dropped for one
reason or another - did you check history?

> @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
>      return 0;
>  }
>  
> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +static bool_t verify_patch_size(uint32_t patch_size)
> +{
> +    uint8_t family;
> +    uint32_t max_size;
> +
> +#define F1XH_MPB_MAX_SIZE 2048
> +#define F14H_MPB_MAX_SIZE 1824
> +#define F15H_MPB_MAX_SIZE 4096
> +#define F16H_MPB_MAX_SIZE 3458
> +
> +    family = boot_cpu_data.x86;
> +    switch (family)
> +    {
> +    case 0x14:
> +        max_size = F14H_MPB_MAX_SIZE;
> +        break;
> +    case 0x15:
> +        max_size = F15H_MPB_MAX_SIZE;
> +        break;
> +    case 0x16:
> +        max_size = F16H_MPB_MAX_SIZE;
> +        break;
> +    default:
> +        max_size = F1XH_MPB_MAX_SIZE;
> +        break;
> +    }
> +
> +    if ( patch_size > max_size )
> +        return 0;
> +
> +    return 1;

Please simply "return patch_size <= max_size" in cases like this.

> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>  
>          last_offset = offset;
>  
> +        if ( error == -EEXIST ) {

Coding style, but as Andrew already indicated this block of code isn't
correct anyway.

> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>      const struct microcode_amd *src = mc;
> +    int error;
>  
> -    if ( !microcode_fits(src, cpu) )
> -        return 0;
> +    error = microcode_fits(src, cpu);
> +    if ( error )
> +        return error;

Is it really correct for this to get switched from success to error return?

> @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>              xfree(mc_amd);
>          }
>  
> +        error = -ENOMEM;
>          mc_amd = xmalloc(struct microcode_amd);
>          uci->mc.mc_amd = mc_amd;
>          if ( !mc_amd )
> -            return -ENOMEM;
> +            return error;

Bogus (pointless) change?

> @@ -408,7 +456,7 @@ err2:
>  err1:
>      xfree(mc_amd);
>      uci->mc.mc_amd = NULL;
> -    return -ENOMEM;
> +    return error;

Same here.

Jan

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan]

On 4/25/2014 2:00 AM, Jan Beulich wrote:
>>>> On 24.04.14 at 21:54, <aravind.gopalakrishnan@amd.com> wrote:
>> Each family has a stipulated max patch_size. Use this as
>> additional sanity check before we apply it.
> And iirc there was a size limit check earlier, and it got dropped for one
> reason or another - did you check history?

Yes, I believe you are referring to this commit:

commit 5663cc8258cef27509a437ebd95061b8b01b9c01
Author:     Christoph Egger <Christoph.Egger@amd.com>
AuthorDate: Thu Dec 15 11:00:09 2011 +0100
Commit:     Christoph Egger <Christoph.Egger@amd.com>
CommitDate: Thu Dec 15 11:00:09 2011 +0100

     x86/ucode: fix for AMD Fam15 CPUs

     Remove hardcoded maximum size a microcode patch can have. This is
     dynamic now.

     The microcode patch for family15h can be larger than 2048 bytes and
     gets silently truncated.

     Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
     Signed-off-by: Jan Beulich <jbeulich@suse.com>
     Committed-by: Jan Beulich <jbeulich@suse.com>

The above patch was to make the microcode patch buffer allocation dynamic.
The hunk below simply verifies that we don't exceed the 'max_size'..
>> @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
>>       return 0;
>>   }
>>   
>> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>> +static bool_t verify_patch_size(uint32_t patch_size)
>> +{
>> +    uint8_t family;
>> +    uint32_t max_size;
>> +
>> +#define F1XH_MPB_MAX_SIZE 2048
>> +#define F14H_MPB_MAX_SIZE 1824
>> +#define F15H_MPB_MAX_SIZE 4096
>> +#define F16H_MPB_MAX_SIZE 3458
>> +
>> +    family = boot_cpu_data.x86;
>> +    switch (family)
>> +    {
>> +    case 0x14:
>> +        max_size = F14H_MPB_MAX_SIZE;
>> +        break;
>> +    case 0x15:
>> +        max_size = F15H_MPB_MAX_SIZE;
>> +        break;
>> +    case 0x16:
>> +        max_size = F16H_MPB_MAX_SIZE;
>> +        break;
>> +    default:
>> +        max_size = F1XH_MPB_MAX_SIZE;
>> +        break;
>> +    }
>> +
>> +    if ( patch_size > max_size )
>> +        return 0;
>> +
>> +    return 1;
> Please simply "return patch_size <= max_size" in cases like this.

Okay.

>> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>>   
>>           last_offset = offset;
>>   
>> +        if ( error == -EEXIST ) {
> Coding style, but as Andrew already indicated this block of code isn't
> correct anyway.

Yes, will fix this. But some help in understanding the microcode_init 
calls would help me here..
(Pleas refer reply to Andrew's comments)
>> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>       struct microcode_amd *mc_amd = uci->mc.mc_amd;
>>       const struct microcode_amd *src = mc;
>> +    int error;
>>   
>> -    if ( !microcode_fits(src, cpu) )
>> -        return 0;
>> +    error = microcode_fits(src, cpu);
>> +    if ( error )
>> +        return error;
> Is it really correct for this to get switched from success to error return?

Previously, microcode_fits returned '1' for Success, '0' for error. So, 
the condition returns '0' when return val is '0'
This mechanism is still preserved in the changes made above..

>> @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>>               xfree(mc_amd);
>>           }
>>   
>> +        error = -ENOMEM;
>>           mc_amd = xmalloc(struct microcode_amd);
>>           uci->mc.mc_amd = mc_amd;
>>           if ( !mc_amd )
>> -            return -ENOMEM;
>> +            return error;
> Bogus (pointless) change?
>
>> @@ -408,7 +456,7 @@ err2:
>>   err1:
>>       xfree(mc_amd);
>>       uci->mc.mc_amd = NULL;
>> -    return -ENOMEM;
>> +    return error;
> Same here.
>

Hmm. Okay, Will just revert this.

Thanks,
-Aravind.

Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Jan Beulich]

>>> On 25.04.14 at 21:48, <aravind.gopalakrishnan@amd.com> wrote:
> On 4/25/2014 2:00 AM, Jan Beulich wrote:
>>>>> On 24.04.14 at 21:54, <aravind.gopalakrishnan@amd.com> wrote:
>>> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>>       struct microcode_amd *mc_amd = uci->mc.mc_amd;
>>>       const struct microcode_amd *src = mc;
>>> +    int error;
>>>   
>>> -    if ( !microcode_fits(src, cpu) )
>>> -        return 0;
>>> +    error = microcode_fits(src, cpu);
>>> +    if ( error )
>>> +        return error;
>> Is it really correct for this to get switched from success to error return?
> 
> Previously, microcode_fits returned '1' for Success, '0' for error. So, 
> the condition returns '0' when return val is '0'
> This mechanism is still preserved in the changes made above..

Definitely not: You return non-zero when "error" is non-zero.

Jan