[Toybox] [PATCH] Support non memory mapped access

[Rob Landley]

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

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

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

Why would you ever need a seek after the write then? Didn't it advance 
to the right place naturally?

Rob