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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.14 at 19:07, <aravind.gopalakrishnan@xxxxxxx> wrote:
> On 6/27/2014 5:50 AM, Jan Beulich wrote:
>>>>> On 25.06.14 at 21:34, <aravind.gopalakrishnan@xxxxxxx> wrote:
>>> @@ -379,12 +462,35 @@ 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 ( save_error )
>>> +        /*
>>> +         * If there happens to be multiple container files and if patch
>>> +         * update succeeded on an earlier container itself, a stale error
>> Do you perhaps mean bogus instead of stale?
> 
> Yes. Wrong word. Will fix.
> 
>>> +         * val is returned from get_ucode_from_buffer_amd. Since we already
>>> +         * succeeded in patch application, force error = 0
>>> +         */
>>> +        if ( offset < bufsize )
>>> +            error = 0;
>>> +        else if ( save_error )
>>>               error = save_error;
>> And still my question stands: Since you don't look at further containers
>> if you found a match and successfully applied the updated, why is this
>> change needed (or is the comment saying the wrong thing)?
> 
> Maybe the comment needs to be more verbose(?)

It's not about the amount of comment, but about it needing to be
precise.

> Yes, we found a match and yes, we applied the patch successfully.
> But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
> and return -EINVAL
> which is assigned to the variable 'error'
> (Assuming ofc that there is a second container there which we don't need 
> to parse because
> we have already succeeded in patch application)
> 
> This is what I wanted to convey from
> 
> "astale  bogus error val is returned from get_ucode_from_buffer_amd."
> 
> But, we need to return 0 on success; which is why this change is needed 
> here..

I think I understand now: This talks about the case of a _subsequent_
container (never really looked at) following, causing the unwanted
-EINVAL. Whereas your comment said "earlier container", implying (to
me) that it talks about one that earlier code did look at.

>>>       }
>>>   
>>>       if ( save_error )
>>>       {
>>> +        /*
>>> +         * By the time 'microcode_init' runs, we have already updated the
>>> +         * patch level on all (currently) running cpus.
>>> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
>>> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
>>> +         * Multiple containers are present && update succeeded with the
>>> +         * first container file itself.
>>> +         *
>>> +         * Only this time, there is no 'applied_offset' as well.
>>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>> +         * Hence, this check is necessary to return 0 for success.
>>> +         */
>>> +        if ( (error != save_error) && (offset < bufsize) )
>>> +            error = 0;
>> Same for this change/comment.
>>
> 
> Hmm.. I'm having trouble trying to re-word this comment then..
> 
> Given the situation where  - we have already applied the patch update after
> 'microcode_presmp_init' and 'microcode_resume_cpu';
>     |
>     v
> Now 'microcode_init' runs and calls into 'cpu_request_microcode';
>     |
>     v
> We use 1st while loop to find_equiv_cpu_id() and match it with the container
>     |
>     v
> For this particular case, we assume it's a match on the 1st container; 
> so break
>     |
>     v
> Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
>     |
>     v
> At some point, it will find the correct patch; but this time there is no 
> need to update
>     |
>     v
> The behavior is now similar to what I have described above. i.e
> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
> and return -EINVAL
> which is assigned to the variable 'error'
>     |
>     v
> But, now (as stated in the comment..)
> 
> * Only this time, there is no 'applied_offset' as well.
> +         * So, 'save_error' = 1. But error = -EINVAL.
> 
>     |
>     v
> And since we need to return 0 for success, this change is needed here.

So since this is similar to the previous comment, rather than
duplicating information, perhaps just refer to the earlier one, adding
_only_ the information of the different aspect(s) here. And use the
right words: To me at least the "Only this time" implies something
different than what I think you mean - "Except that this time ..."
would be the words I'd use (but a native English speaker may need
to be consulted in case you view this differently).

Jan

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