Re: [Xen-devel] [PATCH 2/4] xen/x86: Drop erronious barriers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/12/16 11:18, Jan Beulich wrote:
>>>> On 05.12.16 at 11:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> As to the title - don't you rather mean "pointless" or some such? It's
> not really an error to have them where they are.

True.  I will update.

>
>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -146,9 +146,6 @@ static void nmi_shootdown_cpus(void)
>>      write_atomic((unsigned long *)__va(__pa(&exception_table[TRAP_nmi])),
>>                   (unsigned long)&do_nmi_crash);
>>  
>> -    /* Ensure the new callback function is set before sending out the NMI. 
>> */
>> -    wmb();
>> -
>>      smp_send_nmi_allbutself();
> I don't think I agree with this change - we certainly want to make
> sure the APIC write happens only after after the exception vector
> adjustment became visible, namely when in x2APIC mode (where
> the respective WRMSRs are not serializing).

This wmb() is already only a barrier() (Fixed in the final patch)

Even if it weren't, wrmsr doesn't interact with sfence, so the barrier
would still be pointless.

>
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -346,7 +346,6 @@ void start_secondary(void *unused)
>>      spin_debug_enable();
>>      set_cpu_sibling_map(cpu);
>>      notify_cpu_starting(cpu);
>> -    wmb();
>>  
>>      /*
>>       * We need to hold vector_lock so there the set of online cpus
> Hmm, this one is indeed redundant with the lock_vector_lock()
> following right below, but if that lock wasn't there, I think it
> would be needed to order set_cpu_sibling_map() and the
> setting of the bit in the online map. So I think it would better
> stay (but be relaxed to smb_wmb()).

Why?  It doesn't relate to an smp_rmb() on any other CPU, and is
therefore wrong to use.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel