Re: [Xen-devel] [PATCH v7 03/10] microcode: introduce a global cache of ucode patch

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.19 at 07:33, <chao.gao@xxxxxxxxx> wrote:
> On Tue, Jun 04, 2019 at 09:03:20AM -0600, Jan Beulich wrote:
>>>>> On 27.05.19 at 10:31, <chao.gao@xxxxxxxxx> wrote:
>>> +static void free_patch(struct microcode_patch *microcode_patch)
>>> +{
>>> +    struct microcode_amd *mc_amd = microcode_patch->mc_amd;
>>> +
>>> +    xfree(mc_amd->equiv_cpu_table);
>>> +    xfree(mc_amd->mpb);
>>> +    xfree(mc_amd);
>>> +    xfree(microcode_patch);
>>
>>I think I said so before: Freeing of the generic wrapper struct
>>would probably better be placed in generic code.
> 
> Do you mean something as shown below:
> 
> /* in generic code */
> 
> struct microcode_patch {
>     union {
>         struct microcode_intel *mc_intel;
>       struct microcode_amd *mc_amd;
>       void *mc;
>     };
> };
> 
> void microcode_free_patch(struct microcode_patch *microcode_patch)
> {
>     microcode_ops->free_patch(microcode_patch->mc);
>     xfree(microcode_patch);
> }
> 
> /* in vendor-specific (AMD) code */
> 
> static void free_patch(void *mc)
> {
>     struct microcode_amd *mc_amd = mc;
> 
>     xfree(mc_amd->equiv_cpu_table);
>     xfree(mc_amd->mpb);
>     xfree(mc_amd);
> }

Something along these lines, yes. Whether you do as above or
whether instead you continue to pass struct microcode_patch *
is secondary (and really up to you). Perhaps the above the
(slightly) better.

>>> @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, 
>>> const void *buf,
>>>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>>                                                 &offset)) == 0 )
>>>      {
>>> -        if ( microcode_fits(mc_amd, cpu) )
>>> +        struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd);
>>> +
>>> +        if ( IS_ERR(new_patch) )
>>> +        {
>>> +            error = PTR_ERR(new_patch);
>>> +            break;
>>> +        }
>>> +
>>> +        if ( match_cpu(new_patch) )
>>> +            microcode_update_cache(new_patch);
>>> +        else
>>> +            free_patch(new_patch);
>>
>>Why do you re-do what microcode_update_cache() already does?
>>It calls ->match_cpu() and ->free_patch() all by itself. It looks as
>>if it would need to gain one more ->free_patch() invocation though.
>>
> 
> Will remove both invocations of match_cpu().
> 
> To support the case (the broken bios) you described, a patch which
> needs to be stored isn't necessary to be newer than the microcode loaded
> to current CPU. As long as the processor's signature is covered by the
> patch, we will store the patch regardless the revision number.

Well, if so, then fine. I did get the impression that successful application
is a required pre-condition for storing.

Jan



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