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

>
>>> 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.
> During early boot the equiv table could live right in initrd / ucode
> module as well, couldn't it?

Thinking about it, with a bit of internal refactoring and one new "void
(*bsp_early_load_something_suitable)(buf, size)" hook, we could get all
of the early handling sorted without complicating the "do we cache it,
do we not?" in core.c

In which case I'm now quite convinced that the opaque microcode_patch is
the best way forwards.

>> 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).
> I think we can worry about re-introduction of this when we actually
> learn we'll need it.

Agreed, but prefer not to design myself into a corner by not considering
the implications.

> (Besides shifting allocation responsibility, I
> wonder whether e.g. struct microcode_amd couldn't have a static, i.e.
> build time instance to be used to track early boot data.)

The equivalence table is specific to a single ucode container
(microcode_amd_*.bin), and maps cpu_sig (CPUID.1.EAX) => ucode_sig
(uint16_t, which looks like a compression of CPUID.1 but isn't in some
cases).

In the latest microcode from linux-firmware.git, I see one aliasing
update, where both (cpu) 00100f22 and 00100f23 use (ucode) 1022.  All
others look to be unique blobs.

Thinking about it, the cpu_sig => ucode_sig mapping is almost certainly
fixed for specific cpu, because the latter is the identifying
information in the ucode header.

Therefore, given our homogeneous assumptions, there is a single
applicable ucode_sig globally.  A theoretical heterogeneous case could
maintain a small list (4/8 entries?) of applicable mappings for the
appropriate family, which gets augmented by each equiv table we see.

This might be a very helpful simplification for the AMD driver...

~Andrew

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