Add NetBSD compatible bus_space_peek_N() and bus_space_poke_N() functions and implement it on arm64.
ClosedPublic
Actions

Authored by mmel on Jun 20 2020, 9:49 AM.

Details

Reviewers

andrew
kib
jhb
0mp
manu

Group Reviewers

manpages

Commits

rS365899: Add NetBSD compatible bus_space_peek_N() and bus_space_poke_N() functions.

Summary

One problem with the bus_space_read_N() and bus_space_write_N() family of functions is that they provide no protection against exceptions which can occur when no physical hardware or device responds to the read or write cycles. In such a situation, the system typically would panic due to a kernel-mode bus error. The bus_space_peek_N() and bus_space_poke_N() family of functions provide a mechanism to handle these exceptions gracefully without the risk of crashing the system.

Typical example is access to PCI(e) configuration space in bus enumeration function on badly implemented PCI(e) root complexes (RK3399 or Neoverse N1 N1SDP and/or access to PCI(e) register when device is in deep sleep state.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mmel created this revision.Jun 20 2020, 9:49 AM

Herald added a reviewer: manu. · View Herald TranscriptJun 20 2020, 9:49 AM

Herald added subscribers: emaste, imp. · View Herald Transcript

mmel requested review of this revision.Jun 20 2020, 9:49 AM

The kcsan implementation for peek will be something like the following (untested):

#define CSAN_BUS_PEEK_FUNC(width, type)                                    \
        int kcsan_bus_space_peek_##width(bus_space_tag_t tag,              \
            bus_space_handle_t hnd, bus_size_t offset, type *value)        \
        {                                                                  \
                kcsan_access((uintptr_t)value, sizeof(type), true, false,  \
                    __RET_ADDR);                                           \
                return (bus_space_peek_##width(tag, hnd, offset, value));  \
        }

#if defined(__aarch64__)
CSAN_BUS_PEEK_FUNC(1, uint8_t)
CSAN_BUS_PEEK_FUNC(2, uint16_t)
...
#endif

#define CSAN_BUS_POKE_FUNC(width, type)                                    \
        int kcsan_bus_space_poke_##width(bus_space_tag_t tag,              \
            bus_space_handle_t hnd, bus_size_t offset, type value)         \
        {                                                                  \
                return (bus_space_poke_##width(tag, hnd, offset, value));  \
        }

...

The poke function doesn't need kcsan checks as it only accesses device memory and kcsan doesn't currently track these accesses.

sys/sys/_cscan_bus.h
79 ↗	(On Diff #73388)	You need to add the peek and poke signatures here.

In D25371#559709, @andrew wrote:

The kcsan implementation for peek will be something like the following (untested):

Perfect, many thanks for help, I will do it in next version.

Do you need them in asm ?

As I understand the idea, the code is approximately

uint32_t
bus_space_poke_4(...)
{
    curthread->td_pcb->pcb_onfault = <peek_handler>;
    res = bus_space_read_4(...);
    dmb(sy);
    curthread->td_pcb->pcb_onfault = NULL;
}

And this structure immediately illustrates another question. You currently use fsu_fault for handler, which suppresses all kinds of exceptions. But bus_space_read() needs to dereference some pointers, which might fault due to programmer' errors, not because of the hardware state. It would be nice to distinguish the mistake/hardware error situations and react differently.

In D25371#559724, @kib wrote:

Do you need them in asm ?

See below, but what's wrong on assembly code? I'm asking as the one who starts his programmers career on 6502 assembly :)
But I really think, in this case, that asm code is more appropriate (in terms of the stability of the generated code) that hacked C - I just hate all ideas like 'read_once ()' or something.

As I understand the idea, the code is approximately

uint32_t
bus_space_poke_4(...)
{
    curthread->td_pcb->pcb_onfault = <peek_handler>;
    res = bus_space_read_4(...);
    dmb(sy);
    curthread->td_pcb->pcb_onfault = NULL;
}

Right.

And this structure immediately illustrates another question. You currently use fsu_fault for handler, which suppresses all kinds of exceptions. But bus_space_read() needs to dereference some pointers, which might fault due to programmer' errors, not because of the hardware state. It would be nice to distinguish the mistake/hardware error situations and react differently.

Well, in ideal world we should use specialized fault handler fired only by small subset of aborts ( I think that reality is that only synchronous external abort[1] is appropriate) and covering only single register to memory load/store (+ necessary synchronization instruction). In real-world implementation I select to not make trap.c more complicated and i think that handling all aborts over single load/store (that is reason, why is executive code implemented in asm) is acceptable compromise.
What do you think?

[1]
Ironically, synchronous external abort is not currently handled by fsu handle - but I think that this is bug (a malicious user may pass pointer to this...)
See https://github.com/strejda/freebsd/commit/64a947626543d6885571c98167324ee21a33ea2f for WIP patch.

In D25371#559764, @mmel wrote:

In D25371#559724, @kib wrote:

Do you need them in asm ?

See below, but what's wrong on assembly code? I'm asking as the one who starts his programmers career on 6502 assembly :)

