| Message ID | 1566177928-19114-13-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve late microcode loading | expand |
On Mon, Aug 19, 2019 at 09:25:25AM +0800, Chao Gao wrote: > To create a microcode patch from a vendor-specific update, > allocate_microcode_patch() copied everything from the update. > It is not efficient. Essentially, we just need to go through > ucodes in the blob, find the one with the newest revision and > install it into the microcode_patch. In the process, buffers > like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel > side) can be reused. microcode_patch now is allocated after > it is sure that there is a matching ucode. Oh, I think this answers my question on a previous patch. For future series it would be nice to avoid so many rewrites in the same series, alloc_microcode_patch is already modified in a previous patch, just to be removed here. It also makes it harder to follow what's going on. > > Signed-off-by: Chao Gao <chao.gao@intel.com> > --- > Changes in v9: > - new > --- > xen/arch/x86/microcode_amd.c | 99 +++++++++++++++--------------------------- > xen/arch/x86/microcode_intel.c | 65 ++++++++++----------------- > 2 files changed, 58 insertions(+), 106 deletions(-) > > diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c > index 6353323..ec1c2eb 100644 > --- a/xen/arch/x86/microcode_amd.c > +++ b/xen/arch/x86/microcode_amd.c > @@ -194,36 +194,6 @@ static bool match_cpu(const struct microcode_patch *patch) > return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE); > } > > -static struct microcode_patch *alloc_microcode_patch( > - const struct microcode_amd *mc_amd) > -{ > - struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch); > - struct microcode_amd *cache = xmalloc(struct microcode_amd); > - void *mpb = xmalloc_bytes(mc_amd->mpb_size); > - struct equiv_cpu_entry *equiv_cpu_table = > - xmalloc_bytes(mc_amd->equiv_cpu_table_size); > - > - if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table ) > - { > - xfree(microcode_patch); > - xfree(cache); > - xfree(mpb); > - xfree(equiv_cpu_table); > - return ERR_PTR(-ENOMEM); > - } > - > - memcpy(mpb, mc_amd->mpb, mc_amd->mpb_size); > - cache->mpb = mpb; > - cache->mpb_size = mc_amd->mpb_size; > - memcpy(equiv_cpu_table, mc_amd->equiv_cpu_table, > - mc_amd->equiv_cpu_table_size); > - cache->equiv_cpu_table = equiv_cpu_table; > - cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size; > - microcode_patch->mc_amd = cache; > - > - return microcode_patch; > -} > - > static void free_patch(void *mc) > { > struct microcode_amd *mc_amd = mc; > @@ -320,18 +290,10 @@ static int get_ucode_from_buffer_amd( > return -EINVAL; > } > > - if ( mc_amd->mpb_size < mpbuf->len ) > - { > - if ( mc_amd->mpb ) > - { > - xfree(mc_amd->mpb); > - mc_amd->mpb_size = 0; > - } > - mc_amd->mpb = xmalloc_bytes(mpbuf->len); > - if ( mc_amd->mpb == NULL ) > - return -ENOMEM; > - mc_amd->mpb_size = mpbuf->len; > - } > + mc_amd->mpb = xmalloc_bytes(mpbuf->len); > + if ( mc_amd->mpb == NULL ) Nit: if ( !mc_amd->mpb ) is the usual idiom used in Xen. > + return -ENOMEM; > + mc_amd->mpb_size = mpbuf->len; > memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len); > > pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", > @@ -451,8 +413,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > size_t bufsize) > { > struct microcode_amd *mc_amd; > + struct microcode_header_amd *saved = NULL; > struct microcode_patch *patch = NULL; > - size_t offset = 0; > + size_t offset = 0, saved_size = 0; > int error = 0; > unsigned int current_cpu_id; > unsigned int equiv_cpu_id; > @@ -542,29 +505,21 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > &offset)) == 0 ) > { > - struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); > - > - if ( IS_ERR(new_patch) ) > - { > - error = PTR_ERR(new_patch); > - break; > - } > - > /* > - * If the new patch covers current CPU, compare patches and store the > + * If the new ucode covers current CPU, compare ucodes and store the > * one with higher revision. > */ > - if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) && > - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) > +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id) > + if ( (microcode_fits(mc_amd) != MIS_UCODE) && > + (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) ) > +#undef REV_ID > { > - struct microcode_patch *tmp = patch; > - > - patch = new_patch; > - new_patch = tmp; > + xfree(saved); > + saved = mc_amd->mpb; > + saved_size = mc_amd->mpb_size; > } > - > - if ( new_patch ) > - microcode_free_patch(new_patch); > + else > + xfree(mc_amd->mpb); > > if ( offset >= bufsize ) > break; > @@ -593,9 +548,25 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) > break; > } > - xfree(mc_amd->mpb); > - xfree(mc_amd->equiv_cpu_table); > - xfree(mc_amd); > + > + if ( saved ) > + { > + mc_amd->mpb = saved; > + mc_amd->mpb_size = saved_size; > + patch = xmalloc(struct microcode_patch); > + if ( patch ) > + patch->mc_amd = mc_amd; > + else > + { > + free_patch(mc_amd); > + error = -ENOMEM; > + } > + } > + else > + { > + mc_amd->mpb = NULL; What's the point in setting mpb to NULL if you are just going to free mc_amd below? Also, I'm not sure I understand why you need to free mc_amd, isn't this buff memory that should be freed by the caller? ie: in the Intel counterpart below you don't seem to free the mc cursor used for the get_next_ucode_from_buffer loop. > + free_patch(mc_amd); > + } > > out: > if ( error && !patch ) > diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c > index 96b38f8..ae5759f 100644 > --- a/xen/arch/x86/microcode_intel.c > +++ b/xen/arch/x86/microcode_intel.c > @@ -282,25 +282,6 @@ static enum microcode_match_result compare_patch( > OLD_UCODE; > } > > -static struct microcode_patch *alloc_microcode_patch( > - const struct microcode_header_intel *mc_header) > -{ > - unsigned long total_size = get_totalsize(mc_header); > - void *new_mc = xmalloc_bytes(total_size); > - struct microcode_patch *new_patch = xmalloc(struct microcode_patch); > - > - if ( !new_patch || !new_mc ) > - { > - xfree(new_patch); > - xfree(new_mc); > - return ERR_PTR(-ENOMEM); > - } > - memcpy(new_mc, mc_header, total_size); > - new_patch->mc_intel = new_mc; > - > - return new_patch; > -} > - > static int apply_microcode(const struct microcode_patch *patch) > { > unsigned long flags; > @@ -379,47 +360,47 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > { > long offset = 0; > int error = 0; > - void *mc; > + struct microcode_intel *mc, *saved = NULL; > struct microcode_patch *patch = NULL; > > - while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 ) > + while ( (offset = get_next_ucode_from_buffer((void **)&mc, buf, > + size, offset)) > 0 ) > { > - struct microcode_patch *new_patch; > - > error = microcode_sanity_check(mc); > if ( error ) > - break; > - > - new_patch = alloc_microcode_patch(mc); > - if ( IS_ERR(new_patch) ) > { > - error = PTR_ERR(new_patch); > + xfree(mc); > break; > } > > /* > - * If the new patch covers current CPU, compare patches and store the > + * If the new update covers current CPU, compare updates and store the > * one with higher revision. > */ > - if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) && > - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) > + if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) && > + (!saved || (mc->hdr.rev > saved->hdr.rev)) ) > { > - struct microcode_patch *tmp = patch; > - > - patch = new_patch; > - new_patch = tmp; > + xfree(saved); > + saved = mc; > } > - > - if ( new_patch ) > - microcode_free_patch(new_patch); > - > - xfree(mc); > + else > + xfree(mc); > } > - if ( offset > 0 ) > - xfree(mc); > if ( offset < 0 ) > error = offset; > > + if ( saved ) > + { > + patch = xmalloc(struct microcode_patch); > + if ( patch ) > + patch->mc_intel = saved; > + else > + { > + xfree(saved); > + error = -ENOMEM; > + } > + } > + The above code looks suspiciously similar to the AMD cpu_request_microcode counterpart, which makes me think it could be further abstracted in order to reduce this duplication. In any case, this can be done in a follow up patch. Thanks, Roger.
On Fri, Aug 23, 2019 at 10:11:21AM +0200, Roger Pau Monné wrote: >On Mon, Aug 19, 2019 at 09:25:25AM +0800, Chao Gao wrote: >> To create a microcode patch from a vendor-specific update, >> allocate_microcode_patch() copied everything from the update. >> It is not efficient. Essentially, we just need to go through >> ucodes in the blob, find the one with the newest revision and >> install it into the microcode_patch. In the process, buffers >> like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel >> side) can be reused. microcode_patch now is allocated after >> it is sure that there is a matching ucode. > >Oh, I think this answers my question on a previous patch. > >For future series it would be nice to avoid so many rewrites in the >same series, alloc_microcode_patch is already modified in a previous >patch, just to be removed here. It also makes it harder to follow >what's going on. Got it. This patch is added in this new version. And some trivial patches already got reviewed-by. So I don't merge it with them. >> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> &offset)) == 0 ) >> { >> - struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); >> - >> - if ( IS_ERR(new_patch) ) >> - { >> - error = PTR_ERR(new_patch); >> - break; >> - } >> - >> /* >> - * If the new patch covers current CPU, compare patches and store the >> + * If the new ucode covers current CPU, compare ucodes and store the >> * one with higher revision. >> */ >> - if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) && >> - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) >> +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id) >> + if ( (microcode_fits(mc_amd) != MIS_UCODE) && >> + (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) ) >> +#undef REV_ID >> { >> - struct microcode_patch *tmp = patch; >> - >> - patch = new_patch; >> - new_patch = tmp; >> + xfree(saved); >> + saved = mc_amd->mpb; >> + saved_size = mc_amd->mpb_size; >> } >> - >> - if ( new_patch ) >> - microcode_free_patch(new_patch); >> + else >> + xfree(mc_amd->mpb); It might be better to move 'mc_amd->mpb = NULL' here. >> >> if ( offset >= bufsize ) >> break; >> @@ -593,9 +548,25 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, >> *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >> break; >> } >> - xfree(mc_amd->mpb); >> - xfree(mc_amd->equiv_cpu_table); >> - xfree(mc_amd); >> + >> + if ( saved ) >> + { >> + mc_amd->mpb = saved; >> + mc_amd->mpb_size = saved_size; >> + patch = xmalloc(struct microcode_patch); >> + if ( patch ) >> + patch->mc_amd = mc_amd; >> + else >> + { >> + free_patch(mc_amd); >> + error = -ENOMEM; >> + } >> + } >> + else >> + { >> + mc_amd->mpb = NULL; > >What's the point in setting mpb to NULL if you are just going to free >mc_amd below? To avoid double free here. mc_amd->mpb is always freed or saved. And here we want to free mc_amd itself and mc_amd->equiv_cpu_table. > >Also, I'm not sure I understand why you need to free mc_amd, isn't >this buff memory that should be freed by the caller? But mc_amd is allocated in this function. > >ie: in the Intel counterpart below you don't seem to free the mc >cursor used for the get_next_ucode_from_buffer loop. 'mc' is saved if it is newer than current patch stored in 'saved'. Otherwise 'mc' is freed immediately. So we don't need to free it again after the while loop. Thanks Chao
On Mon, Aug 26, 2019 at 03:03:22PM +0800, Chao Gao wrote: > On Fri, Aug 23, 2019 at 10:11:21AM +0200, Roger Pau Monné wrote: > >On Mon, Aug 19, 2019 at 09:25:25AM +0800, Chao Gao wrote: > >> To create a microcode patch from a vendor-specific update, > >> allocate_microcode_patch() copied everything from the update. > >> It is not efficient. Essentially, we just need to go through > >> ucodes in the blob, find the one with the newest revision and > >> install it into the microcode_patch. In the process, buffers > >> like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel > >> side) can be reused. microcode_patch now is allocated after > >> it is sure that there is a matching ucode. > > > >Oh, I think this answers my question on a previous patch. > > > >For future series it would be nice to avoid so many rewrites in the > >same series, alloc_microcode_patch is already modified in a previous > >patch, just to be removed here. It also makes it harder to follow > >what's going on. > > Got it. This patch is added in this new version. And some trivial > patches already got reviewed-by. So I don't merge it with them. > > >> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > >> &offset)) == 0 ) > >> { > >> - struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); > >> - > >> - if ( IS_ERR(new_patch) ) > >> - { > >> - error = PTR_ERR(new_patch); > >> - break; > >> - } > >> - > >> /* > >> - * If the new patch covers current CPU, compare patches and store the > >> + * If the new ucode covers current CPU, compare ucodes and store the > >> * one with higher revision. > >> */ > >> - if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) && > >> - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) > >> +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id) > >> + if ( (microcode_fits(mc_amd) != MIS_UCODE) && > >> + (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) ) > >> +#undef REV_ID > >> { > >> - struct microcode_patch *tmp = patch; > >> - > >> - patch = new_patch; > >> - new_patch = tmp; > >> + xfree(saved); > >> + saved = mc_amd->mpb; > >> + saved_size = mc_amd->mpb_size; > >> } > >> - > >> - if ( new_patch ) > >> - microcode_free_patch(new_patch); > >> + else > >> + xfree(mc_amd->mpb); > > It might be better to move 'mc_amd->mpb = NULL' here. > > >> > >> if ( offset >= bufsize ) > >> break; > >> @@ -593,9 +548,25 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > >> *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) > >> break; > >> } > >> - xfree(mc_amd->mpb); > >> - xfree(mc_amd->equiv_cpu_table); > >> - xfree(mc_amd); > >> + > >> + if ( saved ) > >> + { > >> + mc_amd->mpb = saved; > >> + mc_amd->mpb_size = saved_size; > >> + patch = xmalloc(struct microcode_patch); > >> + if ( patch ) > >> + patch->mc_amd = mc_amd; > >> + else > >> + { > >> + free_patch(mc_amd); > >> + error = -ENOMEM; > >> + } > >> + } > >> + else > >> + { > >> + mc_amd->mpb = NULL; > > > >What's the point in setting mpb to NULL if you are just going to free > >mc_amd below? > > To avoid double free here. mc_amd->mpb is always freed or saved. > And here we want to free mc_amd itself and mc_amd->equiv_cpu_table. But there's no chance of a double free here, since you are freeing mc_amd in the line below after setting mpb = NULL. I think it would make sense to set mpb = NULL after freeing it inside the loop. With that you can add my: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > >Also, I'm not sure I understand why you need to free mc_amd, isn't > >this buff memory that should be freed by the caller? > > But mc_amd is allocated in this function. > > > > >ie: in the Intel counterpart below you don't seem to free the mc > >cursor used for the get_next_ucode_from_buffer loop. > > 'mc' is saved if it is newer than current patch stored in 'saved'. > Otherwise 'mc' is freed immediately. So we don't need to free it > again after the while loop. Ack, thanks!
On 19.08.2019 03:25, Chao Gao wrote: > @@ -542,29 +505,21 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > &offset)) == 0 ) > { > - struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); > - > - if ( IS_ERR(new_patch) ) > - { > - error = PTR_ERR(new_patch); > - break; > - } > - > /* > - * If the new patch covers current CPU, compare patches and store the > + * If the new ucode covers current CPU, compare ucodes and store the > * one with higher revision. > */ > - if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) && > - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) > +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id) > + if ( (microcode_fits(mc_amd) != MIS_UCODE) && > + (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) ) > +#undef REV_ID I'm not happy with this helper #define, the more that "saved" already is of the correct type. compare_patch() in reality only acts on the header, so I'd suggest having that function forward to a new compare_header() (or some other suitable name) and use that new function here as well. > @@ -379,47 +360,47 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, > { > long offset = 0; > int error = 0; > - void *mc; > + struct microcode_intel *mc, *saved = NULL; > struct microcode_patch *patch = NULL; > > - while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 ) > + while ( (offset = get_next_ucode_from_buffer((void **)&mc, buf, Casts like this make me rather nervous. Please see about getting rid of it (by using a union or a 2nd local variable). Jan
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c index 6353323..ec1c2eb 100644 --- a/xen/arch/x86/microcode_amd.c +++ b/xen/arch/x86/microcode_amd.c @@ -194,36 +194,6 @@ static bool match_cpu(const struct microcode_patch *patch) return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE); } -static struct microcode_patch *alloc_microcode_patch( - const struct microcode_amd *mc_amd) -{ - struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch); - struct microcode_amd *cache = xmalloc(struct microcode_amd); - void *mpb = xmalloc_bytes(mc_amd->mpb_size); - struct equiv_cpu_entry *equiv_cpu_table = - xmalloc_bytes(mc_amd->equiv_cpu_table_size); - - if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table ) - { - xfree(microcode_patch); - xfree(cache); - xfree(mpb); - xfree(equiv_cpu_table); - return ERR_PTR(-ENOMEM); - } - - memcpy(mpb, mc_amd->mpb, mc_amd->mpb_size); - cache->mpb = mpb; - cache->mpb_size = mc_amd->mpb_size; - memcpy(equiv_cpu_table, mc_amd->equiv_cpu_table, - mc_amd->equiv_cpu_table_size); - cache->equiv_cpu_table = equiv_cpu_table; - cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size; - microcode_patch->mc_amd = cache; - - return microcode_patch; -} - static void free_patch(void *mc) { struct microcode_amd *mc_amd = mc; @@ -320,18 +290,10 @@ static int get_ucode_from_buffer_amd( return -EINVAL; } - if ( mc_amd->mpb_size < mpbuf->len ) - { - if ( mc_amd->mpb ) - { - xfree(mc_amd->mpb); - mc_amd->mpb_size = 0; - } - mc_amd->mpb = xmalloc_bytes(mpbuf->len); - if ( mc_amd->mpb == NULL ) - return -ENOMEM; - mc_amd->mpb_size = mpbuf->len; - } + mc_amd->mpb = xmalloc_bytes(mpbuf->len); + if ( mc_amd->mpb == NULL ) + return -ENOMEM; + mc_amd->mpb_size = mpbuf->len; memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len); pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", @@ -451,8 +413,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t bufsize) { struct microcode_amd *mc_amd; + struct microcode_header_amd *saved = NULL; struct microcode_patch *patch = NULL; - size_t offset = 0; + size_t offset = 0, saved_size = 0; int error = 0; unsigned int current_cpu_id; unsigned int equiv_cpu_id; @@ -542,29 +505,21 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, &offset)) == 0 ) { - struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); - - if ( IS_ERR(new_patch) ) - { - error = PTR_ERR(new_patch); - break; - } - /* - * If the new patch covers current CPU, compare patches and store the + * If the new ucode covers current CPU, compare ucodes and store the * one with higher revision. */ - if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) && - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id) + if ( (microcode_fits(mc_amd) != MIS_UCODE) && + (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) ) +#undef REV_ID { - struct microcode_patch *tmp = patch; - - patch = new_patch; - new_patch = tmp; + xfree(saved); + saved = mc_amd->mpb; + saved_size = mc_amd->mpb_size; } - - if ( new_patch ) - microcode_free_patch(new_patch); + else + xfree(mc_amd->mpb); if ( offset >= bufsize ) break; @@ -593,9 +548,25 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) break; } - xfree(mc_amd->mpb); - xfree(mc_amd->equiv_cpu_table); - xfree(mc_amd); + + if ( saved ) + { + mc_amd->mpb = saved; + mc_amd->mpb_size = saved_size; + patch = xmalloc(struct microcode_patch); + if ( patch ) + patch->mc_amd = mc_amd; + else + { + free_patch(mc_amd); + error = -ENOMEM; + } + } + else + { + mc_amd->mpb = NULL; + free_patch(mc_amd); + } out: if ( error && !patch ) diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c index 96b38f8..ae5759f 100644 --- a/xen/arch/x86/microcode_intel.c +++ b/xen/arch/x86/microcode_intel.c @@ -282,25 +282,6 @@ static enum microcode_match_result compare_patch( OLD_UCODE; } -static struct microcode_patch *alloc_microcode_patch( - const struct microcode_header_intel *mc_header) -{ - unsigned long total_size = get_totalsize(mc_header); - void *new_mc = xmalloc_bytes(total_size); - struct microcode_patch *new_patch = xmalloc(struct microcode_patch); - - if ( !new_patch || !new_mc ) - { - xfree(new_patch); - xfree(new_mc); - return ERR_PTR(-ENOMEM); - } - memcpy(new_mc, mc_header, total_size); - new_patch->mc_intel = new_mc; - - return new_patch; -} - static int apply_microcode(const struct microcode_patch *patch) { unsigned long flags; @@ -379,47 +360,47 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, { long offset = 0; int error = 0; - void *mc; + struct microcode_intel *mc, *saved = NULL; struct microcode_patch *patch = NULL; - while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 ) + while ( (offset = get_next_ucode_from_buffer((void **)&mc, buf, + size, offset)) > 0 ) { - struct microcode_patch *new_patch; - error = microcode_sanity_check(mc); if ( error ) - break; - - new_patch = alloc_microcode_patch(mc); - if ( IS_ERR(new_patch) ) { - error = PTR_ERR(new_patch); + xfree(mc); break; } /* - * If the new patch covers current CPU, compare patches and store the + * If the new update covers current CPU, compare updates and store the * one with higher revision. */ - if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) && - (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) ) + if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) && + (!saved || (mc->hdr.rev > saved->hdr.rev)) ) { - struct microcode_patch *tmp = patch; - - patch = new_patch; - new_patch = tmp; + xfree(saved); + saved = mc; } - - if ( new_patch ) - microcode_free_patch(new_patch); - - xfree(mc); + else + xfree(mc); } - if ( offset > 0 ) - xfree(mc); if ( offset < 0 ) error = offset; + if ( saved ) + { + patch = xmalloc(struct microcode_patch); + if ( patch ) + patch->mc_intel = saved; + else + { + xfree(saved); + error = -ENOMEM; + } + } + if ( error && !patch ) patch = ERR_PTR(error);
To create a microcode patch from a vendor-specific update, allocate_microcode_patch() copied everything from the update. It is not efficient. Essentially, we just need to go through ucodes in the blob, find the one with the newest revision and install it into the microcode_patch. In the process, buffers like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel side) can be reused. microcode_patch now is allocated after it is sure that there is a matching ucode. Signed-off-by: Chao Gao <chao.gao@intel.com> --- Changes in v9: - new --- xen/arch/x86/microcode_amd.c | 99 +++++++++++++++--------------------------- xen/arch/x86/microcode_intel.c | 65 ++++++++++----------------- 2 files changed, 58 insertions(+), 106 deletions(-)