Project

General

Profile

Actions
Copy link

Bug #520

open

Factor out verification from erase path

Added by Vincent Fazio 11 months ago. Updated 3 months ago.

Status:
New
Priority:
Normal
Assignee:
Anastasia Klimchuk
Category:
-
Target version:
none
Start date:
12/19/2023
Due date:
% Done:

0%

Estimated time:
Affected versions:
1.3
Needs backport to:
Affected hardware:
Affected OS:

Description

Currently, flashrom_image_erase differs from flashrom_image_write in how it performs its verification.

When writes are completed, verification occurs afterwards per the verify flags specified in the flashctx argument.

    /* Verify only if we actually changed something. */
    if (verify && !all_skipped) {
        msg_cinfo("Verifying flash... ");

        /* Work around chips which need some time to calm down. */
        programmer_delay(flashctx, 1000*1000);

        if (verify_all)
            combine_image_by_layout(flashctx, newcontents, oldcontents);
        ret = verify_by_layout(flashctx, verify_layout, curcontents, newcontents);

For the erase path, there is no post-operation verification. Instead, check_erased_range is called regardless of verify flags, performing verification even if the user doesn't request it and imposing a performance penalty.

Actions
Copy link
 #1

Updated by Anastasia Klimchuk 10 months ago

  • Assignee set to Anastasia Klimchuk
Actions
Copy link
 #2

Updated by Anastasia Klimchuk 10 months ago

Hello Vincent,

I looked into the code of legacy write/erase, and it does the same thing: verification after write respects flags, verification after erase is always done (i.e. ignores flags).
Which is not a great excuse if behaviour is wrong, so looking further,

The code that defines and documents the existing flag, which is -v also known as verify flash against <file> or the content provided on the standard input.
But this flag is only used to detect the "standalone" verify operation against the given file.

The flag which is used to detect verify_after_write is actually -n or dont_verify_it, which makes sense because it should verify by default. It is described as Skip the automatic verification of flash ROM contents after writing. which is consistent with the name.
So the name and description says it's for writing operation only, and the code is consistent.

I don't think that we can change the implementation of the existing flag which has been around for ages.
We can however, add a new flag for erase operation specifically.

What is the exact use case for how you would use the new flag? Do you erase a lot of chips with the script?

Actions
Copy link
 #3

Updated by Vincent Fazio 10 months ago

Anastasia Klimchuk wrote in #note-2:

The flag which is used to detect verify_after_write is actually -n or dont_verify_it, which makes sense because it should verify by default. It is described as Skip the automatic verification of flash ROM contents after writing. which is consistent with the name.
So the name and description says it's for writing operation only, and the code is consistent.

I don't think that we can change the implementation of the existing flag which has been around for ages.
We can however, add a new flag for erase operation specifically.

What is the exact use case for how you would use the new flag? Do you erase a lot of chips with the script?

I worded this a bit weird and it should be clarified. It's been a while since I thought about this so I'm trying to jog my own memory...

I think some of this is a semantic thing. I interpret "writing" to a flash chip any action which commits new values to be persisted, so an erase operation is a "write" because it's flipping bits on flash.

I think the biggest issues are:

  • when a user says "do not verify these writes", erasures are verified anyway. Regardless of call path (erase or write), check_erased_range performs verification after blocks are erased and there is no shield from this.
    • For the flashrom_image_write call path, this means we're performing verification on erase and then performing verification after the writes occur (so we're verifying twice) unless dont_verify_it is specified, but this doesn't shield verifying erasures
    • For the flashrom_image_erase call path, we only perform the forced verification via check_erased_range
  • If an erase is unaligned, there is a subsequent write to fill in data that should not have been erased. Some write functions implement a verify_range call, but not all functions look to verify the write was persisted (in fact, it only looks like the WRITE_JEDEC{1} verifies writes). flashrom_image_write can get away with this because it performs a post-write verification (unless told otherwise) and can spot a problem, however this is not true for flashrom_image_erase as it only verifies on erase and implements whatever checks are intrinsic to the device write function, which may be nothing.

Of course, i may have misread the code, but this is what it looks like to me.

As for the CLI changes, I realize this wouldn't happen for a while, but large API/interface changes and CLI changes to redefine flags should be allowed for major version bumps, otherwise legacy codebases are stuck maintaining old code forever. Adding new flags don't require this however.

Actions
Copy link
 #4

Updated by Anastasia Klimchuk 9 months ago

  • Affected versions 1.3 added

Vincent, so sorry for the long delay in reply! But I was thinking about this in the background.

I have just re-read everything, and re-read the code too. I agree that something can be done.

I think we have two main options:

1) Change the meaning of the existing flag -n (dont_verify_it) to also cover erase operation, in addition to covering write.
2) Introduce a new flag which would mean "dont_verify_erase" and would cover exclusively erase. The meaning of the old flag does not change.

It's a good question whether erase counts as writing. Maybe you are right, I can agree that it makes sense.

Am I right that you like option 1 more?

With the choice of options, I am thinking of starting a thread on the mailing list, to see what other regular users think, there may be a bunch of different use cases for how people run flashrom.
The topic can be titled something like "consistency of verification after erase vs write operations"
If there are no responses on the thread, which means no strong opinions either way, we will decide between us two. Would you agree on the plan? And thank you!

Actions
Copy link
 #5

Updated by Vincent Fazio 9 months ago

Anastasia Klimchuk wrote in #note-4:

Am I right that you like option 1 more?

Yes, I think I personally prefer option 1. However, I understand it changes semantics a bit and may need to be behind some larger version bump that denotes a compatibility break with the previous version.

Actions
Copy link
 #6

Updated by Anastasia Klimchuk 9 months ago

I started a thread "Verification after write vs erase operations" on the mailing list.

Actions
Copy link
 #7

Updated by Anastasia Klimchuk 8 months ago

It looks like the option to cover erase operation by existing flag (option 1) will be the way to go.

I think I can produce a patch for this, and see how it goes. However I would do this after https://ticket.coreboot.org/issues/525 is fixed, it looks like a real issue and the fix will change the same code. So once 525 is fixed (which will take some time, mainly because of testing) I will produce a patch for this and add you as reviewer!
Alternatively, if you want, you are very welcome create a patch yourself, but keep in mind that 525 will change the code, so maybe after it.

Actions
Copy link
 #8

Updated by Anastasia Klimchuk 3 months ago

I am currently working on a detailed test for verify operation https://review.coreboot.org/c/flashrom/+/84078

Once it is done, the feature can be implemented, together with tests (flag on / off)

Actions
Copy link

Also available in: Atom PDF