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

[Andreas Klinger]

Hi Mohammad and Jose,

Mohammad-Reza Nabipoor <mnabipoor@gnu.org> schrieb am Mo, 08. Jan 01:58:
> 
> 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;

This example I gave you here is much simpler than the real world. In real world
in the JPG header we have segments and these segments are not aligned but
contain an uint16 size value. Here is the pickle I'm about to develop at the
moment:

type JPG_SOI = 
  struct
  {
    byte ff = 0xFF;
    byte xx = 0xD8;
  };

type JPG_SOS = 
  struct
  {
    byte ff = 0xFF;
    byte xx = 0xDA;
  };

type JPG_EOI = 
  struct
  {
    byte ff = 0xFF;
    byte xx = 0xD9;
  };

type JPG_Segment = 
  struct
  {
    byte ff == 0xFF;
    byte xx : xx != 0xD8 && xx != 0xDA && xx != 0xD9 && xx >= 0xC0 && xx < 0xFF;
    uint16 size;
    char[size-2] data;
    method _print = void:
    {
      printf ("\ntag: %u16x  size: %u16d",  ff:::xx, size); 
    }
  };

type JPG_Header =
  struct
  {
    JPG_SOI soi;
    JPG_Segment[] segments;
    JPG_SOS sos;
    method get_nsegs = int:
    {
      return segments'length;
    }
  };

The problem is the uint16 size in JPG_Segment which is also on unaligned 
addresses if the predecessing segment used an odd size value for its data.

Sorry for giving you an over-simplified example on first hand.

>   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?

Because of the example above to return E_BADALIGN is no option in my use case
and to use an integral structure is (from my point of view) also not possible if
I want to model the data structure given by the spec.

Best regards,

Andreas

signature.asc
Description: PGP signature