Re: git: e769bc771843 - main - sym(4): Employ memory barriers also on x86

On 26 Jan 2026, at 16:34, Konstantin Belousov <kostikbel@gmail.com> wrote:

> On Mon, Jan 26, 2026 at 03:57:45PM +0000, Marius Strobl wrote:
>> The branch main has been updated by marius:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=e769bc77184312b6137a9b180c97b87c0760b849
>> 
>> commit e769bc77184312b6137a9b180c97b87c0760b849
>> Author:     Marius Strobl <marius@FreeBSD.org>
>> AuthorDate: 2026-01-26 13:58:57 +0000
>> Commit:     Marius Strobl <marius@FreeBSD.org>
>> CommitDate: 2026-01-26 15:54:48 +0000
>> 
>>    sym(4): Employ memory barriers also on x86
>> 
>>    In an MP world, it doesn't hold that x86 requires no memory barriers.
> It does hold.  x86 is much more strongly ordered than all other arches
> we currently support.
> 
> That said, the use of the barriers in drivers is usually not justified
> (I did not looked at this specific case).
> 
> Even if needed, please stop using rmb/wmb etc.  Use atomic_thread_fence()
> of appropriate kind, see atomic(9).  Then on x86 it does the right thing.

atomic_thread_fence() isn’t appropriate when dealing with MMIO devices.

Ideally all this would use bus_space_* and that would handle everything.

Jessica

>>    This change should also fix panics due to out-of-sync data seen with
>>    FreeBSD VMs on top of OpenStack and HBAs of type lsiLogic. [1]
>> 
>>    While at it:
>>    - Improve the granularity somewhat by distinguishing between read and
>>      write memory barriers as well as refer to existing *mb(9) functions
>>      instead of duplicating these [2], unless IO barriers are also used.
>>    - Nuke the unused SYM_DRIVER_NAME macro.
>> 
>>    PR:             270816 [1]
>>    Obtained from:  BSD-licensed Linux sym53c8xx driver [2]
>>    MFC after:      1 week
>> ---
>> sys/dev/sym/sym_hipd.c | 42 +++++++++++++++---------------------------
>> 1 file changed, 15 insertions(+), 27 deletions(-)
>> 
>> diff --git a/sys/dev/sym/sym_hipd.c b/sys/dev/sym/sym_hipd.c
>> index 0e51607fb07a..f78d595a73ce 100644
>> --- a/sys/dev/sym/sym_hipd.c
>> +++ b/sys/dev/sym/sym_hipd.c
>> @@ -58,7 +58,6 @@
>>  */
>> 
>> #include <sys/cdefs.h>
>> -#define SYM_DRIVER_NAME "sym-1.6.5-20000902"
>> 
>> /* #define SYM_DEBUG_GENERIC_SUPPORT */
>> 
>> @@ -114,27 +113,16 @@ typedef u_int32_t u32;
>> #include <dev/sym/sym_fw.h>
>> 
>> /*
>> - *  IA32 architecture does not reorder STORES and prevents
>> - *  LOADS from passing STORES. It is called `program order'
>> - *  by Intel and allows device drivers to deal with memory
>> - *  ordering by only ensuring that the code is not reordered
>> - *  by the compiler when ordering is required.
>> - *  Other architectures implement a weaker ordering that
>> - *  requires memory barriers (and also IO barriers when they
>> - *  make sense) to be used.
>> - */
>> -#if defined __i386__ || defined __amd64__
>> -#define MEMORY_BARRIER() do { ; } while(0)
>> -#elif defined __powerpc__
>> -#define MEMORY_BARRIER() __asm__ volatile("eieio; sync" : : : "memory")
>> -#elif defined __arm__
>> -#define MEMORY_BARRIER() dmb()
>> -#elif defined __aarch64__
>> -#define MEMORY_BARRIER() dmb(sy)
>> -#elif defined __riscv
>> -#define MEMORY_BARRIER() fence()
>> + *  Architectures may implement weak ordering that requires memory barriers
>> + *  to be used for LOADS and STORES to become globally visible (and also IO
>> + *  barriers when they make sense).
>> + */
>> +#ifdef __powerpc__
>> +#define MEMORY_READ_BARRIER() __asm__ volatile("eieio; sync" : : : "memory")
>> +#define MEMORY_WRITE_BARRIER() MEMORY_READ_BARRIER()
>> #else
>> -#error "Not supported platform"
>> +#define MEMORY_READ_BARRIER() rmb()
>> +#define MEMORY_WRITE_BARRIER() wmb()
>> #endif
>> 
>> /*
>> @@ -892,13 +880,13 @@ struct sym_nvram {
>>  */
>> #define OUTL_DSP(v) \
>> do { \
>> - MEMORY_BARRIER(); \
>> + MEMORY_WRITE_BARRIER(); \
>> OUTL (nc_dsp, (v)); \
>> } while (0)
>> 
>> #define OUTONB_STD() \
>> do { \
>> - MEMORY_BARRIER(); \
>> + MEMORY_WRITE_BARRIER(); \
>> OUTONB (nc_dcntl, (STD|NOCOM)); \
>> } while (0)
>> 
>> @@ -2908,7 +2896,7 @@ static void sym_put_start_queue(hcb_p np, ccb_p cp)
>> if (qidx >= MAX_QUEUE*2) qidx = 0;
>> 
>> np->squeue [qidx]   = cpu_to_scr(np->idletask_ba);
>> - MEMORY_BARRIER();
>> + MEMORY_WRITE_BARRIER();
>> np->squeue [np->squeueput] = cpu_to_scr(cp->ccb_ba);
>> 
>> np->squeueput = qidx;
>> @@ -2920,7 +2908,7 @@ static void sym_put_start_queue(hcb_p np, ccb_p cp)
>> *  Script processor may be waiting for reselect.
>> *  Wake it up.
>> */
>> - MEMORY_BARRIER();
>> + MEMORY_WRITE_BARRIER();
>> OUTB (nc_istat, SIGP|np->istat_sem);
>> }
>> 
>> @@ -3061,7 +3049,7 @@ static int sym_wakeup_done (hcb_p np)
>> 
>> cp = sym_ccb_from_dsa(np, dsa);
>> if (cp) {
>> - MEMORY_BARRIER();
>> + MEMORY_READ_BARRIER();
>> sym_complete_ok (np, cp);
>> ++n;
>> } else
>> @@ -3859,7 +3847,7 @@ static void sym_intr1 (hcb_p np)
>> *  On paper, a memory barrier may be needed here.
>> *  And since we are paranoid ... :)
>> */
>> - MEMORY_BARRIER();
>> + MEMORY_READ_BARRIER();
>> 
>> /*
>> *  First, interrupts we want to service cleanly.
>