[Toybox] [PATCH] Support non memory mapped access

[Karthik Ramasubramanian]

On Fri, Oct 25, 2024 at 10:06 AM Rob Landley <rob at landley.net> wrote:

> On 10/24/24 09:45, enh wrote:
> > On Wed, Oct 23, 2024 at 7:02 PM Karthikeyan Ramasubramanian <
> > kramasub at google.com> wrote:
> >
> >> Add --no-mmap flag to indicate seek and read/write access. This allows
> >> accessing devices that do not support mapping into memory - eg.
> >> /dev/nvram, /dev/msr0 etc.
> >>
> >> Also currently only WIDTH bytes are mapped into memory even when more
> >> data is accessed. Fix this by mapping WIDTH * number of data.
> >>
> >> Test: ./post_update.sh && m toybox. Push devmem test into DUT and access
> >> /dev/mem through memory mapped access, /dev/nvram & /dev/msr* through
> >> non memory-mapped access.
> >>
> >> ---
> >>   android/device/generated/flags.h   |  8 +++--
> >>   android/device/generated/help.h    |  2 +-
> >>   android/device/generated/newtoys.h |  2 +-
> >>   android/linux/generated/flags.h    |  8 +++--
> >>   android/linux/generated/help.h     |  2 +-
> >>   android/linux/generated/newtoys.h  |  2 +-
> >>   android/mac/generated/flags.h      |  8 +++--
> >>   android/mac/generated/help.h       |  2 +-
> >>   android/mac/generated/newtoys.h    |  2 +-
> >>
> >
> > (these files only exist in AOSP ... sorry, i should have said to apply
> your
> > devmem.c change to a fresh `git clone` of upstream!)
>
> I already stripped the android files off and applied it locally
> yesterday, although that was mostly so I could go:
>
>    git diff -b HEAD^1 HEAD
>
Sorry that I added android files. I will remove them in the next patch
revision.

>
> I left off yesterday looking at:
>
> -    map_len = (addr+bytes-map_off);
> +      map_len = addr + (writing ? (toys.optc - 2) * bytes : bytes) -
> map_off;
>
> Which presumably corresponds to the commit comment section:
>
>      Also currently only WIDTH bytes are mapped into memory even when
>      more data is accessed. Fix this by mapping WIDTH * number of data.
>
> And I'm going... that's unrelated to the rest of the patch and seems
> like a user interface change? If nobody noticed this before now, what's
> going on here?
>
> Looking at it today, I _think_ the issue is that mmap() gets rounded up
> to page size and if you either do a lot of data at a size larger than
> bytes, or do an unaligned access that goes off the edge of the page,
> this could go boing?
>
> >>     // Not using peek()/poke() because registers care about size of
> >> read/write.
> >>     if (writing) {
> >>       for (int i = 2; i < toys.optc; i++) {
> >>         data = xatolu(toys.optargs[i], bytes);
> >> -      if (bytes==1) *(char *)p = data;
> >> -      else if (bytes==2) *(unsigned short *)p = data;
> >> -      else if (bytes==4) *(unsigned int *)p = data;
> >> -      else if (sizeof(long)==8 && bytes==8) *(unsigned long *)p = data;
> >> -      p += bytes;
> >> +      if (FLAG(no_mmap)) {
> >> +        xwrite(fd, &data, bytes);
> >> +        xlseek(fd, bytes, SEEK_CUR);
> >
> > is this clearer if you remove the xlseek at the top, and move this
> xlseek()
> > before the xwrite()? i know that would mean incrementing addr, but at
> least
> > the reasoning is localized (and you don't have (a) "aren't these the
> wrong
> > way round?" and (b) "do we actually need a SEEK_CUR? why didn't the write
> > advance the file offset?").
>
> It's a pity there aren't any tests using "-f filename" to modify a local
> file created and checked with xxd.
>
> (Also, xxd is not in the debian base install but the test suite is now
> full of uses of it, meaning "TEST_HOST=1 make tests" doesn't pass in a
> fresh debootstrap. Sigh, gotta document dependencies. New FAQ entry on
> setting up a testing debootstrap I guess...)
>
> My problem here is I dunno what the expected use case is: do you
> normally read lots of things in sequence in a single invocation, or or
> scattered around? My mental model before this was you run the command
> once per read/write pair and stick a shell for loop around it if you
> wanted anything fancier, neither of which would require a loop or more
> than one lseek(), but apparently not...
>
The sequential use-case is:
devmem -f ${path_to_file} ADDR WIDTH DATA1 DATA2 .. DATAn
where each DATA is of size WIDTH bytes. Also this sequential use-case is
currently supported only for write access. The sequential read access
use-case
is currently not supported in devmem. This sequential use-case supports
writing to WIDTH * n contiguous bytes in a file/device.

>
>    usage: devmem [-f FILE] ADDR [WIDTH [DATA...]]
>
>    Read/write physical addresses. WIDTH is 1,2,4, or 8 bytes (default 4)
>    Prefix ADDR with 0x for hexadecimal, output in same base as address.
>
> How would one specify two disjunct read/write addresses? There's no
> square bracket around "ADDR". And how would you specify multiple widths?

Multiple widths or addresses cannot be specified in a single invocation.
For that,
one has to invoke each address and width scenario separately.

>
> It just seems like you specify one address, and then MAYBE specify one
> width, and then specify data. And if you don't specify anything (since
> everything in square brackets is presumably optional" it does SOMETHING,
> presumably read and display one 4 byte value at the address?

Yes, that is correct.

>
> Why would you ever need a seek after the write then? Didn't it advance
> to the right place naturally?
>
That is again my bad. I confirmed that with this extra xlseek, it advanced
to
unexpected offset and accessed incorrect bytes. I will fix this in the next
patch revision. I will also update the test case to test this scenario as
well.

>
> Rob
>
Thanks and Regards,
Karthikeyan.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20241025/76257889/attachment-0001.htm>