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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 6/23/2014 1:33 AM, Jan Beulich wrote:
> > > > On 20.06.14 at 18:06, <aravind.gopalakrishnan@xxxxxxx> wrote:
> > On 6/20/2014 3:11 AM, Jan Beulich wrote:
> > > > > > On 19.06.14 at 17:14, <aravind.gopalakrishnan@xxxxxxx> 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.

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