Give reasons for URT_040 and URT_050

[jhauser-us]

The Architecture Review Committee would like the BRS document to include some non-normative justifications for URT_040 and URT_050, which concern the setting and persistence of non-volatile UEFI variables.

[vlsunil]

@jhauser-us : Need some help understanding the feedback better.
URT_040 says - "Non-volatile UEFI variables MUST persist across calls to the ResetSystem() runtime service call."

It looks obvious to me that it merely states that non-volatile variables must be persistent across reboots. What type of clarification you want to see for this?

@andreiw : Was original intent of above requirement to rule out emulated variables on RAM? If so, can we just state it explicitly that only non-volatile variables are supported?

For URT_050 which says:

UEFI runtime services MUST be able to update the UEFI variables directly without the aid of an OS.

Would below additional note be sufficient to clarify?
UEFI variables are normally saved in a dedicated storage which is not directly accessible by the operating system.

[jhauser-us]

It looks obvious to me that it merely states that non-volatile variables must be persistent across reboots. What type of clarification you want to see for this?

I'm not actually the source of the feedback reported in this GitHub issue; I'm just the ARC's messenger. However, my understanding is that URT_040 is demanding that the non-volatile UEFI variables be non-volatile. Isn't that redundant? So why is this rule in our RISC-V BRS standard? That sounds to me like a fair question for a non-normative comment to answer.

[vlsunil]

It looks obvious to me that it merely states that non-volatile variables must be persistent across reboots. What type of clarification you want to see for this?

I'm not actually the source of the feedback reported in this GitHub issue; I'm just the ARC's messenger. However, my understanding is that URT_040 is demanding that the non-volatile UEFI variables be non-volatile. Isn't that redundant? So why is this rule in our RISC-V BRS standard? That sounds to me like a fair question for a non-normative comment to answer.

Yeah. it looks redundant to say so. But I think this requirement just exists to prohibit volatile UEFI variables emulated in RAM which then requires an additional sentence to say so actually. I hope @andreiw or @adurbin-rivos can clarify.

[adurbin-rivos]

I can't speak to the requirement on the variables. I agree that it seems redundant. Likewise, I think the note "UEFI variables are normally saved in a dedicated storage which is not directly accessible by the operating system." seems fine as well.

[jhauser-us]

In the referenced commit (b9383ae), I don't see any explanation given for the apparent redundancy of URT_040, so why is this issue closed?

[vlsunil]

In the referenced commit (b9383ae), I don't see any explanation given for the apparent redundancy of URT_040, so why is this issue closed?

