Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check

[Chao Gao <chao.gao@intel.com>]

On Tue, Dec 04, 2018 at 10:39:03PM +0000, Woods, Brian wrote:
>On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
>> On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
>> > On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>> > >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>> > >> ... and search caches to find a suitable one when loading.
>> > >
>> > >Why do you need to save all of them? You are only going to load a
>> > >single microcode, so I don't understand the need to cache them all.
>> 
>> I think the above question needs an answer.
>> 
>> > >IMO making such modifications to the AMD code without testing it is
>> > >very dangerous. Could you get an AMD system or ask an AMD dev to test
>> > >it? I would try with the AMD SVM maintainers.
>> > 
>> > It is improbable for me to find an AMD machine in my team. I will copy AMD
>> > SVM maintainers in the coming versions and ask them to help to test this
>> > series.
>> 
>> I'm Cc'ing them now in case they want to provide some feedback.
>> 
>> > >> +static int save_patch(struct ucode_patch *new_patch)
>> > >> +{
>> > >> +    struct ucode_patch *ucode_patch;
>> > >> +    struct microcode_amd *new_mc = new_patch->data;
>> > >> +    struct microcode_header_amd *new_header = new_mc->mpb;
>> > >> +
>> > >> +    list_for_each_entry(ucode_patch, &microcode_cache, list)
>> > >> +    {
>> > >> +        struct microcode_amd *old_mc = ucode_patch->data;
>> > >> +        struct microcode_header_amd *old_header = old_mc->mpb;
>> > >> +
>> > >> +        if ( new_header->processor_rev_id == old_header->processor_rev_id )
>> > >> +        {
>> > >> +            if ( new_header->patch_id <= old_header->patch_id )
>> > >> +                return -1;
>> > >> +            list_replace(&ucode_patch->list, &new_patch->list);
>> > >> +            free_ucode_patch(ucode_patch);
>> > >> +            return 0;
>> > >> +        }
>> > >> +    }
>> > >
>> > >This could be made common code with a specific hook for AMD and Intel
>> > >in order to do the comparison, so that at least the loop over the
>> > >list of ucode entries could be shared.
>> > 
>> > Something like pt_pirq_iterate()? Will give it a try.
>> 
>> Yes, that might also be helpful. I was thinking of adding such a
>> comparison hook in microcode_ops, also having something like
>> pt_pirq_iterate will be helpful if you need to iterate over the cache
>> in other functions.
>> 
>> > >> @@ -491,6 +559,21 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>> > >>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>> > >>                                                 &offset)) == 0 )
>> > >>      {
>> > >> +        struct ucode_patch *ucode_patch;
>> > >> +
>> > >> +        /*
>> > >> +         * Save this microcode before checking the signature. It is to
>> > >> +         * optimize microcode update on a mixed family system. Parsing
>> > >
>> > >Er, is it possible to have a system with CPUs of different family?
>> > >What's going to happen with CPUs having different features?
>> > 
>> > I have no idea. That each cpu has a per-cpu variable to store the
>> > microcode rather than a global one gives me a feeling that the current
>> > implementation wants to make it work on a system with CPUs of different
>> > family.
>> 
>> I think we need AMD maintainers input on this one. TBH I very much
>> doubt there are (working) systems out there with mixed family CPUs.
>> 
>> Thanks, Roger.
>
>Sorry about the delay.  From the PPR for F17 M00-0FH
>"AMD Family 17h processors with different OPNs or different revisions
>cannot be mixed in a multiprocessor system. If an unsupported
>configuration is detected, BIOS should configure the BSP as a single
>processor system and signal an error."
>
>Even mixing OPNs within a model is a no go for us.  Mixing different
>families is something I highly doubt will ever be supported.
>

Thanks for this information. It is my bad to use the word 'mixed-family'
here without thinking over its rationality or checking SDM carefully.
Will reword the description here.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel