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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 6/23/2014 1:33 AM, Jan Beulich wrote:
>>>> On 20.06.14 at 18:06, <aravind.gopalakrishnan@amd.com> wrote:
>> On 6/20/2014 3:11 AM, Jan Beulich wrote:
>>>>>> On 19.06.14 at 17:14, <aravind.gopalakrishnan@amd.com> wrote:
>>>> @@ -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.
>>>
>>>
>> The change is needed (as stated in the comments) because we end up
>> returning err val (EINVAL) like so-
>> DEBUG: cpu_request_microcode :  no save_err;error=-22
>>
>> even though update succeeded-
>> (XEN) microcode: CPU0 found a matching microcode update with version
>> 0x600063d (current=0x6000626)
>> (XEN) microcode: CPU0 updated from revision 0x6000626 to 0x600063d
>>
>> That said, I also understand your point that we can just drop the if().
>> So, I'll change this as per your suggestion.
>>
>> The fact that we have to make this change as a _consequence_ of adding
>> multiple container file support,
>> might explain why it was done later right?
>> I can make this a separate patch as well. Let me know what you think.
> I think I follow what you're saying, in which case this clearly shouldn't
> be a separate patch. But behavior for single-container files still should
> remain unaltered, so some change other than folding the if and else
> branches is going to be needed afaict.
>
>

I have addressed your concern in a V2.

Thanks,
-Aravind.