Re: Re: [PATCH v2] Add io space MMAP for memory mapping devices and files

[Mohammad-Reza Nabipoor]

Hello Andreas and Jose.


Let's summarize the problems and different solutions (Andreas, please correct 
me if
I'm missing something):

Goal:
  We want to poke structured data in RAM (the content of memory is only 
changable
  through load/store instructions).

Problem:
  Attemping to Poke data of the following type will cause SIGBUS due to 
unaligned
  access at offset 1#B (i.e. reading 2#B from an odd address).

    type MY_Tag =
      struct
      {
        byte b;
        uint16 u;
      };

    var m = MY_Tag @ 0x0#B;

  Detailed discussion: MMAP-backed IOS, translate mapping of `MY_TAG' into two
  `memcpy':

    memcpy (buf, base_adr + 0, 1);
  and
    memcpy (buf, base_adr + 1, 2);

  The 2nd `memcpy' will cause the linux module's code to generate an unaligned
  memory access which in turn, leads to a SIGBUS in the poke process.

Possible fix:
  Detect these unaligned memory accesses in the MMAP IOD and doing the right 
thing.
  Which is the following code:


On Sun, Jan 07, 2024 at 02:15:59AM +0100, Andreas Klinger wrote:
> static int
> ios_dev_mmap_pread (void *iod, void *buf, size_t count, ios_dev_off offset)
> {
>   struct ios_dev_mmap *dev_map = iod;
>   int align = sizeof(void*);
>   char *m = buf;
> 
>   if (offset > dev_map->size || count > dev_map->size - offset)
>     return IOD_EOF;
> 
>   /* copy unaligned bytes */
>   while (count && offset % align)
>     {
>       memcpy (m, dev_map->addr + offset, 1);
>       count--;
>       offset++;
>       m++;
>     }
> 
>   /* copy with the address bus size */
>   while (count >= align)
>     {
>       memcpy (m, dev_map->addr + offset, align);
>       count -= align;
>       offset += align;
>       m += align;
>     }
> 
>   /* copy remaining unaligned bytes */
>   while (count)
>     {
>       memcpy (m, dev_map->addr + offset, 1);
>       count--;
>       offset++;
>       m++;
>     }
> 
>   return IOD_OK;
> }
> 


SIGBUS or SIGSEGV will not happen for the function defined above.


Now let's look at another class of problems:

Goal:
  We want to poke Memory-Mapped IO to deal with the machine's peripherals.

Problem:
  type SomeRegister =
    struct
    {
       uint<8> part_a;
       uint<16> part_b;
       uint<8> part_c;
    };
  var register_value = SomeRegister @ some_register_address;

Fix (in Poke code):
  User should use integral struct, instead of normal structs for MMIO.

  type SomeRegister =
    struct uint<32>
    {
       uint<8> part_a;
       uint<16> part_b;
       uint<8> part_c;
    };

  Which will leads to one `memcpy (buf, adr, 4);'.
  This means that the IOD doesn't need to do anything!

Fix (in IOD):
  But relying on user to do the right thing is not always a good thing.
  I'd argue that the same "fix" in IOD for this problem is wrong.  One cannot 
read
  the content of a register partially. Because this memory is "volatile" and
  machine can change it (for reasons other than load/store instructions) and
  also reading/writing a register can cause side-effects.

  This should raise an `E_bad_align' (or `E_ios' or whatever Poke exception)
  to inform the user that he/she did something wrong.

  Easiest fix is to open this IOS using an `MMAP_IOD_F_ALIGNED' flag.

    var fd = open ("mmap://0xcafe/0xbabe/dev/my-peripheral", 
MMAP_IOD_F_ALIGNED);

  In IOD's pread/pwrite functions, we can refuse to do any read/write operation
  on unaligned addresses:

    #define ALIGNMENT_MASK (sizeof (void *) - 1)

      if (offset & ALIGNMENT_MASK)
        return E_BADALIGN;
      // ...

    #undef ALIGNMENT_MASK

  Or we can have more ALIGNED flags, e.g., IOD_F_ALIGN4 and IOD_F_ALIGN8 to give
  user more control (instead of relying on `sizeof (void *)').


So with these two approaches, I think we're covering most use cases and our
fixes are simple and effective.  WDYT?


Regards,
Mohammad-Reza