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.
Tags
Referenced Files
F106147034: D13586.diff
Thu, Dec 26, 4:20 AM
Unknown Object (File)
Wed, Dec 25, 9:54 AM
Unknown Object (File)
Fri, Nov 29, 1:50 PM
Unknown Object (File)
Nov 21 2024, 12:17 AM
Unknown Object (File)
Nov 20 2024, 10:21 AM
Unknown Object (File)
Nov 20 2024, 5:33 AM
Unknown Object (File)
Nov 12 2024, 7:47 AM
Unknown Object (File)
Oct 29 2024, 11:30 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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?

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.

sys/mips/mips/support.S
872 ↗(On Diff #36908)

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

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.

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.

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

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...

Remove mips 32bit atomic_load/store_64.

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.