Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Chao Gao <chao.gao@intel.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org, Jan Beulich <jbeulich@suse.com>,
	Ashok Raj <ashok.raj@intel.com>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch
Date: Mon, 26 Aug 2019 15:03:22 +0800
Message-ID: <20190826070320.GA11910@gao-cwp> (raw)
In-Reply-To: <20190823081121.khm5622trozzhswe@Air-de-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

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

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-19  1:25 [Xen-devel] [PATCH v9 00/15] improve late microcode loading Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 01/15] microcode/intel: extend microcode_update_match() Chao Gao
2019-08-28 15:12   ` Jan Beulich
2019-08-29  7:15     ` Chao Gao
2019-08-29  7:14       ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 02/15] microcode/amd: fix memory leak Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 03/15] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 04/15] microcode: introduce a global cache of ucode patch Chao Gao
2019-08-22 11:11   ` Roger Pau Monné
2019-08-28 15:21   ` Jan Beulich
2019-08-29 10:18   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 05/15] microcode: clean up microcode_resume_cpu Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 06/15] microcode: remove struct ucode_cpu_info Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 07/15] microcode: remove pointless 'cpu' parameter Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 08/15] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
2019-08-22 13:08   ` Roger Pau Monné
2019-08-28 15:26   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 09/15] microcode: pass a patch pointer to apply_microcode() Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 10/15] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
2019-08-22 13:59   ` Roger Pau Monné
2019-08-29 10:06     ` Jan Beulich
2019-08-30  3:22       ` Chao Gao
2019-08-30  7:25         ` Jan Beulich
2019-08-29 10:19   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 11/15] microcode: unify loading update during CPU resuming and AP wakeup Chao Gao
2019-08-22 14:10   ` Roger Pau Monné
2019-08-22 16:44     ` Chao Gao
2019-08-23  9:09       ` Roger Pau Monné
2019-08-29  7:37         ` Chao Gao
2019-08-29  8:16           ` Roger Pau Monné
2019-08-29 10:26           ` Jan Beulich
2019-08-29 10:29   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch Chao Gao
2019-08-23  8:11   ` Roger Pau Monné
2019-08-26  7:03     ` Chao Gao [this message]
2019-08-26  8:11       ` Roger Pau Monné
2019-08-29 10:47   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 13/15] x86/microcode: Synchronize late microcode loading Chao Gao
2019-08-19 10:27   ` Sergey Dyasli
2019-08-19 14:49     ` Chao Gao
2019-08-29 12:06   ` Jan Beulich
2019-08-30  3:30     ` Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 14/15] microcode: remove microcode_update_lock Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 15/15] microcode: block #NMI handling when loading an ucode Chao Gao
2019-08-23  8:46   ` Sergey Dyasli
2019-08-26  8:07     ` Chao Gao
2019-08-27  4:52       ` Chao Gao
2019-08-28  8:52         ` Sergey Dyasli
2019-08-29 12:11         ` Jan Beulich
2019-08-30  6:35           ` Chao Gao
2019-09-09  5:52             ` Chao Gao
2019-09-09  6:16               ` Jan Beulich
2019-08-29 12:22   ` Jan Beulich
2019-08-30  6:33     ` Chao Gao
2019-08-30  7:30       ` Jan Beulich
2019-08-22  7:51 ` [Xen-devel] [PATCH v9 00/15] improve late microcode loading Sergey Dyasli
2019-08-22 15:39   ` Chao Gao

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190826070320.GA11910@gao-cwp \
    --to=chao.gao@intel.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=ashok.raj@intel.com \
    --cc=jbeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git