From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH V3] x86, amd_ucode: Support multiple container files appended together Date: Thu, 26 Jun 2014 16:00:04 -0400 Message-ID: <53AC7BC4.6070902@oracle.com> References: <53AC43EF.807@oracle.com> <53AC4C60.7080206@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53AC4C60.7080206@amd.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: Aravind Gopalakrishnan Cc: xen-devel@lists.xen.org, keir@xen.org, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 06/26/2014 12:37 PM, Aravind Gopalakrishnan wrote: > On 6/26/2014 11:01 AM, Boris Ostrovsky wrote: >> >> + >> + error = container_fast_forward(buf, bufsize - >> offset, &offset); >> + if ( error ) >> + { >> + printk(KERN_ERR "microcode: CPU%d incorrect >> or corrupt container file\n" >> + "microcode: Failed to update patch >> level. " >> + "Current lvl:%#x\n", cpu, >> uci->cpu_sig.rev); >> + goto out; >> + } >> + } >> + >> >> >> I just realized that I don't understand something: if you have >> two merged container files, both having an entry for current >> processor in the equivalence table but only the second file >> having the actual patch (or at least the patch with higher >> version), will we get to load that patch? >> >> >> We will not come across such a situation: >> One container has patch files for families 10h,11h,12h,14h and the >> other has patches for 15h. >> So there will be an equiv_cpu_id match in (at least and) only >> *one* of the containers. >> (If there ever are patches for 16h, then I suspect it will have a >> separate container of it's own. So there will not be any clashes >> with older containers) >> >> Besides, the patches themselves are not incremental, i.e; >> If there is a newer patch, then it handles all errata/bugs that >> were handled by the previous patch and then some. >> Which means, you will not have a container that has two patches >> for the same processor. >> So, *strictly speaking*, even the loop to >> get_ucode_from_buffer_amd() till the end of buffer is not >> necessary once we >> have already applied a patch. >> >> But, to change this behavior now will need more logic rework.. >> >> >> I don't think there is a whole lot of work/testing (especially since >> I am not the one doing it ;-)) --- you just need to skip header and >> equivalence table of the next container. >> >> if ( mpbuf->type != UCODE_UCODE_TYPE ) >> { >> if ( *(const uint32_t *)buf == UCODE_MAGIC ) >> { >> struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[2]; // or >> [1]? > Neither will work as we will need to first advance buf 4 bytes.. (but > I get your point) > Regardless, this is not necessary. > Let me clarify (below) >> *offset += mpbuf->len + CONT_HDR_SIZE + 4; // I think 4, to >> skip MAGIC >> // and next word. >> // Now we've fast-forwarded past header and eq. table >> return 0; // or, goto beginning? >> } >> } >> >> Will that work? > > But we don't actually need to parse the second container for reasons > given in the previous thread. > > Example scenarios- > 1. Order of containers in initrd: f15h container, container for > remaining families. > - equiv_cpu_id match for first container (surely you are on a f15h > model) > - parse this container, find correct patch and try to apply > - No need to parse second container as it will not have patches > for f15h. > - no match for 1st container, but match on 2nd, then current > processor is in set [f10h,f14h] > - parse this container, find correct patch and try to apply > - no match on both. > - borked containers. So, abort. > > 2. Order of containers: container for families from f10h-f14h; > container for f15h > - equiv_cpu_id match for 1st container (surely you are on some > family in the set of [f10h, f14h]) > - Parse container, find patch, try to apply > - No need to parse second container as it will not have patches > for f10h-f14h > - found match on 2nd, then current processor is a f15h model, > - parse container, find patch, try to apply > - no match > - borked container. So abort. > > The process(,scripts) to create containers ensure that we only have > patches for f10h-f14h on 'microcode_amd.bin' > and patches for f15h models (could be different model/stepping values) > on microcode_amd_fam15h.bin > > So it is safe to assume that we we only have a matching equiv_id in > only one of the containers. The trouble is that if I have two files in my hands, both called microcode_amd_fam15h.bin, I don't know which one is the more up-to-date (would be nice to have a tool to print file info). Of course, one can argue that in that case I shouldn't be using neither but HW makes its own verification of patches so in principle trying both should be safe (provided that SW tries not to load older revision). -boris