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

[Mohammad-Reza Nabipoor]

Hi Andreas,


On Fri, Jan 05, 2024 at 08:05:34PM +0100, Andreas Klinger wrote:
> 
> Thanks for the reviews, feedbacks and further tests on embedded hardware some
> improvements could be implemented:
> - coding conventions
> - add test programms
> - add openmmap helper function
> - fix alignment issues when accessing memory mapped hardware registers, e. g.
>   long @ 0x3#B need to be properly aligned
> - add error messages via pk_printf()
> - add error code IOD_EMMAP
> 


Thank you very much for the v2.
I have some comments.


> 
> diff --git a/doc/poke.texi b/doc/poke.texi
> index 01ed802a..585cc19f 100644
> --- a/doc/poke.texi
> +++ b/doc/poke.texi
> @@ -8931,6 +8933,46 @@ the buffer.
>  When a new memory buffer IOS is opened it becomes the current IO
>  space.  @xref{file command}.
>  
> +@node mmap command
> +@section @code{.mmap}
> +@cindex @code{.mmap}
> +@cindex opening memory mapped device
> +@cindex IO space
> +The @command{.mmap} command opens a new IO space where the content is memory
> +mapped from the device driver or file system. This is especially useful when 
> a
> +kernel driver offers a mmap function for mapping the content of kernel memory
> +into userspace. The most famous example is the mem device with it's device 
> node
> +/dev/mem. But there are also many drivers especially in embedded systems 
> using
> +it.
> +In the case of a regular file the mapping is done by the file system driver.
> +The syntax is:
> +
> +@example
> +.mmap @var{filename}, @var{base}, @var{size}
> +@end example
> +
> +@noindent
> +where @var{filename} is the name of the device node or the file name, 
> @var{base}
> +is the starting offset (or base address) of the mapping and @var{size} is the
> +length of the mapped area.
> +
> +The @var{base} has to be a multiple of the mmu page size on the system. It 
> can


s/mmu/MMU/


> +be retrieved via the c function getpagesize(). On most systems it's 4096 
> Bytes.


s/c function/C function/


> +
> +When writing to the mapped area the portion which is written is memory synced
> +with the syscall msync() after every write operation.
> +
> +For an example of mapping the gpio registers via /dev/mem on the Beaglebone


s/gpio/GPIO/


> +black board and accessing the output enable (GPIO_OE) and data out
> +(GPIO_DATAOUT) registers of the io memory:


s/io/IO/


> diff --git a/libpoke/ios-dev-mmap.c b/libpoke/ios-dev-mmap.c
> new file mode 100644
> index 00000000..33fe11a2
> --- /dev/null
> +++ b/libpoke/ios-dev-mmap.c
>...
> +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 = count;
> +  int spare = 0;
> +
> +  if ((offset + count) > dev_map->size)


Please use the following condition to prevent integer overflow:

        !(offset < dev_map->size && count <= dev_map->size - offset)

Or

        offset >= dev_map->size || count > dev_map->size - offset


> +    return IOD_EOF;
> +
> +  if (align > 8)
> +    align = 8;
> +
> +  if (align > 1 && offset % align)


Why not returning IOD_EIOFF for "Invalid offset"? Or some other error?
If user is asking for an invalid range of memory, we have to return
an error, rather than trying to "fix" the problem.  It prevents mistakes.

Or we can handle the alignment like how some processors handle?
  For N byte read, you have to be aligned on N-byte boundaries.


> +    {
> +      void *tmp;
> +      spare = offset % align;
> +      tmp = malloc (count + align);


Also for this temporary allocation, can't we use a small array with a static
size or a small dynamic array?  By adding a field in `struct ios_dev_mmap':

        unsigned char buf[SOME_FIXED_VALUE];

or
        unsigned char *buf;  // That'll be initialized by malloc in open


Then we need two memcpy (to copy different ranges from `dev_map->addr').


> +      memcpy (tmp, dev_map->addr + offset - spare, count + align);
> +      memcpy (buf, tmp + spare, count);
> +      free (tmp);
> +    }
> +  else
> +    memcpy (buf, dev_map->addr + offset, count);
> +
> +  return IOD_OK;
> +}
> +
> +static int
> +ios_dev_mmap_pwrite (void *iod, const void *buf, size_t count,
> +                     ios_dev_off offset)
> +{
> +  struct ios_dev_mmap *dev_map = iod;
> +  int align = count;
> +
> +  if ((offset + count) > dev_map->size)
> +    return IOD_EOF;
> +


Same here.


> +  if (align > 8)
> +    align = 8;
> +
> +  if (align > 1 && offset % align)


Ditto.


> +    {
> +      void *tmp;
> +      int spare = 0;
> +      spare = offset % align;
> +      tmp = malloc (count + spare);
> +      memcpy (tmp, dev_map->addr + offset - spare, align);
> +      memcpy (tmp + spare, buf, count);
> +      memcpy (tmp + spare + count, dev_map->addr + offset + count - spare, 
> align);
> +      memcpy (dev_map->addr + offset - spare, tmp, count + align);
> +      free (tmp);
> +    }
> +  else
> +    memcpy (dev_map->addr + offset, buf, count);
> +
> +  return IOD_OK;
> +}
> +
> +static ios_dev_off
> +ios_dev_mmap_size (void *iod)
> +{
> +  struct ios_dev_mmap *dev_map = iod;
> +
> +  return dev_map->size;
> +}
> +
> +static int
> +ios_dev_mmap_flush (void *iod, ios_dev_off offset)
> +{
> +  struct ios_dev_mmap *dev_map = iod;
> +  int ret;
> +
> +  if (dev_map->reg_file)
> +    {
> +      ret = msync (dev_map->addr, dev_map->size, MS_SYNC);
> +      if (ret == -1)
> +        {
> +          pk_printf ("Error in msync of %s base: 0x%lx len: 0x%lx err: %s\n",
> +                     dev_map->filename, dev_map->addr, dev_map->size,
> +                     strerror (errno));


Why not `return IOD_ERROR' here?


> +        }
> +    }
> +
> +  return IOS_OK;


return IOD_OK; // Device, not Space


> diff --git a/poke/pk-help.pk b/poke/pk-help.pk
> index e5129e2d..b7b35e06 100644
> --- a/poke/pk-help.pk
> +++ b/poke/pk-help.pk
> @@ -245,6 +245,28 @@ Create a IO space providing access to a Network Block 
> Device
>  identified by the given URI."
>      };
>  
> +pk_help_add_topic
> +  :entry Poke_HelpEntry {
> +      category = "dot-commands",
> +      topic = ".mmap",
> +      synopsis = ".mmap FILENAME, BASE, SIZE",
> +      summary = "open a device driver or file with memory mapping",
> +      description = "\
> +Create a IO space providing access to a device driver with mmap function
> +or also a file where the mmap is provided by the file system driver.
> +
> +.mmap /dev/mem, BASE, SIZE
> +Mapps physical memory area at BASE address which have to be
> +a multiple of the page size.
> +
> +.mmap /dev/mem, 0x4804c000, 0x1000
> +int @ 0x13C#B
> +int @ 0x134#B
> +Example on a Beaglebone black for watching gpio registers and possibly 
> further


s/gpio/GPIO/




Regards,
Mohammad-Reza