[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] add io space MMAP for memory mapping devices and files
From: |
Jose E. Marchesi |
Subject: |
Re: [PATCH] add io space MMAP for memory mapping devices and files |
Date: |
Wed, 03 Jan 2024 10:04:38 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Hello Andreas.
Thanks for the patch. It is a good one, and it is great to have that
support in the IOS.
The only major comment I have is that it needs at least some minimal
tests. I suppose a way to test this would be to use regular test data
files, like:
/* { dg-do run } */
/* { dg-data {c*} {0x10 0x20 0x30 0x40 0x50 0x60 0x70 0x80} foo.data } */
/* { dg-command { .mmap 1,4,foo.data } } */
/* { dg-output {Whatever error} } */
In poke.cmd (see sub-1.pk for another test testing a dot-command for
example).
Please see some other minor coments below.
> 2024-01-02 Andreas Klinger <ak@it-klinger.de>
>
> * libpoke/ios-dev-mmap.c: Create new io space for memory mapping
> devices and files. Use poke flags of reading and writing as
> open flags and for mmap protection. After every write there's
> a msync on the copied memory area.
> Expected handler format:
> mmap://BASE/SIZE/FILENAME
* libpoke/ios-dev-mmap.c: New file.
> * libpoke/Makefile.am: Add io space device from ios-dev-mmap.c
* libpoke/Makefile.am (libpoke_la_SOURCES): Add ios-dev-mmap.c.
> * libpoke/ios.c: Add io space for memory mapping
* libpoke/ios.c (IOS_DEV_MMAP): Define.
(ios_dev_ifs): Initialize IOS_DEV_MMAP.
> * poke/pk-cmd-ios: Add new .mmap command. Syntax:
> .mmap FILENAME, BASE, SIZE
> where BASE needs to be a multiple of the mmu page size,
> e. g. 4096 on many systems
* poke/pk-cmd-ios.c (pk_cmd_mmap): New function.
> * poke/pk-cmd: Add mmap_cmd as new command.
* poke/pk-cmd.c (dot_cmds): Add mmap_cmd.
> * poke/pk-help.pk: Add help text for .mmap.
> * doc/poke.texi: Add documentation on new .mmap command
* doc/poke.texi (mmap command): New section.
> ---
>
> Thanks to Mohammad-Reza for indicating me exactly where I should start with
> development. This saved me a lot of time.
>
> doc/poke.texi | 42 +++++
> libpoke/Makefile.am | 2 +-
> libpoke/ios-dev-mmap.c | 337 +++++++++++++++++++++++++++++++++++++++++
> libpoke/ios.c | 3 +
> poke/pk-cmd-ios.c | 51 +++++++
> poke/pk-cmd.c | 2 +
> poke/pk-help.pk | 22 +++
> 7 files changed, 458 insertions(+), 1 deletion(-)
> create mode 100644 libpoke/ios-dev-mmap.c
>
> diff --git a/doc/poke.texi b/doc/poke.texi
> index 01ed802a..585cc19f 100644
> --- a/doc/poke.texi
> +++ b/doc/poke.texi
> @@ -208,6 +208,7 @@ Dot-Commands
> * source command:: Executing commands in files.
> * file command:: Opening and selecting file IO spaces.
> * mem command:: Opening and selecting memory IO spaces.
> +* mmap command:: Opening and selecting memory mapped devices.
> * nbd command:: Opening and selecting NBD IO spaces.
> * proc command:: Opening and selecting process IO spaces.
> * ios command:: Switching between IO spaces.
> @@ -8819,6 +8820,7 @@ au BufRead,BufNewFile *.pk set filetype=poke
> * source command:: Executing commands in files.
> * file command:: Opening and selecting file IO spaces.
> * mem command:: Opening and selecting memory IO spaces.
> +* mmap command:: Opening and selecting memory mapped devices.
> * nbd command:: Opening and selecting NBD IO spaces.
> * proc command:: Opening and selecting process IO spaces.
> * sub command:: Opening IO sub-spaces.
> @@ -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
> +be retrieved via the c function getpagesize(). On most systems it's 4096
> Bytes.
> +
> +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
> +black board and accessing the output enable (GPIO_OE) and data out
> +(GPIO_DATAOUT) registers of the io memory:
> +
> +@example
> +$ poke
> +(poke) .mmap /dev/mem, 0x4804c000, 0x1000
> +(poke) int @ 0x13C#B
> +(poke) int @ 0x134#B
> +@end example
> +
> @node nbd command
> @section @code{.nbd}
> @cindex @code{.nbd}
> diff --git a/libpoke/Makefile.am b/libpoke/Makefile.am
> index f7a66113..71c4a0e6 100644
> --- a/libpoke/Makefile.am
> +++ b/libpoke/Makefile.am
> @@ -62,7 +62,7 @@ libpoke_la_SOURCES = libpoke.h libpoke.c \
> ios-dev-file.c ios-dev-mem.c \
> ios-dev-zero.c ios-dev-sub.c \
> ios-buffer.h ios-buffer.c \
> - ios-dev-stream.c
> + ios-dev-stream.c ios-dev-mmap.c
>
> libpoke_la_SOURCES += ../common/pk-utils.c ../common/pk-utils.h
>
> diff --git a/libpoke/ios-dev-mmap.c b/libpoke/ios-dev-mmap.c
> new file mode 100644
> index 00000000..154cd6ad
> --- /dev/null
> +++ b/libpoke/ios-dev-mmap.c
> @@ -0,0 +1,337 @@
> +/* ios-dev-dev_map.c - Memory mapped devices. */
s/ios-dev-dev_map.c/ios-dev-mmap.c/
> +
> +/* Copyright (C) 2024 Andreas Klinger */
> +
> +/* This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/* This file implements an IO device that can be used in order to edit
> + the memory mapped from device drivers via dev_map syscall. */
> +
> +#include <config.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +/* We want 64-bit file offsets in all systems. */
> +#define _FILE_OFFSET_BITS 64
> +
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +
> +#include "ios.h"
> +#include "ios-dev.h"
> +
> +/* State associated with a file device. */
> +
> +struct ios_dev_mmap
> +{
> + char *filename;
> + int fd;
> + uint64_t flags;
> + int open_flags;
> + int prot;
> + uint64_t base;
> + uint64_t size;
> + void *addr;
> +};
> +
> +static const char *
> +ios_dev_mmap_get_if_name () {
> + return "MMAP";
> +}
> +
> +static char *
> +ios_dev_mmap_handler_normalize (const char *handler, uint64_t flags, int*
> error)
> +{
> + char *new_handler = NULL;
> +
> + if (strlen (handler) > 6
> + && handler[0] == 'm'
> + && handler[1] == 'm'
> + && handler[2] == 'a'
> + && handler[3] == 'p'
> + && handler[4] == ':'
> + && handler[5] == '/'
> + && handler[6] == '/')
> + {
> + new_handler = strdup (handler);
> + if (new_handler == NULL && error)
> + *error = IOD_ENOMEM;
> + }
> +
> + if (error)
> + *error = IOD_OK;
> + return new_handler;
> +}
> +
> +/* Returns 0 when the flags are inconsistent. */
> +static inline int
> +ios_dev_mmap_convert_flags_open (int mode_flags)
> +{
> + int flags_for_open = 0;
> +
> + if ((mode_flags & IOS_F_READ)
> + && (mode_flags & IOS_F_WRITE))
> + {
> + flags_for_open |= O_RDWR;
> + }
> + else if (mode_flags & IOS_F_READ)
> + {
> + flags_for_open |= O_RDONLY;
> + }
> + else if (mode_flags & IOS_F_WRITE)
> + {
> + flags_for_open |= O_WRONLY;
> + }
> + else
> + /* Cannot open a file neither to write nor to read. */
> + return -1;
> +
> + return flags_for_open;
> +}
> +
> +/* Returns 0 when the flags are inconsistent. */
> +static inline int
> +ios_dev_mmap_convert_mmap_prot (int open_flags)
> +{
> + int mmap_prot = 0;
> +
> + if (open_flags & O_RDWR)
> + {
> + mmap_prot |= PROT_READ | PROT_WRITE;
> + }
> + else if (open_flags & O_RDONLY)
> + {
> + mmap_prot |= PROT_READ;
> + }
> + else if (open_flags & O_WRONLY)
> + {
> + mmap_prot |= PROT_WRITE;
> + }
> + else
> + /* Cannot dev_map neither to write nor to read. */
> + return -1;
> +
> + return mmap_prot;
> +}
> +
> +static void *
> +ios_dev_mmap_open (const char *handler, uint64_t flags, int *error,
> + void *data __attribute__ ((unused)))
> +{
> + struct ios_dev_mmap *dev_map = NULL;
> + int internal_error = IOD_ERROR;
> + uint8_t mode_flags = flags & IOS_FLAGS_MODE;
> + int open_flags = 0;
> + int fd;
> + const char *p;
> + char *end;
> +
> + dev_map = malloc (sizeof (struct ios_dev_mmap));
> + if (!dev_map)
> + goto err;
> +
> + memset(dev_map, 0, sizeof(struct ios_dev_mmap));
Please leave space between function name and open parentheses.
Same in other points in the patch.
> +
> + /* Format of handler:
> + mmap://BASE/SIZE/FILE-NAME */
> +
> + /* Skip the mmap:// */
> + p = handler + 7;
> +
> + /* parse the base address of memory mapped area. This is an uint64. */
> + dev_map->base = strtoull (p, &end, 0);
> + if (*p != '\0' && *end == '/')
> + /* Valid integer found. */;
> + else
> + goto err;
> + p = end + 1;
> +
> + /* parse the size of the memory mapped area. This is an uint64. */
> + dev_map->size = strtoull (p, &end, 0);
> + if (*p != '\0' && *end == '/')
> + /* Valid integer found. */;
> + else
> + goto err;
> + p = end + 1;
> +
> + /* The rest of the string is the name, which may be empty. */
> + dev_map->filename = strdup (p);
> + if (!p)
> + goto err;
> +
> + /* Ok now do some validation. */
> + /* base needs to be a multiple of PAGE_SIZE */
> + if (dev_map->base % getpagesize())
> + {
> + internal_error = IOD_EFLAGS;
> + goto err;
> + }
> +
> + if (mode_flags)
> + {
> + /* Decide what mode to use to open the file. */
> + open_flags = ios_dev_mmap_convert_flags_open (mode_flags);
> + if (open_flags == -1)
> + {
> + internal_error = IOD_EFLAGS;
> + goto err;
> + }
> + fd = open (dev_map->filename, open_flags);
> + if (fd == -1)
> + goto err;
> + flags = mode_flags;
> + }
> + else
> + {
> + /* Try read-write initially.
> + If that fails, then try read-only.
> + If that fails, then try write-only. */
> + open_flags = O_RDWR;
> + fd = open (dev_map->filename, open_flags);
> + flags |= (IOS_F_READ | IOS_F_WRITE);
> + if (fd == -1)
> + {
> + open_flags = O_RDONLY;
> + fd = open (dev_map->filename, open_flags);
> + if (fd != -1)
> + flags &= ~IOS_F_WRITE;
> + }
> + if (fd == -1)
> + {
> + open_flags = O_WRONLY;
> + fd = open (dev_map->filename, open_flags);
> + if (fd != -1)
> + flags &= ~IOS_F_READ;
> + }
> + if (fd == -1)
> + goto err;
> + }
> +
> + dev_map->fd = fd;
> + dev_map->flags = flags;
> + dev_map->open_flags = open_flags;
> + dev_map->prot = ios_dev_mmap_convert_mmap_prot (open_flags);
> +
> + dev_map->addr = mmap (0, dev_map->size, dev_map->prot, MAP_SHARED,
> + fd, dev_map->base);
> + if (dev_map->addr == MAP_FAILED)
> + {
> + internal_error = errno;
> + goto err;
> + }
> +
> + if (error)
> + *error = IOD_OK;
> +
> + return dev_map;
> +
> +err:
> + if (dev_map)
> + free (dev_map->filename);
> + free (dev_map);
> +
> + if (error)
> + {
> + *error = internal_error;
> + }
We usually prefer to be brief and do not use braces around single
statements, like:
if (error)
*error = internal_error;
Same in other points in the patch.
> + return NULL;
> +}
> +
> +static int
> +ios_dev_mmap_close (void *iod)
> +{
> + struct ios_dev_mmap *dev_map = iod;
> +
> + munmap(dev_map->addr, dev_map->size);
> + close(dev_map->fd);
> +
> + free (dev_map->filename);
> + free (dev_map);
> + return IOD_OK;
> +}
> +
> +static uint64_t
> +ios_dev_mmap_get_flags (void *iod)
> +{
> + struct ios_dev_mmap *dev_map = iod;
> +
> + return dev_map->flags;
> +}
> +
> +static int
> +ios_dev_mmap_pread (void *iod, void *buf, size_t count, ios_dev_off offset)
> +{
> + struct ios_dev_mmap *dev_map = iod;
> +
> + if ((offset + count) > dev_map->size)
> + {
> + return IOD_EOF;
> + }
> +
> + 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;
> +
> + if ((offset + count) > dev_map->size)
> + {
> + return IOD_EOF;
> + }
> +
> + memcpy(dev_map->addr + offset, buf, count);
> + msync(dev_map->addr + offset, count, MS_SYNC);
Please
> +
> + 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;
> +
> + msync(dev_map->addr, dev_map->size, MS_SYNC);
> +
> + return IOS_OK;
> +}
> +
> +struct ios_dev_if ios_dev_mmap =
> + {
> + .get_if_name = ios_dev_mmap_get_if_name,
> + .handler_normalize = ios_dev_mmap_handler_normalize,
> + .open = ios_dev_mmap_open,
> + .close = ios_dev_mmap_close,
> + .pread = ios_dev_mmap_pread,
> + .pwrite = ios_dev_mmap_pwrite,
> + .get_flags = ios_dev_mmap_get_flags,
> + .size = ios_dev_mmap_size,
> + .flush = ios_dev_mmap_flush
> + };
> diff --git a/libpoke/ios.c b/libpoke/ios.c
> index 48719c6a..c88a1c41 100644
> --- a/libpoke/ios.c
> +++ b/libpoke/ios.c
> @@ -103,6 +103,7 @@ extern struct ios_dev_if ios_dev_nbd; /* ios-dev-nbd.c */
> extern struct ios_dev_if ios_dev_proc; /* ios-dev-proc.c */
> #endif
> extern struct ios_dev_if ios_dev_sub; /* ios-dev-sub.c */
> +extern struct ios_dev_if ios_dev_mmap; /* ios-dev-mmap.c */
>
> enum
> {
> @@ -112,6 +113,7 @@ enum
> IOS_DEV_NBD,
> IOS_DEV_PROC,
> IOS_DEV_SUB,
> + IOS_DEV_MMAP,
> IOS_DEV_FILE, /* File must be last */
> };
>
> @@ -131,6 +133,7 @@ static const struct ios_dev_if *ios_dev_ifs[] =
> [IOS_DEV_PROC] = NULL,
> #endif
> [IOS_DEV_SUB] = &ios_dev_sub,
> + [IOS_DEV_MMAP] = &ios_dev_mmap,
> [IOS_DEV_FILE] = &ios_dev_file, /* File must be last */
> };
>
> diff --git a/poke/pk-cmd-ios.c b/poke/pk-cmd-ios.c
> index 1435266f..47dc4719 100644
> --- a/poke/pk-cmd-ios.c
> +++ b/poke/pk-cmd-ios.c
> @@ -576,6 +576,53 @@ pk_cmd_nbd (int argc, struct pk_cmd_arg argv[], uint64_t
> uflags)
> }
> #endif /* HAVE_LIBNBD */
>
> +static int
> +pk_cmd_mmap (int argc, struct pk_cmd_arg argv[], uint64_t uflags)
> +{
> + /* mmap FILENAME BASE SIZE */
> + uint64_t base, size;
> + char *handler;
> + int ios_id;
> +
> + assert (argc == 4);
> + assert (PK_CMD_ARG_TYPE (argv[1]) == PK_CMD_ARG_STR);
> +
> + /* Create a new IO space. */
> + const char *filename = PK_CMD_ARG_STR (argv[1]);
> +
> + assert (PK_CMD_ARG_TYPE (argv[2]) == PK_CMD_ARG_INT);
> + base = PK_CMD_ARG_INT (argv[2]);
> +
> + assert (PK_CMD_ARG_TYPE (argv[3]) == PK_CMD_ARG_INT);
> + size = PK_CMD_ARG_INT (argv[3]);
> +
> + if (access (filename, F_OK) == 0)
> +
> + if (pk_ios_search (poke_compiler, filename, PK_IOS_SEARCH_F_EXACT) != NULL)
> + {
> + printf (_("File %s already opened. Use `.ios IOS' to switch.\n"),
> + filename);
> + return 0;
> + }
> +
> + /* Build the handler for the mmap IOS. */
> + if (asprintf (&handler, "mmap://0x%" PRIx64 "/0x%" PRIx64 "/%s",
> + base, size, filename) == -1)
> + return 0;
> +
> + /* Open the IOS. */
> + ios_id = pk_ios_open (poke_compiler, handler, 0, 1);
> + if (ios_id == PK_IOS_NOID)
> + {
> + pk_printf (_("Error creating mmap IOS %s\n"), handler);
> + free (handler);
> + return 0;
> + }
> + free (handler);
> +
> + return 1;
> +}
> +
> const struct pk_cmd ios_cmd =
> {"ios", "?s", "", 0, NULL, NULL, pk_cmd_ios, ".ios IOS",
> poke_completion_function};
>
> @@ -598,6 +645,10 @@ const struct pk_cmd nbd_cmd =
> {"nbd", "s", "", 0, NULL, NULL, pk_cmd_nbd, ".nbd URI", NULL};
> #endif
>
> +const struct pk_cmd mmap_cmd =
> + {"mmap", "s,i,i", "", 0, NULL, NULL, pk_cmd_mmap, ".mmap FILENAME, BASE,
> SIZE",
> + NULL};
> +
> const struct pk_cmd close_cmd =
> {"close", "s", "", PK_CMD_F_REQ_IO, NULL, NULL, pk_cmd_close,
> ".close [IOS]", poke_completion_function};
> diff --git a/poke/pk-cmd.c b/poke/pk-cmd.c
> index e6825423..ddc1ca85 100644
> --- a/poke/pk-cmd.c
> +++ b/poke/pk-cmd.c
> @@ -48,6 +48,7 @@ extern const struct pk_cmd mem_cmd; /* pk-cmd-ios.c */
> #ifdef HAVE_LIBNBD
> extern const struct pk_cmd nbd_cmd; /* pk-cmd-ios.c */
> #endif
> +extern const struct pk_cmd mmap_cmd; /* pk-cmd-mmap.c */
> extern const struct pk_cmd close_cmd; /* pk-cmd-file.c */
> extern const struct pk_cmd load_cmd; /* pk-cmd-file.c */
> extern const struct pk_cmd source_cmd; /* pk-cmd-ios.c */
> @@ -91,6 +92,7 @@ static const struct pk_cmd *dot_cmds[] =
> #ifdef HAVE_LIBNBD
> &nbd_cmd,
> #endif
> + &mmap_cmd,
> &null_cmd
> };
>
> 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
> +changing them.
> +"
> + };
> +
> pk_help_add_topic
> :entry Poke_HelpEntry {
> category = "dot-commands",