[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH V3] x86, amd_ucode: Support multiple container files appended together
On 6/30/2014 4:32 AM, Jan Beulich wrote:
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).
Ok. I have followed a similar step-wise approach (think this is simpler
to follow) in the comments
for V4. Hopefully it's more precisely worded..
I am also working to document container file format, where I will also
talk about why a correct patch can
be on only one or the other container, but not both.
Thanks,
-Aravind.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