Why write a code in assembly if it can be expressed in C ?

But I really think, in this case, that asm code is more appropriate (in terms of the stability of the generated code) that hacked C - I just hate all ideas like 'read_once ()' or something.
As I understand the idea, the code is approximately
uint32_t
bus_space_poke_4(...)
{
    curthread->td_pcb->pcb_onfault = <peek_handler>;
    res = bus_space_read_4(...);
    dmb(sy);
    curthread->td_pcb->pcb_onfault = NULL;
}
Right.

And this structure immediately illustrates another question. You currently use fsu_fault for handler, which suppresses all kinds of exceptions. But bus_space_read() needs to dereference some pointers, which might fault due to programmer' errors, not because of the hardware state. It would be nice to distinguish the mistake/hardware error situations and react differently.

Well, in ideal world we should use specialized fault handler fired only by small subset of aborts ( I think that reality is that only synchronous external abort[1] is appropriate) and covering only single register to memory load/store (+ necessary synchronization instruction). In real-world implementation I select to not make trap.c more complicated and i think that handling all aborts over single load/store (that is reason, why is executive code implemented in asm) is acceptable compromise.
What do you think?

If you implement load and store in assembler, you can specialize the trap handler on the pc value which caused the abort, in addition to the abort type. We do something along that lines for segments reload on amd64. See ld_es/ld_ds labels in amd64/exceptions.S and then look for the same symbols in amd64/trap.c (this is the simplest case).

[1]
Ironically, synchronous external abort is not currently handled by fsu handle - but I think that this is bug (a malicious user may pass pointer to this...)
See https://github.com/strejda/freebsd/commit/64a947626543d6885571c98167324ee21a33ea2f for WIP patch.

crowston_protonmail.com added a subscriber: crowston_protonmail.com.Jun 22 2020, 10:28 PM

diizzy added a subscriber: diizzy.Jun 25 2020, 11:38 AM

finish implementation

added manpage
added default implementation for other architectures
kcsan functions was implemented
fault detection fault for arm64. was rewritten

Herald added a reviewer: manu. · View Herald TranscriptJul 12 2020, 8:56 AM

Herald added a reviewer: manpages. · View Herald Transcript

Herald added a subscriber: jhibbits. · View Herald Transcript

gbe added a subscriber: gbe.Jul 12 2020, 9:07 AM

kib added inline comments.Jul 17 2020, 9:00 PM

share/man/man9/bus_space.9
1184 ↗	(On Diff #74346)	I think it is important to provide some more information there, in particular, on which arches that can happen (arm, what else ?) , and which arches silently return some pattern on accesses to unclaimed addresses (x86).
sys/arm64/arm64/trap.c
162 ↗	(On Diff #74346)	It is enough to write return (addr == (uint64_t)generic_bs_peek1 \|\| ...));
493 ↗	(On Diff #74346)	This space trim is unrelated, please commit it in advance.

kib added inline comments.Jul 17 2020, 9:05 PM

sys/arm64/arm64/bus_space_asm.S
406 ↗	(On Diff #74346)	There a comment should be added noting that test_bs_fault() (or external_abort()) depends on the bus access instruction be the very first instruction in the generic_bs_p* functions.

Address objections

mmel marked 2 inline comments as done.Sep 18 2020, 9:25 AM

mmel added inline comments.

share/man/man9/bus_space.9
1184 ↗	(On Diff #74346)	I don't want to be a much exact. The problem is not related (in theory) to architecture, but to PCI(e) (or any other) controller implementation (or with its SoC integration). I hear about similar problems also i386/amd64 few (many:) ) years ago, but I think that these problems are handled/hidden by firmware (acpi) now...
sys/arm64/arm64/bus_space_asm.S
406 ↗	(On Diff #74346)	On second read, I slightly prefer to use own labels - it makes code to more friendly for future expansion .

kib accepted this revision.Sep 18 2020, 2:10 PM

kib added inline comments.

share/man/man9/bus_space.9
1184 ↗	(On Diff #74346)	You would be right if we only spoke about CPU architecture, but as machine architecture i386 and amd64 have pretty much defined PCIe bridge behavior that cannot be changed. I am aware of at least one high-profile driver that depends on specific values returned when access is done to either config or io space for gone device. Anyway.