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.

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.