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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> However, having taken a look also at the next patch I wonder
> why you even retain that wrapper structure containing just
> a single pointer? Why can't what is now
> struct microcode_{amd,intel} become struct microcode_patch,
> with - as you say there - different per-vendor layout which
> is opaque to common code?

Various fixes along these lines are pending (but having the resulting
change not be "rewrite the entire file from scratch" is proving harder
than I'd anticipated).

Both Intel and AMD make pointless intermediate memory allocations /
frees for every individual ucode they find in the containers.  Fixing
this is moderately easy and an obvious win.


However, I was also thinking further forwards, perhaps with some
different changes.

We've currently got some awkward hoops to jump through for accessing the
initrd/ucode module, and the dependency on memory allocations forces us
to load microcode much later than ideal on boot.

I was considering whether we could rearrange things so all allocations
were done in core.c, with the vendor specific logic simply identifying a
subset of the provided buffer if an applicable patch is found.

This way, very early boot can load straight out of the initrd/ucode
module (or builtin firmware, for which there is a patch outstanding),
and setting up the ucode cash can happen later when dynamic memory
allocations are available.

This is easy to do for Intel, and not so easy for AMD, given the second
allocation for the equivalence table.

For AMD, the ucode patches don't have the processor signature in them,
and the use of the equivalence table is necessary to turn the processor
signature into the opaque signature in the ucode header.   After
parsing, it is only used for sanity checks, and given the other
restrictions we have on assuming a heterogeneous system, I think we can
get away with dropping the allocation.

OTOH, if we do go down these lines (and specifically, shift the
allocation reponsibility into core.c), I can't see a way of
reintroducing heterogeneous support (on AMD.  Again, Intel is easy, and
we're going to need it eventually for Lakefield support).

Thoughts?

>
>> Take the opportunity to make the hooks idempotent.
> I'm having difficulty seeing what part of the patch this is
> about.

The "if ( patch )" clauses in free_patch().

but I realise that what I meant to write was "tolerate NULL".  Sorry.

We have a weird mix where some of the functions tolerate a NULL patch
(where they can reasonably expect never to be given NULL), but the free
hook doesn't (where it would be most useful for caller simplicity).

~Andrew

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