<div dir="ltr"><div dir="ltr"><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 25, 2024 at 10:06 AM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 10/24/24 09:45, enh wrote:<br>
> On Wed, Oct 23, 2024 at 7:02 PM Karthikeyan Ramasubramanian <<br>
> <a href="mailto:kramasub@google.com" target="_blank">kramasub@google.com</a>> wrote:<br>
> <br>
>> Add --no-mmap flag to indicate seek and read/write access. This allows<br>
>> accessing devices that do not support mapping into memory - eg.<br>
>> /dev/nvram, /dev/msr0 etc.<br>
>><br>
>> Also currently only WIDTH bytes are mapped into memory even when more<br>
>> data is accessed. Fix this by mapping WIDTH * number of data.<br>
>><br>
>> Test: ./post_update.sh && m toybox. Push devmem test into DUT and access<br>
>> /dev/mem through memory mapped access, /dev/nvram & /dev/msr* through<br>
>> non memory-mapped access.<br>
>><br>
>> ---<br>
>>   android/device/generated/flags.h   |  8 +++--<br>
>>   android/device/generated/help.h    |  2 +-<br>
>>   android/device/generated/newtoys.h |  2 +-<br>
>>   android/linux/generated/flags.h    |  8 +++--<br>
>>   android/linux/generated/help.h     |  2 +-<br>
>>   android/linux/generated/newtoys.h  |  2 +-<br>
>>   android/mac/generated/flags.h      |  8 +++--<br>
>>   android/mac/generated/help.h       |  2 +-<br>
>>   android/mac/generated/newtoys.h    |  2 +-<br>
>><br>
> <br>
> (these files only exist in AOSP ... sorry, i should have said to apply your<br>
> devmem.c change to a fresh `git clone` of upstream!)<br>
<br>
I already stripped the android files off and applied it locally <br>
yesterday, although that was mostly so I could go:<br>
<br>
   git diff -b HEAD^1 HEAD<br></blockquote><div>Sorry that I added android files. I will remove them in the next patch revision. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I left off yesterday looking at:<br>
<br>
-    map_len = (addr+bytes-map_off);<br>
+      map_len = addr + (writing ? (toys.optc - 2) * bytes : bytes) - <br>
map_off;<br>
<br>
Which presumably corresponds to the commit comment section:<br>
<br>
     Also currently only WIDTH bytes are mapped into memory even when<br>
     more data is accessed. Fix this by mapping WIDTH * number of data.<br>
<br>
And I'm going... that's unrelated to the rest of the patch and seems <br>
like a user interface change? If nobody noticed this before now, what's <br>
going on here?<br>
<br>
Looking at it today, I _think_ the issue is that mmap() gets rounded up <br>
to page size and if you either do a lot of data at a size larger than <br>
bytes, or do an unaligned access that goes off the edge of the page, <br>
this could go boing?<br>
<br>
>>     // Not using peek()/poke() because registers care about size of<br>
>> read/write.<br>
>>     if (writing) {<br>
>>       for (int i = 2; i < toys.optc; i++) {<br>
>>         data = xatolu(toys.optargs[i], bytes);<br>
>> -      if (bytes==1) *(char *)p = data;<br>
>> -      else if (bytes==2) *(unsigned short *)p = data;<br>
>> -      else if (bytes==4) *(unsigned int *)p = data;<br>
>> -      else if (sizeof(long)==8 && bytes==8) *(unsigned long *)p = data;<br>
>> -      p += bytes;<br>
>> +      if (FLAG(no_mmap)) {<br>
>> +        xwrite(fd, &data, bytes);<br>
>> +        xlseek(fd, bytes, SEEK_CUR);<br>
> <br>
> is this clearer if you remove the xlseek at the top, and move this xlseek()<br>
> before the xwrite()? i know that would mean incrementing addr, but at least<br>
> the reasoning is localized (and you don't have (a) "aren't these the wrong<br>
> way round?" and (b) "do we actually need a SEEK_CUR? why didn't the write<br>
> advance the file offset?").<br>
<br>
It's a pity there aren't any tests using "-f filename" to modify a local <br>
file created and checked with xxd.<br>
<br>
(Also, xxd is not in the debian base install but the test suite is now <br>
full of uses of it, meaning "TEST_HOST=1 make tests" doesn't pass in a <br>
fresh debootstrap. Sigh, gotta document dependencies. New FAQ entry on <br>
setting up a testing debootstrap I guess...)<br>
<br>
My problem here is I dunno what the expected use case is: do you <br>
normally read lots of things in sequence in a single invocation, or or <br>
scattered around? My mental model before this was you run the command <br>
once per read/write pair and stick a shell for loop around it if you <br>
wanted anything fancier, neither of which would require a loop or more <br>
than one lseek(), but apparently not...<br></blockquote><div>The sequential use-case is:</div><div>devmem -f ${path_to_file} ADDR WIDTH DATA1 DATA2 .. DATAn</div><div>where each DATA is of size WIDTH bytes. Also this sequential use-case is</div><div>currently supported only for write access. The sequential read access use-case</div><div>is currently not supported in devmem. This sequential use-case supports</div><div>writing to WIDTH * n contiguous bytes in a file/device.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
   usage: devmem [-f FILE] ADDR [WIDTH [DATA...]]<br>
<br>
   Read/write physical addresses. WIDTH is 1,2,4, or 8 bytes (default 4)<br>
   Prefix ADDR with 0x for hexadecimal, output in same base as address.<br>
<br>
How would one specify two disjunct read/write addresses? There's no <br>
square bracket around "ADDR". And how would you specify multiple widths?</blockquote><div>Multiple widths or addresses cannot be specified in a single invocation. For that,</div><div>one has to invoke each address and width scenario separately.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br>
It just seems like you specify one address, and then MAYBE specify one <br>
width, and then specify data. And if you don't specify anything (since <br>
everything in square brackets is presumably optional" it does SOMETHING, <br>
presumably read and display one 4 byte value at the address?</blockquote><div>Yes, that is correct. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Why would you ever need a seek after the write then? Didn't it advance <br>
to the right place naturally?<br></blockquote><div>That is again my bad. I confirmed that with this extra xlseek, it advanced to</div><div>unexpected offset and accessed incorrect bytes. I will fix this in the next</div><div>patch revision. I will also update the test case to test this scenario as well.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Rob<br></blockquote><div>Thanks and Regards,</div><div>Karthikeyan. </div></div></div>