Page MenuHomeFreeBSD

Add NetBSD compatible bus_space_peek_N() and bus_space_poke_N() functions and implement it on arm64.
Needs ReviewPublic

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

Details

Reviewers
andrew
kib
jhb
0mp
manu
Group Reviewers
manpages
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
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mmel created this revision.Jun 20 2020, 9:49 AM
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–87

You need to add the peek and poke signatures here.

mmel added a comment.Jun 20 2020, 10:33 AM

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

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

kib added a comment.EditedJun 20 2020, 11:38 AM

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.

mmel added a comment.Jun 20 2020, 4:20 PM
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.

kib added a comment.Jun 20 2020, 11:03 PM
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.

mmel updated this revision to Diff 74346.Sun, Jul 12, 8:56 AM
mmel edited the summary of this revision. (Show Details)
mmel edited reviewers, added: 0mp; removed: manu.

finish implementation

  • added manpage
  • added default implementation for other architectures
  • kcsan functions was implemented
  • fault detection fault for arm64. was rewritten
gbe added a subscriber: gbe.Sun, Jul 12, 9:07 AM
kib added inline comments.Fri, Jul 17, 9:00 PM
share/man/man9/bus_space.9
1184

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

It is enough to write

return (addr == (uint64_t)generic_bs_peek1 || ...));
493

This space trim is unrelated, please commit it in advance.

kib added inline comments.Fri, Jul 17, 9:05 PM
sys/arm64/arm64/bus_space_asm.S
406

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.