Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2020 17:48, Andrew Cooper wrote:
> On 20/03/2020 16:16, Jan Beulich wrote:
>> On 20.03.2020 17:10, Andrew Cooper wrote:
>>> On 20/03/2020 15:15, Jan Beulich wrote:
>>>> On 20.03.2020 15:50, Andrew Cooper wrote:
>>>>> On 20/03/2020 13:51, Jan Beulich wrote:
>>>>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>>>>> The data layout for struct microcode_patch is extremely poor, and
>>>>>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with 
>>>>>>> the
>>>>>>> exception of free_patch().
>>>>>>>
>>>>>>> Move the responsibility for freeing the patch into the free_patch() 
>>>>>>> hook,
>>>>>>> which will allow each driver to do a better job.
>>>>>> But that wrapper structure is something common, i.e. to be
>>>>>> allocated as well as to be freed (preferably) by common code.
>>>>>> We did specifically move there during review of the most
>>>>>> recent re-work.
>>>>> The current behaviour of having it allocated by the request() hook, but
>>>>> "freed" in a mix of common code and a free() hook, cannot possibly have
>>>>> been an intended consequence from moving it.
>>>>>
>>>>> The free() hook is currently necessary, as is the vendor-specific
>>>>> allocation logic, so splitting freeing responsibility with the common
>>>>> code is wrong.
>>>> Hmm, yes, with the allocation done in vendor code, the freeing
>>>> could be, too. But the wrapper struct gets allocated last in
>>>> cpu_request_microcode() (for both Intel and AMD), and hence ought
>>>> to be relatively easy to get rid of, instead of moving around
>>>> the freeing (the common code part of the freeing would then
>>>> simply go away).
>>> I am working on removing all unnecessary allocations, including folding
>>> microcode_{intel,amd} into microcode_patch, but I'm still confident this
>>> wants to be done with microcode_patch being properly opaque to core.c
>> Oh, sure - I didn't mean to put this under question. It just seems
>> to me the the route there may better be somewhat different from this
>> and the following patch.
> 
> How?
> 
> We want to remove the pointer from microcode_patch, and don't want the
> current contents of microcode_{intel,amd} escaping from their current
> source files.
> 
> I don't see any option but to rearrange it to be opaque.

I agree. But why do you need to first re-arrange freeing, if then you
drop the wrapper struct (which really is a union) anyway? By dropping
it right away, the split freeing will go away as a side effect.

Jan

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