[5/6] x86/ucode: Alter ops->free_patch() to free the entire patch
diff mbox series

Message ID 20200319152622.31758-6-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/ucode: Cleanup - Part 1/n
Related show

Commit Message

Andrew Cooper March 19, 2020, 3:26 p.m. UTC
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.  Take the opportunity to make
the hooks idempotent.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c     | 17 ++++++++++++-----
 xen/arch/x86/cpu/microcode/core.c    |  3 +--
 xen/arch/x86/cpu/microcode/intel.c   |  8 ++++++--
 xen/arch/x86/cpu/microcode/private.h |  2 +-
 4 files changed, 20 insertions(+), 10 deletions(-)

Comments

Jan Beulich March 20, 2020, 1:51 p.m. UTC | #1
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.

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?

> Take the opportunity to make the hooks idempotent.

I'm having difficulty seeing what part of the patch this is
about.

Jan
Andrew Cooper March 20, 2020, 2:50 p.m. UTC | #2
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
Jan Beulich March 20, 2020, 3:15 p.m. UTC | #3
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).

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

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

Jan
Andrew Cooper March 20, 2020, 4:10 p.m. UTC | #4
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
Jan Beulich March 20, 2020, 4:16 p.m. UTC | #5
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.

Jan
Andrew Cooper March 20, 2020, 4:48 p.m. UTC | #6
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.

~Andrew
Jan Beulich March 23, 2020, 7:52 a.m. UTC | #7
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

Patch
diff mbox series

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 768fbcf322..77e582c8e1 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -180,10 +180,8 @@  static bool match_cpu(const struct microcode_patch *patch)
     return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
 }
 
-static void free_patch(void *mc)
+static void free_mc_amd(struct microcode_amd *mc_amd)
 {
-    struct microcode_amd *mc_amd = mc;
-
     if ( mc_amd )
     {
         xfree(mc_amd->equiv_cpu_table);
@@ -192,6 +190,15 @@  static void free_patch(void *mc)
     }
 }
 
+static void free_patch(struct microcode_patch *patch)
+{
+    if ( patch )
+    {
+        free_mc_amd(patch->mc_amd);
+        xfree(patch);
+    }
+}
+
 static enum microcode_match_result compare_header(
     const struct microcode_header_amd *new_header,
     const struct microcode_header_amd *old_header)
@@ -564,12 +571,12 @@  static struct microcode_patch *cpu_request_microcode(const void *buf,
             patch->mc_amd = mc_amd;
         else
         {
-            free_patch(mc_amd);
+            free_mc_amd(mc_amd);
             error = -ENOMEM;
         }
     }
     else
-        free_patch(mc_amd);
+        free_mc_amd(mc_amd);
 
   out:
     if ( error && !patch )
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 61e4b9b7ab..30017e3e0f 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -245,8 +245,7 @@  static struct microcode_patch *parse_blob(const char *buf, size_t len)
 
 static void microcode_free_patch(struct microcode_patch *microcode_patch)
 {
-    microcode_ops->free_patch(microcode_patch->mc);
-    xfree(microcode_patch);
+    microcode_ops->free_patch(microcode_patch);
 }
 
 /* Return true if cache gets updated. Otherwise, return false */
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 48544e8d6d..0e6ba50048 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -248,9 +248,13 @@  static bool match_cpu(const struct microcode_patch *patch)
     return microcode_update_match(&patch->mc_intel->hdr) == NEW_UCODE;
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *patch)
 {
-    xfree(mc);
+    if ( patch )
+    {
+        xfree(patch->mc_intel);
+        xfree(patch);
+    }
 }
 
 static enum microcode_match_result compare_patch(
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index c32ddc8d19..897d32a8e9 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -26,7 +26,7 @@  struct microcode_ops {
     int (*apply_microcode)(const struct microcode_patch *patch);
     int (*start_update)(void);
     void (*end_update_percpu)(void);
-    void (*free_patch)(void *mc);
+    void (*free_patch)(struct microcode_patch *patch);
     bool (*match_cpu)(const struct microcode_patch *patch);
     enum microcode_match_result (*compare_patch)(
         const struct microcode_patch *new, const struct microcode_patch *old);