Re: [Xen-devel] [PATCH] x86, amd_ucode: Support multiple container files appended together

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.06.14 at 17:14, <aravind.gopalakrishnan@xxxxxxx> wrote:
> @@ -124,6 +127,25 @@ static bool_t verify_patch_size(uint32_t patch_size)
>      return (patch_size <= max_size);
>  }
>  
> +static void find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
> +                              unsigned int current_cpu_id,
> +                              unsigned int *equiv_cpu_id)
> +{
> +    unsigned int i;
> +
> +    if ( !equiv_cpu_table )
> +        return;
> +
> +    for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ )
> +    {
> +        if ( current_cpu_id == equiv_cpu_table[i].installed_cpu )
> +        {
> +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> +            break;
> +        }
> +    }
> +}

Please have this function return a found/not-found indication
rather than having the callers rely on *equiv_cpu_id transitioning
from zero to no-zero. That way you can also drop the respective
initializers in the callers.

> @@ -272,13 +294,16 @@ static int get_ucode_from_buffer_amd(
>  
>  static int install_equiv_cpu_table(
>      struct microcode_amd *mc_amd,
> -    const uint32_t *buf,
> -    size_t *offset)
> +    const uint8_t *data,
> +    size_t *tot_size,
> +    size_t *curr_offset)
>  {
> +    size_t off = *curr_offset;
> +    uint32_t *buf = (uint32_t *) &data[off];

I realize there are may casts there in this file. But please don't add
more of them without a strict need (replacing existing ones would
also be much appreciated). In the case here, if "data" was "const
void *" you'd get away with just "data + off". Furthermore there's
no reason for having the one-time use variable "off" here - just
use *offset directly.

> +static int container_fast_forward(const uint8_t *data, size_t size_left, 
> size_t *offset)
> +{
> +    size_t size, off;
> +    uint32_t *header;
> +
> +    while ( size_left )
> +    {
> +        off = *offset;
> +        header = (uint32_t *) &data[off];

Same comments as a above apply here.

> +        if ( header[0] == UCODE_MAGIC &&
> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = header[1] + SECTION_HDR_SIZE;
> +        *offset += size;
> +        size_left -= size;

Don't you think you should first check for size_left >= size here?

> @@ -334,7 +389,30 @@ static int cpu_request_microcode(int cpu, const void 
> *buf, size_t bufsize)
>          goto out;
>      }
>  
> -    error = install_equiv_cpu_table(mc_amd, buf, &offset);
> +    /*
> +     * Multiple container file support:
> +     * 1. check if this container file has equiv_cpu_id match
> +     * 2. If not, fast-fwd to next container file
> +     */
> +    while ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size,
> +                                             &offset)) == 0 )
> +    {
> +        find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> +                          &equiv_cpu_id);
> +        if ( equiv_cpu_id )
> +            break;
> +
> +        error = container_fast_forward(buf, (bufsize - offset), &offset);
> +        if ( error )
> +        {
> +            printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container 
> file\n"
> +                   "microcode: Failed to update patch level. "
> +                   "Current lvl:%#x\n", cpu, uci->cpu_sig.rev);
> +            error = -EINVAL;

Either you use the error value returned from container_fast_forward(),
or you make that function simply return a bool_t.

> @@ -379,8 +457,16 @@ static int cpu_request_microcode(int cpu, const void 
> *buf, size_t bufsize)
>          save_error = get_ucode_from_buffer_amd(
>              mc_amd, buf, bufsize, &applied_offset);
>  
> +        /*
> +         * If there happens to be multiple container files and if patch
> +         * update succeeded on first container itself, a stale error val
> +         * is returned from get_ucode_from_buffer_amd. So, force
> +         * error = 0 here as we have already succeeded in the update.
> +         */
>          if ( save_error )
>              error = save_error;
> +        else
> +            error = 0;

First of all I don't think this change is logically a part of $subject (as
it also alters behavior for single container files). With that the
question then is why the change is really correct. And if it was really
correct, you could just drop the if() instead.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel