From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH V3] x86, amd_ucode: Support multiple container files appended together Date: Mon, 30 Jun 2014 11:51:13 -0500 Message-ID: <53B19581.7020909@amd.com> References: <1403724849-14360-1-git-send-email-aravind.gopalakrishnan@amd.com> <53AD689D020000780001DE4E@mail.emea.novell.com> <53ADA4D7.3090801@amd.com> <53B14AD9020000780001E6B5@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53B14AD9020000780001E6B5@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel@lists.xen.org, boris.ostrovsky@oracle.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org 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.