Renew arm userspace accessors

[kib]

Summary

Take advantage of the FreeBSD memory spaces layout and check for invalid user buffers passed to copyin(9), fubyte(9) etc by comparing the buffer upper boundary with VM_MAXUSER_ADDRESS. This effectively removes the need of using ldrt/strt instructions, which are not correctly handled right now (but see D3617), and makes arm similar to other architectures.

Also, I used an opportunity to switch ARM to fueword(9) and casueword(9).

I did not handled bcopyinout_xscale.S (yet ?). If D3617 also comes in, it might make sense to leave the code as is.

Test Plan

Booted multiuser. Also tested casueword(9) by running some threading code.

Also tested with https://www.kib.kiev.ua/kib/copyin-arm.tbz.

[kib]

Also handle bcopyinout_xscale.S, which is apparently used always.

[mmel]

Can you use full diff next time, please?
I think that "BX" instruction is only available on CPUs with Thumb support. So this code don't work on some ARMv4 CPUs. Please see definition of RET macro.

sys/arm/arm/copystr.S

116

VM_MAXUSER_ADDRESS + 1 ?

169

Ditto

sys/arm/arm/fusu.S

92

Why clrex?

[kib]

New revision will appear in a minute with bxcs replaced by RETc(cs).

sys/arm/arm/copystr.S

116

Hm, no. Note that I compare VM_MAXUSER_ADDRESS with the address which is accessed, not the one past of the buffer end address, which is done when the whole buffer length is validated in advance, like in copyin and copyout.

copyinstr cannot validate the maxlength buffer, because e.g. exec_copyout_strings() is somewhat relaxed on the maxlength usage.

sys/arm/arm/fusu.S

92

Because in the case when old value is not equal to the *base value, we branch out of the loop,with monitor still armed. I think that it is good programming practice to clean after itself. Might be, clearing the monitor would even turn off some unneccessary CPU blocks.

If the old and *base values are equal, the monitor is unarmed by strex.

[kib]

Replace direct bxcs lr usage with RETc(cs).

[mmel]

sys/arm/arm/bcopyinout_xscale.S

72

(Here and more times bellow)
Use RETc(cs). Direct access to PC is deprecated for ARMv6+, See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/dom1361289878994.html

sys/arm/arm/copystr.S

116

Sorry for noise, my bad.

sys/arm/arm/fusu.S

87

It is, of course only minor detail. On ARM, usage of STREX without corresponding LDREX with same address results to UNDEFINED behavior. CLREX doesn't change nothing on this. CLREX, imo, is designed for usage in CPU context switch.

[andrew]

sys/arm/arm/copystr.S

210

When does this label get used?

sys/arm/arm/fusu.S

74

This doesn't look correct, shouldn't it be ldmfdeq? i.e. only load when equal. Alternatively you could use the ip register (r12) above as it's callee saved, then keep the stmfd below the DIAGNOSTIC section..

89–91

This looks wrong. Either the ne was incorrectly removed, or the mov is unneeded, and you can use r5 in the store below.

[kib]

Am I miss something, or ARMv5 path in casueword lacks atomicity ? Should e.g. interrupts be disabled between #else/#endif for __ARM_ARCH<6 ?

sys/arm/arm/copystr.S

210

It leaked from the intermediate version, removed.

sys/arm/arm/fusu.S

74

I cannot keep pcb pointer in r12, since fault would jump through trap() and register would be corrupted. Due to this, I think it is simpler to just use ldmfdeq there.

87

I do not follow. Could you, please, explain a possible execution path where I do clrex without ldrex, or ldrex/strex/clrex sequence ?

89–91

mov is not needed, it was just wrong.

[kib]

Remove unused label.
Replace movcs pc,lr usage with RETc(cs).
Use ldmfdeq in the casueword DIAGNOSTIC block.
Remove unneeded move in the casueword ARMv5 path, which clobbered base address.

[mmel]

sys/arm/arm/fusu.S

92

It is a long story, and im not able to explain it here clearly here. But I'm ready to discuss this by mail, if you want it and if it allows my english.

[kib]

sys/arm/arm/fusu.S

92

Use mail. I surely find email more convenient than the web gui.

[andrew]

sys/arm/arm/copystr.S

210

Ok, there is probably no need to move the mov instruction then.

sys/arm/arm/fusu.S

92

Was the clrex staying or not?

[kib]

sys/arm/arm/fusu.S

92

There were no discussion in mail, and I still do not see why clearing monitor after the ldrex which is not paired with strex is wrong.

[andrew]

sys/arm/arm/fusu.S

92

As best as I can tell the clrex is unneeded. The ARM sample code in D7.3.1 of the ARM ARM is similar, other than it uses a fixed value to compare against. ARM doesn't use a clrex in this code.

Without the clrex we can remove two branch instructions, so the code would be:

ldrex   r4, [r0]
cmp     r4, r1
strexeq r5, r3, [r0]
cmpeq   r5, #1
beq     1b
str     r4, [r2]

[kib]

Do not use clrex in casueword. To test, I run multithreading code which is known to ccause contention and fall back to kernel on priority-inherited umtx.

[kib]

Change movXX r0,#1 to mov r0,#0 to actually load -1. Noted by andrew@.

[emaste]

It looks like this did not auto-close because Differential Revision: isn't on the last line in rS289372

[kib]

r289372