[flashrom] Giving to -i option some real use

[Salvador Eduardo Tropea]

Hi Michael!

My patch modifies the buil_new_image function. The function is split in 
two cases:

1) The default case is when read_all_first=1. In this case the behavior 
should exactly the same as without the patch. In this case the function 
that replaces build_new_image is called modify_new_image_from_old. This 
function should behave as the old buil_new_image invoked with 
old_image_valid = 1. So this function is some kind of filter that 
modifies the content of "newcontents" using information from 
"oldcontents". Nothing new, just clearer code because it explicitly use 
memcpy  (we know that olcontents contains a copy of the actual memory).
2) When read_all_first=0 (command line --no-read-all or -A) the beahvior 
changes. If the user specifies -A, but doesn't specify at least one 
image with -i an error is reported indicating that at least one image 
must be specified. In this case the buil_new_image function is replaced 
by "read_cur_content". Here is the comment I put on the code:

/**
  * Fill the @oldcontents buffer with data from the memory. The regions 
not included by the user are filled
  * with data from @newcontents instead of the actual memory content. It 
forces to skip them.
  * This function is used only when @oldcontents is empty.
  * Is also mandatory that at least one region was specified by the user.
  */

My intention was to use the same (nasty) tricks that the code already 
uses. By forcing areas to match the rest of the code avoids touching it.
Also note that the change in the behavior is selected using -A, so you 
should get the same old behavior by default. Of course, I could 
introduced some bug, I'm not perfect. But my intention was to change the 
behavior only when the user asked for it. IMHO the default should be 
read_all_first=0, but this will confuse old users. Here are the two 
functions that replace buil_new_image:


/**
  * Modify @newcontents so that it contains the data that should be on 
the chip eventually. In the case the user
  * wants to update only parts of it, copy the chunks to be preserved 
from @oldcontents to @newcontents.
  * This function is used only when @oldcontents contains the full 
memory information.
  */
void modify_new_image_from_old(struct flashctx *flash, const uint8_t 
*oldcontents, uint8_t *newcontents)
{
     unsigned int start = 0;
     romentry_t *entry;
     unsigned int size = flash->chip->total_size * 1024;

     /* If no regions were specified for inclusion, assume
      * that the user wants to write the complete new image.
      */
     if (num_include_args == 0)
         return;

     /* Non-included romentries are ignored.
      * The union of all included romentries is used from the new image.
      */
     while (start < size) {
         entry = get_next_included_romentry(start);
         /* No more romentries for remaining region? */
         if (!entry) { /* Copy the old content to skip it */
             memcpy(newcontents + start, oldcontents + start, size);
             break;
         }
         /* For non-included region, copy from old content. */
         if (entry->start > start)
             memcpy(newcontents + start, oldcontents + start, 
entry->start - start);
         /* Skip to location after current romentry. The user wants to 
preserve it. */
         start = entry->end + 1;
         /* Catch overflow. */
         if (!start)
             break;
     }
     return;
}

/**
  * Fill the @oldcontents buffer with data from the memory. The regions 
not included by the user are filled
  * with data from @newcontents instead of the actual memory content. It 
forces to skip them.
  * This function is used only when @oldcontents is empty.
  * Is also mandatory that at least one region was specified by the user.
  */
int read_cur_content(struct flashctx *flash, uint8_t *oldcontents, const 
uint8_t *newcontents)
{
     unsigned int start = 0;
     romentry_t *entry;
     unsigned int size = flash->chip->total_size * 1024;
     unsigned int region_size;
     int ret = 0;

     /* We will read from memory only the regions indicated by the user.
      * The rest will contain the same as the newcontents */
     memcpy(oldcontents, newcontents, size);

     /* Catch a missuse */
     if (num_include_args == 0) {
         msg_perr("Internal error! build_old_image() invoked, but no 
regions specified.\n");
         return 1;
     }

     /* Non-included romentries are ignored.
      * The union of all included romentries is fetched from memory.
      */
     while (start < size) {
         entry = get_next_included_romentry(start);
         /* No more romentries for remaining region? */
         if (!entry)
             break;
         /* For included regions, read memory content content. */
         if (entry->start == start) {
             msg_gdbg2("Read a chunk 0x%06x-0x%06x.\n", start, entry->end);
             region_size = entry->end - start + 1;
             ret = flash->chip->read(flash, oldcontents + start, start, 
region_size);
             if (ret != 0) {
                 msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", 
start, entry->end);
                 break;
             }
         }
         /* Skip to location after current romentry. */
         start = entry->end + 1;
         /* Catch overflow. */
         if (!start)
             break;
     }
     return ret;
}

Hope that now you have a better idea of what I'm talking about.

Regards, SET

El 19/04/16 a las 17:57, Michael Karcher escribió:
> On 04/19/2016 07:48 PM, Salvador Eduardo Tropea wrote:
>> Hi All!
>>
>> Now I'm using an FPGA board with 4 MiB of flash, was using one with
>> 128 kiB. So now the time to read/write/verify the whole memory is
>> enough to annoy me ;-)
>> So I tried creating a layout like this:
>>
>> <--
>> 00000000:00007e2b fpga
>> 00007e2c:003fffff free
>> <--
>>
>> Note: yes only 32300 bytes used, the rest can be used for other
>> purposes (0,77%).
>>
>> And then I used "-i fpga".
>> But it didn't help ... flashrom read the 4 MiB twice (one at the
>> beggining and another for the verification).
>> Looking at the code I found a variable named read_all_first with a
>> comment about the lack of implementation.
>> So I implemented the needed code to:
>> 1) Add a command line option to make it 0 (avoiding the big reads).
>> This option can be used only when at least one image is indicated with -i
>> 2) Skip the big read when read_all_first is 0. Only the regions
>> indicated by the user are actually fetched from the memory.
>> 3) Same for the verification stage.
> Just a quick question: How does this mix with range erasure? Most
> likely, I can't erase 0-7e2b, but I have to erase 0-8000, or even the
> whole chip. If the chip was not read before erasing, the contents of
> those areas get destroyed. There are use cases for "-i" with the
> intention to keep everything in the chip intact except for the selected
> address range.
> What you need is a way to differentiate between "must not change" and "I
> don't care" areas, but the current layout system provides no way to do
> that. Traditionally "-i" is interpreted as declaring everything outside
> as "must not change", but your description sound like your patch changes
> the interpretation to "I don't care".
>
> Regards,
>    Michael Karcher
>
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>


-- 
Ing. Salvador Eduardo Tropea          http://utic.inti.gob.ar/
INTI - Micro y Nanoelectrónica (CMNB) http://www.inti.gob.ar/
Unidad Técnica Sistemas Inteligentes  Av. General Paz 5445
Tel: (+54 11) 4724 6300 ext. 6919     San Martín - B1650KNA
FAX: (+54 11) 4754 5194               Buenos Aires * Argentina