| Message ID | 20200319152622.31758-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Cleanup - Part 1/n | expand |
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
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
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
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
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
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
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
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);
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(-)