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

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

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.

Out of consideraton for a mixed-family system. Anyway, Since Jan commented
that we gave up support of a mixed-family system, we only need to save
a single microcode for offlined or hot-plugged cpus.

>
>> >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.

As I am going to remove the microcode cache list, I needn't to iterate
over a list.

Thanks
Chao

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