Re: [Xen-devel] [PATCH v10 12/16] x86/microcode: Synchronize late microcode loading

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2019 09:22, Chao Gao wrote:
> @@ -264,38 +336,158 @@ static int microcode_update_cpu(const struct 
> microcode_patch *patch)
>      return err;
>  }
>  
> -static long do_microcode_update(void *patch)
> +static bool wait_for_state(unsigned int state)
> +{
> +    while ( loading_state != state )
> +    {
> +        if ( state != LOADING_EXIT && loading_state == LOADING_EXIT )
> +            return false;

This is at least somewhat confusing: There's no indication here
that "loading_state" may change behind the function's back. So
in general one could be (and I initially was) tempted to suggest
dropping the apparently redundant left side of the &&. But that
would end up wrong if the compiler translates the above to two
separate reads of "loading_state". Therefore I'd like to suggest

static bool wait_for_state(typeof(loading_state) state)
{
    typeof(loading_state) cur_state;

    while ( (cur_state = ACCESS_ONCE(loading_state)) != state )
    {
        if ( cur_state == LOADING_EXIT )
            return false;
        cpu_relax();
    }

    return true;
}

or something substantially similar (if, e.g., you dislike the
use of typeof() here).

> +static int secondary_thread_fn(void)
> +{
> +    unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
> +
> +    if ( !wait_for_state(LOADING_CALLIN) )
> +        return -EBUSY;
> +
> +    cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
> +
> +    if ( !wait_for_state(LOADING_EXIT) )
> +        return -EBUSY;

This return looks to be unreachable, doesn't it?

> +static int control_thread_fn(const struct microcode_patch *patch)
>  {
> -    unsigned int cpu;
> -    int ret = microcode_update_cpu(patch);
> +    unsigned int cpu = smp_processor_id(), done;
> +    unsigned long tick;
> +    int ret;
> +
> +    /*
> +     * We intend to disable interrupt for long time, which may lead to
> +     * watchdog timeout.
> +     */
> +    watchdog_disable();

With the move here, the comment should start "We intend to keep
interrupts disabled for a long time, ..." - they are disabled
already at this point.

> -    /* Store the patch after a successful loading */
> -    if ( !ret && patch )
> +    /* Allow threads to call in */
> +    set_state(LOADING_CALLIN);
> +
> +    cpumask_set_cpu(cpu, &cpu_callin_map);
> +
> +    /* Waiting for all threads calling in */
> +    ret = wait_for_condition(wait_cpu_callin,
> +                             (void *)(unsigned long)num_online_cpus(),

I'm puzzled by my own suggestion in reply to v9, and I'm equally
puzzled that you didn't take it that little step further: All of
this casting would be unnecessary if the two wait_cpu_*()
functions took "unsigned int" as their parameters. (The
observation with v9 was that both are similar enough to allow
for a common signature, even if that signature would not be a
typical one for a callback.)

> +                             MICROCODE_CALLIN_TIMEOUT_US);
> +    if ( ret )
>      {
> -        spin_lock(&microcode_mutex);
> -        microcode_update_cache(patch);
> -        spin_unlock(&microcode_mutex);
> -        patch = NULL;
> +        set_state(LOADING_EXIT);
> +        return ret;
>      }
>  
> -    if ( microcode_ops->end_update_percpu )
> -        microcode_ops->end_update_percpu();
> +    /* Let primary threads load the given ucode update */
> +    set_state(LOADING_ENTER);
>  
> +    ret = microcode_ops->apply_microcode(patch);
> +    if ( !ret )
> +        atomic_inc(&cpu_updated);
> +    atomic_inc(&cpu_out);
> +
> +    tick = rdtsc_ordered();
> +    /* Wait for primary threads finishing update */
> +    done = atomic_read(&cpu_out);

Would you mind eliminating the duplication of this and ...

> +    while ( done != nr_cores )
> +    {
> +        /*
> +         * During each timeout interval, at least a CPU is expected to
> +         * finish its update. Otherwise, something goes wrong.
> +         *
> +         * Note that RDTSC (in wait_for_condition()) is safe for threads to
> +         * execute while waiting for completion of loading an update.
> +         */
> +        if ( wait_for_condition(wait_cpu_callout,
> +                                (void *)(unsigned long)(done + 1),
> +                                MICROCODE_UPDATE_TIMEOUT_US) )
> +            panic("Timeout when finished updating microcode (finished 
> %u/%u)",
> +                  done, nr_cores);
> +
> +        /* Print warning message once if long time is spent here */
> +        if ( tick && rdtsc_ordered() - tick >= cpu_khz * 1000 )
> +        {
> +            printk(XENLOG_WARNING
> +                   "WARNING: UPDATING MICROCODE HAS CONSUMED MORE THAN 1 
> SECOND!\n");
> +            tick = 0;
> +        }
> +        done = atomic_read(&cpu_out);

... this, by doing the assignment inside the while()?

Jan

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