Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch
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,
> )) == 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(, buf, size, offset)) >
> 0 )
> +while ( (offset = get_next_ucode_from_buffer((void **), 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch
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,
> >> )) == 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é
> >
> >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!
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch
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,
>> )) == 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
Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch
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
> ---
> 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,
> )) == 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
> {
> -