That could be because github automatically closes based on "Fixes" tag in my commit to address URT_050 which was in the same github issue. Sorry for confusion. I have raised a new PR (#208) which removes this requirement since I think it is not required and redundant as you pointed. But it is very likely that I may be missing something which Andrei is aware. Please review the PR #208 (for other issues as well).

[jhauser-us]

The description for commit 53f495b appears to say that the intent of rule URT_040 is that all UEFI variables are non-volatile. If that's the intent, it's not what URT_040 actually says. A proper statement of such a rule would be more like "All UEFI variables MUST be non-volatile." Furthermore, if that's the case, the Architecture Review Committee would like to see a non-normative comment providing some explanation for this requirement.

On the other hand, if we ignore the description for 53f495b and just take URT_040 at its word, the ARC would still like there to be some non-normative explanation for why all UEFI variables must persist across reset, even those that do not survive a loss of power. (Are we even certain that's going to be correct in all circumstances? Do we know for sure there are absolutely no variables that software depends on being removed/zeroed/cleared on reset?)

[xypron]

The requirements in the UEFI specification are:

• "Once ExitBootServices() is performed, only variables that have EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set can be set with SetVariable()" (see 8.2.3 SetVariable()).
• Some runtime variables like BootCurrent, LangCodes are required to be volatile (see 3.3 Globally Defined Variables).
• A non-volatile variable is one that is "preserved across reboots" (see 23.4.1 EFI_SYSTEM_RESOURCE_TABLE).

We should try to conform to the UEFI specification.

[vlsunil]

Thanks @xypron and @jhauser-us . Your feedback makes sense.

The real intention of this extra requirement on top of UEFI, was to avoid the situation where non-volatile variables are persistent only till ExitBootServices(). (#208 (review)).

How about "The UEFI variables with EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE attributes set, MUST be persistent across ResetSystem() call."?

The case where only EFI_VARIABLE_NON_VOLATILE is set would be covered by the UEFI spec anyway via "If the EFI_VARIABLE_NON_VOLATILE attribute is not set, the firmware stores the variable in normal memory and it is not maintained across a power cycle."

It is bit tricky to make a meaningful sentence for this requirement. Let me know if you have better text.

[jhauser-us]

The ARC didn't necessarily have a problem with the original text of URT_040, which said, "Non-volatile UEFI variables MUST persist across calls to the ResetSystem() runtime service call." While the new proposed text might be an improvement that should be adopted, the point of this issue was to "include some non-normative justifications", as I originally wrote at the top. If other implementations have been breaking UEFI rules about non-volatile variables, then that would be an explanation for the existence of URT_040 that otherwise appears to be a redundancy. Or, if the redundancy was never intended, then rewriting the rule is another solution.

[vlsunil]

Okay. Please let me know whether this looks fine.

[jhauser-us]

Unfortunately, this still just looks redundant with the existing requirements of the UEFI spec.

Does any significant UEFI implementation fail to preserve non-volatile variables across ResetSystem()? If not, then I see no evidence we need URT_040 to duplicate what the UEFI spec already requires. On the other hand, if there are such errant implementations, then say that's the reason why the BRS spec includes this apparently redundant rule. According to what I've heard, the latter is the actual truth of the matter, is it not?

[ved-rivos]

@vlsunil perhaps add a statement to the note such as:
"This rule is included in this specification to address a common mistake in implementing the UEFI requirements for non-volatile variables, even though it may appear redundant with the existing UEFI specification."

[vlsunil]

@jhauser-us @ved-rivos : Thanks again for suggestions.

Actually, it is not a redundant requirement but relaxing the UEFI spec requirement a bit so that BRS v1.0 spec can cover some of the existing HW.

UEFI spec has a stricter requirement as below for SetVariable().

The only rules the firmware must implement when saving a nonvolatile variable is that it has actually been saved to
nonvolatile storage before returning EFI_SUCCESS, and that a partial save is not performed. If power fails during a
call to SetVariable() the variable may contain its previous value, or its new value. In addition there is no read, write,
or delete security protection.

However this is not possible in systems which don't have a dedicated NV storage for the FW. So, at OS runtime the NV variables would be still volatile. This may be true for most of the existing RISC-V HW today. So, this requirement URT_040 provides a way for such systems to make sure they remember to flush atleast as part of the ResetSystem() as a workaround.

I think this can be considered as a temporary relaxation in BRS v1.0 and may be removed in BRS v2.0. This may not be perfect but things like RAS etc may not matter for such systems anyway. IMO, it is a good tradeoff.

I was bit hesitant to add every details in the spec because systems should try to adhere to UEFI spec requirement itself and this should be the last resort instead of banking on this.

But looks like it is better to have non-normative text say "This requirement ensures NV variables are persistent across reboots at least even if systems don't persist immediately as part of the SetVariable() update".

I hope it answers your questions how it is different from UEFI spec requirement. We can remove this completely as well (which I thought initially) but most of the RISC-V HW will not be able to either be complaint with UEFI/BRS or can have side effects that NV variables are not flushed across reboots if they don't change ResetSystem() implementation.

Let me know your thoughts and suggestions.

[ved-rivos]

We should not create rules that attempt to suggest ways to subvert the UEFI requirements. As you noted that a lot of implementations make this common mistake of not implementing non-volatile variables using non-volatile storage, it is good to have the rule. However, the rule should not suggest hacks where an operating system may get surprised that SetVariable() returned EFI_SUCCESS but the variable is not persisted. An implementation that does the persistence at ResetSystem() will likely fail some compliance tests.

[jhauser-us]

I agree with @ved-rivos that we should avoid encouraging deviations from the UEFI spec. If some implementors feel they must violate the spec, they will do so. Maybe they can justify this to their customers/users---but it's still a violation. RVI would need an extremely good reason for us to endorse such a violation, even implicitly, and I don't see one here.

On the other hand, I accept that URT_040 may be helpful if it prods those same implementors to at least minimize their deviation from the UEFI spec. I think the text suggested by @ved-rivos, or something similar, gives a reasonable explanation for having a redundant rule without us actually endorsing any bad behavior.

But if that compromise is rejected as too weak, then I will recommend that we siimply remove URT_040 altogether.

[avpatel]

@ved-rivos @jhauser-us The URT_040 is not recommending bad behavior rather it is only relaxing a strict requirement on SetVariable() in UEFI spec. There have been cases where popular real-world platforms have not been able to comply with the strict UEFI requirement for SetVariable(). The rationale here is to have relaxation in BRS v1.0 and we can remove the relaxation in BRS v2.0 if not needed so that platforms can at least comply with BRS v1.0.

The non-normative text suggested by @ved-rivos looks good to me as well.

[jhauser-us]

Relaxing a requirement of the UEFI spec is endorsing implementations that do not strictly conform to the UEFI spec as written. I don't see any other way to interpret it.

I'll run this thought by the rest of the ARC at next week's meeting.

[vlsunil]

I agree with @ved-rivos that we should avoid encouraging deviations from the UEFI spec. If some implementors feel they must violate the spec, they will do so. Maybe they can justify this to their customers/users---but it's still a violation. RVI would need an extremely good reason for us to endorse such a violation, even implicitly, and I don't see one here.

On the other hand, I accept that URT_040 may be helpful if it prods those same implementors to at least minimize their deviation from the UEFI spec. I think the text suggested by @ved-rivos, or something similar, gives a reasonable explanation for having a redundant rule without us actually endorsing any bad behavior.

It makes sense to me. Let me update the BRS spec and send next version addressing all other comments as well so that it can be discussed in the next ARC meeting.