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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.14 at 00:42, <aravind.gopalakrishnan@xxxxxxx> wrote:
> On 6/24/2014 2:23 AM, Jan Beulich wrote:
>>> @@ -236,7 +260,15 @@ static int get_ucode_from_buffer_amd(
>>>       mpbuf = (const struct mpbhdr *)&bufp[off];
>>>       if ( mpbuf->type != UCODE_UCODE_TYPE )
>>>       {
>>> -        printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
>>> +        /*
>>> +         * We will hit this condition only if an update has succeeded
>>> +         * but there is still another container file being parsed.
>>> +         * In that case, there is no need of this ERR message to be
>>> +         * printed.
>>> +         */
>>> +        if ( mpbuf->type != UCODE_MAGIC )
>>> +            printk(KERN_ERR "microcode: Wrong microcode payload type 
> field\n");
>> The more often I look at it, the less convinced am I that this is okay:
>> UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
>> that the field matches up with the next block signature. Either don't
>> abuse the field, or make clear via comment why it is done this way.
> 
> Would this be better? -
> if ( *(const uint32_t *)buf != UCODE_MAGIC )
> 
> I can still let the above comment be there (or modify it to make it more 
> clear?)

As I said in the previous reply (see above), I'm not really decided
which of the two options is the better one. Perhaps the one using
a cast on buf as you just now suggested is slightly preferable, as
it matches the other similar check(s).

>>> +    return 0;
>>> +}
>>> +
>>> +static int container_fast_forward(const void *data, size_t size_left, 
> size_t *offset)
>>> +{
>>> +    size_t size;
>>> +    uint32_t *header;
>>> +
>>> +    while ( size_left )
>>> +    {
>>> +        header = (uint32_t *) (data + *offset);
>> Pointless (and wrong, as it discards the const qualifier) cast.
> 
> Ok. I have removed this cast (and as a consequence *header) entirely.
> Also need -
> if ( size_left < 0 ) check. Reason-
> With offset value being advanced aggressively in 'install_equiv_cpu_table',
> It could be that we go over the bufsize. This situation will be caught here.

Except that size_left is of unsigned type, i.e. will never be < 0.

Jan


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