xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Chao Gao <chao.gao@intel.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Sergey Dyasli" <sergey.dyasli@citrix.com>,
	"Ashok Raj" <ashok.raj@intel.com>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v10 09/16] microcode: split out apply_microcode() from cpu_request_microcode()
Date: Fri, 13 Sep 2019 14:47:34 +0800	[thread overview]
Message-ID: <20190913064732.GA14305@gao-cwp> (raw)
In-Reply-To: <3877cba9-4707-5fe9-3224-74f825545e1b@suse.com>

On Thu, Sep 12, 2019 at 04:07:16PM +0200, Jan Beulich wrote:
>On 12.09.2019 09:22, Chao Gao wrote:
>> @@ -249,49 +249,80 @@ bool microcode_update_cache(struct microcode_patch *patch)
>>      return true;
>>  }
>>  
>> -static int microcode_update_cpu(const void *buf, size_t size)
>> +/*
>> + * Load a microcode update to current CPU.
>> + *
>> + * If no patch is provided, the cached patch will be loaded. Microcode update
>> + * during APs bringup and CPU resuming falls into this case.
>> + */
>> +static int microcode_update_cpu(const struct microcode_patch *patch)
>>  {
>> -    int err;
>> -    unsigned int cpu = smp_processor_id();
>> -    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>> +    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>>  
>> -    spin_lock(&microcode_mutex);
>> +    if ( unlikely(err) )
>> +        return err;
>>  
>> -    err = microcode_ops->collect_cpu_info(sig);
>> -    if ( likely(!err) )
>> -        err = microcode_ops->cpu_request_microcode(buf, size);
>> -    spin_unlock(&microcode_mutex);
>> +    if ( patch )
>> +        err = microcode_ops->apply_microcode(patch);
>> +    else if ( microcode_cache )
>> +    {
>> +        spin_lock(&microcode_mutex);
>> +        err = microcode_ops->apply_microcode(microcode_cache);
>> +        if ( err == -EIO )
>> +        {
>> +            microcode_free_patch(microcode_cache);
>> +            microcode_cache = NULL;
>> +        }
>> +        spin_unlock(&microcode_mutex);
>> +    }
>
>I'm having trouble understanding the locking discipline here: Why
>do you call ->apply_microcode() once with the lock held and once
>without? If this is to guard against microcode_cache changing,

Yes. microcode_cache is protected by microcode_mutex;

>then (a) the check of it being non-NULL would need to be done with
>the lock held as well and

Will do.

>(b) you'd need to explain why the non-
>locked call to ->apply_microcode() is okay.

->apply_microcode() was always called with this lock held was because
it always read the old per-cpu cache which was protected by the lock.
It gave us an impression that ->apply_microcode() was protected by the
lock.

The patch before this one makes ->apply_microcode() accept a patch
pointer. With this change, if the patch being passed should be accessed
with some lock held (like the secondary call site above), we acquire
the lock. Otherwise, no lock is taken and the caller of
microcode_update_cpu() is supposed to guarantee the patch won't be
changed by others.

>
>It certainly wasn't this way in v8, yet the v9 revision log also
>doesn't mention such a (not insignificant) change (which is part
>of the reason why I didn't spot it in v9).

It is my bad.

>
>> +    else
>> +        /* No patch to update */
>> +        err = -ENOENT;
>>  
>>      return err;
>>  }
>>  
>> -static long do_microcode_update(void *_info)
>> +static long do_microcode_update(void *patch)
>>  {
>> -    struct microcode_info *info = _info;
>> -    int error;
>> -
>> -    BUG_ON(info->cpu != smp_processor_id());
>> +    unsigned int cpu;
>> +    int ret = microcode_update_cpu(patch);
>>  
>> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
>> -    if ( error )
>> -        info->error = error;
>> +    /* Store the patch after a successful loading */
>> +    if ( !ret && patch )
>> +    {
>> +        spin_lock(&microcode_mutex);
>> +        microcode_update_cache(patch);
>> +        spin_unlock(&microcode_mutex);
>> +        patch = NULL;
>> +    }
>>  
>>      if ( microcode_ops->end_update_percpu )
>>          microcode_ops->end_update_percpu();
>>  
>> -    info->cpu = cpumask_next(info->cpu, &cpu_online_map);
>> -    if ( info->cpu < nr_cpu_ids )
>> -        return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>> +    /*
>> +     * Each thread tries to load ucode and only the first thread of a core
>> +     * would succeed. Ignore error other than -EIO.
>> +     */
>> +    if ( ret != -EIO )
>> +        ret = 0;
>
>I don't think this is a good idea. Ignoring a _specific_ error
>code (e.g. indicating "already loaded" or "newer patch already
>loaded") is fine, but here you also ignore things like -ENOMEM
>or -EINVAL.

will do.

>
>> +    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
>> +    if ( cpu < nr_cpu_ids )
>> +        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?
>> +                                                                          : ret;
>
>When there's no middle operand I don't think ? and : should be on
>separate lines.
>
>> @@ -299,32 +330,46 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>      if ( microcode_ops == NULL )
>>          return -EINVAL;
>>  
>> -    info = xmalloc_bytes(sizeof(*info) + len);
>> -    if ( info == NULL )
>> +    buffer = xmalloc_bytes(len);
>> +    if ( !buffer )
>>          return -ENOMEM;
>>  
>> -    ret = copy_from_guest(info->buffer, buf, len);
>> -    if ( ret != 0 )
>> +    if ( copy_from_guest(buffer, buf, len) )
>> +    {
>> +        ret = -EFAULT;
>> +        goto free;
>> +    }
>> +
>> +    patch = parse_blob(buffer, len);
>
>You don't look to be using buffer anymore below this point. Why don't
>you free it right here, avoiding the need for the "free" label below
>and also further reducing the overall code churn as it seems.

Yes. Good idea.

Thanks
Chao

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

  reply	other threads:[~2019-09-13  6:43 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12  7:22 [Xen-devel] [PATCH v10 00/16] improve late microcode loading Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 01/16] microcode/intel: extend microcode_update_match() Chao Gao
2019-09-12 10:24   ` Jan Beulich
2019-09-13  6:50     ` Jan Beulich
2019-09-13  7:02       ` Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 02/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 03/16] microcode: introduce a global cache of ucode patch Chao Gao
2019-09-12 10:29   ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 04/16] microcode: clean up microcode_resume_cpu Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 05/16] microcode: remove struct ucode_cpu_info Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 06/16] microcode: remove pointless 'cpu' parameter Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 07/16] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
2019-09-12 12:34   ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 08/16] microcode: pass a patch pointer to apply_microcode() Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 09/16] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
2019-09-12 14:07   ` Jan Beulich
2019-09-13  6:47     ` Chao Gao [this message]
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 10/16] microcode: unify ucode loading during system bootup and resuming Chao Gao
2019-09-12 14:59   ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 11/16] microcode: reduce memory allocation and copy when creating a patch Chao Gao
2019-09-12 15:04   ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 12/16] x86/microcode: Synchronize late microcode loading Chao Gao
2019-09-12 15:32   ` Jan Beulich
2019-09-13  7:01     ` Chao Gao
2019-09-13  7:15       ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 13/16] microcode: remove microcode_update_lock Chao Gao
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 14/16] microcode: rendezvous CPUs in NMI handler and load ucode Chao Gao
2019-09-13  9:14   ` Jan Beulich
2019-09-16  3:18     ` Chao Gao
2019-09-16  8:22       ` Jan Beulich
2019-09-13  9:18   ` Jan Beulich
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 15/16] microcode: disable late loading if CPUs are affected by BDF90 Chao Gao
2019-09-13  9:22   ` Jan Beulich
2019-09-17  9:01     ` Chao Gao
2019-09-17 10:49       ` Jan Beulich
2019-09-13 12:23   ` Andrew Cooper
2019-09-12  7:22 ` [Xen-devel] [PATCH v10 16/16] microcode/intel: writeback and invalidate cache conditionally Chao Gao
2019-09-13  9:32   ` Jan Beulich
2019-09-13  8:47 ` [Xen-devel] [PATCH v10 00/16] improve late microcode loading Jan Beulich
2019-09-17  7:09   ` Chao Gao
2019-09-17  7:11     ` Jan Beulich

Reply instructions:

You may reply publicly 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=20190913064732.GA14305@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=sergey.dyasli@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).