lkml.org 
[lkml]   [2021]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH for 4.16 v7 02/11] powerpc: membarrier: Skip memory barrier in switch_mm()
From
Date


Le 19/06/2021 à 17:02, Segher Boessenkool a écrit :
> On Sat, Jun 19, 2021 at 11:35:34AM +0200, Christophe Leroy wrote:
>>
>>
>> Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit :
>>> ----- On Jun 18, 2021, at 1:13 PM, Christophe Leroy
>>> christophe.leroy@csgroup.eu wrote:
>>> [...]
>>>>
>>>> I don't understand all that complexity to just replace a simple
>>>> 'smp_mb__after_unlock_lock()'.
>>>>
>>>> #define smp_mb__after_unlock_lock() smp_mb()
>>>> #define smp_mb() barrier()
>>>> # define barrier() __asm__ __volatile__("": : :"memory")
>>>>
>>>>
>>>> Am I missing some subtility ?
>>>
>>> On powerpc CONFIG_SMP, smp_mb() is actually defined as:
>>>
>>> #define smp_mb() __smp_mb()
>>> #define __smp_mb() mb()
>>> #define mb() __asm__ __volatile__ ("sync" : : : "memory")
>>>
>>> So the original motivation here was to skip a "sync" instruction whenever
>>> switching between threads which are part of the same process. But based on
>>> recent discussions, I suspect my implementation may be inaccurately doing
>>> so though.
>>>
>>
>> I see.
>>
>> Then, if you think a 'sync' is a concern, shouldn't we try and remove the
>> forest of 'sync' in the I/O accessors ?
>>
>> I can't really understand why we need all those 'sync' and 'isync' and
>> 'twi' around the accesses whereas I/O memory is usually mapped as 'Guarded'
>> so memory access ordering is already garantied.
>>
>> I'm sure we'll save a lot with that.
>
> The point of the twi in the I/O accessors was to make things easier to
> debug if the accesses fail: for the twi insn to complete the load will
> have to have completed as well. On a correctly working system you never
> should need this (until something fails ;-) )
>
> Without the twi you might need to enforce ordering in some cases still.
> The twi is a very heavy hammer, but some of that that gives us is no
> doubt actually needed.
>

Well, I've always been quite perplex about that. According to the documentation of the 8xx, if a bus
error or something happens on an I/O access, the exception will be accounted on the instruction
which does the access. But based on the following function, I understand that some version of
powerpc do generate the trap on the instruction which was being executed at the time the I/O access
failed, not the instruction that does the access itself ?

/*
* I/O accesses can cause machine checks on powermacs.
* Check if the NIP corresponds to the address of a sync
* instruction for which there is an entry in the exception
* table.
* -- paulus.
*/
static inline int check_io_access(struct pt_regs *regs)
{
#ifdef CONFIG_PPC32
unsigned long msr = regs->msr;
const struct exception_table_entry *entry;
unsigned int *nip = (unsigned int *)regs->nip;

if (((msr & 0xffff0000) == 0 || (msr & (0x80000 | 0x40000)))
&& (entry = search_exception_tables(regs->nip)) != NULL) {
/*
* Check that it's a sync instruction, or somewhere
* in the twi; isync; nop sequence that inb/inw/inl uses.
* As the address is in the exception table
* we should be able to read the instr there.
* For the debug message, we look at the preceding
* load or store.
*/
if (*nip == PPC_INST_NOP)
nip -= 2;
else if (*nip == PPC_INST_ISYNC)
--nip;
if (*nip == PPC_INST_SYNC || (*nip >> 26) == OP_TRAP) {
unsigned int rb;

--nip;
rb = (*nip >> 11) & 0x1f;
printk(KERN_DEBUG "%s bad port %lx at %p\n",
(*nip & 0x100)? "OUT to": "IN from",
regs->gpr[rb] - _IO_BASE, nip);
regs->msr |= MSR_RI;
regs->nip = extable_fixup(entry);
return 1;
}
}
#endif /* CONFIG_PPC32 */
return 0;
}

Am I right ?

It is not only the twi which bother's me in the I/O accessors but also the sync/isync and stuff.

A write typically is

sync
stw

A read is

sync
lwz
twi
isync

Taking into account that HW ordering is garanteed by the fact that __iomem is guarded, isn't the
'memory' clobber enough as a barrier ?

Thanks
Christophe

\
 
 \ /
  Last update: 2021-06-21 16:12    [W:0.106 / U:0.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site