Page MenuHomeFreeBSD

Adapt assembler implementations of atomic_load_64() and atomic_store_64() on o32 mips to new calling conventions.
ClosedPublic

Authored by kib on Dec 22 2017, 3:19 PM.

Details

Summary

Also, existing assembler implementation uses ld/sd instructions which
are only available on MIPS64. Use lw/sw instructions instead.

Note that the new implementation keeps disabling interrupts around the
load and store sequence, but this does not make the operations atomic.
Only use of llx instruction from MIPS32 v6 would allow this.
The functions are only used by ddb with somewhat doubtful reasoning,
so atomicity is in fact not too critical.

I kept the nops after the accesses, I suppose this is done due for
some requirements about MMIO.

I believe that the code is endian-neutral.

Test Plan

I only did the tinderbox run.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Dec 22 2017, 3:19 PM
kib edited the summary of this revision. (Show Details)Dec 22 2017, 3:29 PM
imp added inline comments.Dec 22 2017, 4:22 PM
sys/mips/mips/support.S
864 ↗(On Diff #36908)

Why don't these use the normal ll/sc loops?

Also, the interrupt disabling is bogus. The number of NOP hazards vary from CPU to CPU and newer mips32 instructions allow one to do this without the raciness of the read / modify / write of the STATUS register. This is a fairly heavyweight operation.

872 ↗(On Diff #36908)

where do we load the actual value now?

kib added inline comments.Dec 22 2017, 4:46 PM
sys/mips/mips/support.S
864 ↗(On Diff #36908)

How can I use ll/sc for 64bit value on 32bit mips ? According to the architecture specification, only one ll/sc sequence can be active, and second ll terminates the monitor armed by the first ll. I already mentioned llx which indeed allows to load two 32bit values atomically, but llx is only available on r6.

Interrupt disabling was there in the version before my changes. Sure it is bogus. Feel free to use more appropriate interrupt control.

872 ↗(On Diff #36908)

Somewhere in the caller.

kib added inline comments.Dec 22 2017, 5:39 PM
sys/mips/mips/support.S
872 ↗(On Diff #36908)

I later realized that your question clearly indicates the missed context. Please see r327074.

jhb added a comment.Dec 22 2017, 6:14 PM

BTW, I looked and the only place atomic_load_64 and atomic_store_64 are used in sys/mips is for DDB (db_read_bytes and db_write_bytes in sys/mips/mips/db_interface.c). It is not clear to me that this actually needs to be near this fancy. Most architectures just do a simple byte loop. Only 32-bit arm, mips, and powerpc attempt to handle accesses for exact word-size specially. arm doesn't try to handle 64-bit accesses with this kind of magic. powerpc just uses a 'uint64_t' load and store via C the same as it does for other word sizes. I suspect the db_read/write_bytes can just be simplified to match what powerpc does and and use simple C load/store and then these functions can be removed.

kib added a comment.Dec 22 2017, 6:28 PM
In D13586#284229, @jhb wrote:

BTW, I looked and the only place atomic_load_64 and atomic_store_64 are used in sys/mips is for DDB (db_read_bytes and db_write_bytes in sys/mips/mips/db_interface.c). It is not clear to me that this actually needs to be near this fancy. Most architectures just do a simple byte loop. Only 32-bit arm, mips, and powerpc attempt to handle accesses for exact word-size specially. arm doesn't try to handle 64-bit accesses with this kind of magic. powerpc just uses a 'uint64_t' load and store via C the same as it does for other word sizes. I suspect the db_read/write_bytes can just be simplified to match what powerpc does and and use simple C load/store and then these functions can be removed.

Yes, I mentioned this in r327074 commit message, and the only purpose of this revision is to get out of the conflict created by MI atomic_load_64().

If you prefer to remove mips atomic_load_64() and do plain load/store in ddb, I will do that instead.

jhb added a comment.Dec 22 2017, 8:40 PM

Yes, let's kill these and fix DDB instead.

imp added a comment.Dec 22 2017, 8:44 PM
In D13586#284245, @jhb wrote:

Yes, let's kill these and fix DDB instead.

Yea, not that I have the full context, it's better to kill these routines entirely...

kib updated this revision to Diff 36922.Dec 22 2017, 9:58 PM

Remove mips 32bit atomic_load/store_64.

jhb accepted this revision.Dec 22 2017, 11:07 PM
jhb added inline comments.
sys/mips/include/atomic.h
343 ↗(On Diff #36922)

Unrelated: I wonder if this should be conditional on 64-bit registers (N32 and N64).

This revision is now accepted and ready to land.Dec 22 2017, 11:07 PM
This revision was automatically updated to reflect the committed changes.