Command-line argument syntax enhancement suggestion

[GeographicCone]

Thank you for your work. The tool runs great for me with no bugs. This is just an enhancement suggestion for consideration. There are two very minor things I noticed:

1. For a clumsy person such as myself, it's easy to write something accidentally, which could be addressed if -w had to be prepended before the value to be written (i.e. no -w, no write).
2. Having to type -s 0x2 instead of even just -s 2 is mildly annoying.

This however got me thinking how the command-line arguments could be rethought in general. What if the syntax were instead:

• setup_var.efi [VarStoreName:]<Offset>[(Size)][=NewValue]

For example:

• setup_var.efi 0x0CA9 Read a byte at offset 0x0CA9 from Setup (backward-compatible)

• setup_var.efi SaSetup:0x0F(2) Read a word (two bytes) at offset 0x0F from SaSetup

• setup_var.efi AMITSESetup:0x40=0x01 Write 0x01 to AMITSESetup at offset 0x40

This would have the added benefit that the tool could iterate through multiple white space-separated commands in one go:

• setup_var.efi SaSetup:0x018D=0x01 SaSetup:0x05=0x01 SaSetup:0x06=0x30 SaSetup:0x0F(2)=0x1250

It's not a contrived example. On the laptop I have, the above sets the memory profile to Custom, reference clock to 100MHz, ratio to ×48, and Refresh Time (tREFI) to 4688.

And in the future, multiple commands could also be read from a file, for example:

• setup_var.efi < my_tweaks.txt

While in an even more distant future, where variables could be set and referenced, the said file could look:

# Define offsets
$MemProfile=SaSetup:0x018D
$MemClock=SaSetup:0x05
$MemRatio=SaSetup:0x06
$MemIntREFI=SaSetup:0x0F(2)

# Apply changes
@MemProfile=0x01
@MemClock=0x01
@MemRatio=0x30
@MemIntREFI=0x1250

I think this would make it slightly more convenient to use (not that it isn't now), and a bit less prone to user error.

Of course I realize it's your project and your time. It's great as it is, and thank you for making it available. This is just food for thought. Hope you don't mind my sharing this idea.

---

Edit: There's one other thing I forgot to mention. With the above argument structure, the output from the program could be exactly in the same format that it would also accept back as the input, for example:

% setup_var.efi SaSetup:0x0F(2)
SaSetup:0x0F(2)=0x1250

This way it could be easily saved to a file:

setup_var.efi SaSetup:0x018D SaSetup:0x05 SaSetup:0x06 SaSetup:0x0F(2) > my_defaults.txt

And then restored:

setup_var.efi < my_defaults.txt

[datasone]

Thanks very much for proposing the reworked cli arguments and I/O file specs! I'm currently working on it, and have some questions on the specs (though I think these are trivial changes and won't affect when the code will be done).

• Is it still necessary to preserve Setup as the default VarStoreName? In newer firmwares it seems the settings are mostly split into multiple varstores, in which Setup is rarely used. And backward compatibility doesn't seem so important as we have changed them all XD. If the Setup varstore isn't used that much nowadays, I think we don't need that default value.
• The way to specify size doesn't fit very well in the assignment like statement, though I can't come up with a better idea, so I think the proposed one is good enough.
• In the input/output file, the addresses are defined with $ symbol prefix and accessed with @ symbol prefix. My thoughts on this is to use the walrus operator := to define, and $ symbol prefix to access (like shell script or rust macro). i.e. Addr:=VarStore:Offset, and $Addr=Value. But I don't know if this is widely used in such tools.

As a developer, my understanding may be biased with normal users (like the walrus operator), and those questions are just subtle design details. So do please feel free to propose your ideas on the questions, I'd like to choose designs that are aligned with widely used designs in similar tools or scripts, which I think you know better than me :)

[GeographicCone]

Thank you for your work! And for even mentioning me in the commit! As if I actually did anything… I did not, it's all your hard work. I really appreciate it.

I'm sorry for the late reply. I've been spending all my free time working on another project, which I've only just completed, more or less.

1. Setup as the default VarStoreName

Just a single data point but with the AMI BIOS of an HP Omen laptop I've been playing with most recently, ~ 36% of all the settings are indeed stored in the Setup variable. From my notes, the breakdown is as follows:

Items   VStId  VarStore        Rd?  Wr?  Persists?  Description

1,646    0x01  Setup           Yes  Yes  Yes        General Settings
1,469    0x07  PchSetup        Yes  Yes  Yes        Platform Controller Host Settings
  608    0x05  SaSetup         Yes  Yes  Yes (No?)  System Agent Settings
  512    0x02  CpuSetup        Yes  Yes  Yes        Processor Settings
  369 LEFT EXCLUDING THE 4 ABOVE
   98    *     *VolatileData   Yes  Yes  No         Changes to Volatile Data Are Overwritten on Reboot, So Not Interesting
   80    *     Usb*            Yes  Yes  ?          USB Settings, Generally Uninteresting
   35    0x09  MeSetup         Yes  Yes  Yes        Management Engine Setup
   13    0x0A  MeSetupStorage  Yes  Yes  Yes        Management Engine Setup [Distinction Unclear]
   13    *     Hide*           Yes  Yes  No         Hide HP-Specific Setup Menu Items, Not Persistent
  130    *     *               Yes  ?    ?          All the Remaining Settings
4,604 TOTAL

It's of course heavily device-dependent. And on top of that, the question is which are the settings that are the most likely to be accessed and changed. In the case of this laptop, it'd most likely be SaSetup anyway (for the memory settings).

So I agree with you it's not necessary to have a default.

2. Size in parentheses: ( and )

Come to think of it, maybe it would be more logical to use square brackets: [ and ] for this purpose as that's the convention for specifying an array in many programming languages. In my defense, I'm a creature of habit and when I was writing this I had already implemented such a syntax in a tool for reading from and writing to the Embedded Controller, i.e. the command-line arguments are -Ec HPCM RPM1(2) RPM3(2) and the output is:

Embedded Controller (-Ec)
- Register 0x95 Byte: 0x31 = 0b00110001 = 49 [HPCM]
- Register 0xb0 Word: 0x089c = 0b0000100010011100 = 2204 (Little-Endian) [RPM1]
- Register 0xb2 Word: 0x07ee = 0b0000011111101110 = 2030 (Little-Endian) [RPM3]

So I just reapplied my earlier idea.

3. Variable setting and referencing

You make a very good point the $ sign is mostly used for referencing variables already defined earlier as in UN*X sh(1), so my original suggestion goes against the convention.

I agree $ should be used for referencing instead.

But I'm not a fan of the walrus in a definition for two reasons: (1) it adds an extra character, which I think is unnecessary when we could just use any arbitrary single character instead, and (2) it's much more readable (in my opinion) to have a definition start with a special character like @. Looking at many such rows in a text file, I can immediately see which one is a definition and which one isn't, even without syntax highlighting.

---

I'm not very much a "normal user" myself to be honest. And, most importantly, you're the author, so you have the final say. My use case now is that I'm trying to experiment with the settings on a device (the HP Omen laptop I mentioned) where I can only change them with the help of a tool like yours.

It already works great for this purpose, all I could wish for is to make it a tad more efficient in changing multiple settings at a time, which is why I made this suggestion. I don't really insist on any specific details, it's just food for thought. That being said, I think what I came up with is generally not a bad idea. 😀

From the other end, I'd have a specially-formatted Setup Internal Forms Representation dump so that I can just pick the settings I want changed, write the values, and pass it to your utility. In fact, a couple of days after I posted this issue, I already wrote a tool that does what I had in mind: https://github.com/GeographicCone/SlimIFR/

Now, "wrote" is maybe saying too much, it's just a bunch of regular expressions thrown in, and I based it on a script that already existed, but it comes up with output like this: one setting per line, ready to add =s and feed into your tool:

AMITSESetup:0x0040               Boot: Quiet Boot [0x00 / 0x01]
MeSetup:0x0003                   Firmware Update Configuration: Me FW Image Re-Flash [0x00 - Disabled, 0x01 - Enabled]
SaSetup:0x000F(0x02)             Memory Overclocking Menu: tREFI [0x0000 - 0xFFFF]

A longer example: https://github.com/GeographicCone/SlimIFR/blob/master/Example-Output.txt

So that's how I'd see it being used.

Thanks again!

[GeographicCone]

Updated 2023-11-18: All issues fixed and the source is at: https://github.com/GeographicCone/UefiVarTool as a fork with many other changes. I really appreciate everything you did, I hope the credit I gave you as the original author is appropriate. I also made you a co-author on the initial commit.

I'd be glad to resume the discussion if you ever get the time but meanwhile, the immediate problem I had is solved, and the solution to it is available to everyone. So this is just a heads-up my original comment below can be disregarded now, although I'm still leaving it for reference:

---

I'm trying to use the new setup_var.efi version now from the master branch in the repository. Looks very promising. Thank you again for considering and implementing my ideas.

I wanted to ask if it's completed as is, or is it work in progress.

The reason I ask is it doesn't really seem to work out of the box for me. But then maybe it's my setup or environment (for the record, it's Shell v2.2 from edk2-stable202305).

​1. When passed command-line arguments, it'd for some reason try to parse its own name as an argument and fail. Looking at the code, I found the culprit at src/args.rs:236 in drop_first_arg() where it's treating the first argument as an option to parse, unless it ends with .efi\0. Thing is, the way I use it, in /efi/boot/startup.nsh I have an alias -v v "setup_var.efi" and I'd usually just call setup_var.efi as v only. Even without an alias, I'd still call setup_var without explicitly specifying the extension, which fails as well. It took a peek into the code to find out what was going on.

I'm not really sure about the use case when argv[0] (or whatever the Rust equivalent for that is) should be parsed as an argument: I'd have expected it'd always be the process's own name, but in any case, in my opinion it shouldn't make the assumption that it is not unless it has that extension. At the very least I think it should also check its own basename (w/o extension) and possibly also the aliases. In my compiled version I've changed that from the default false to true so that I could get past that hurdle.

​2. It doesn't care about stdin, just shows usage information as if there were no input whatsoever. After changing src/args.rs:260 in parse_args_from_str() from opt_len == 0 to 1 (although it's not a complete fix because following the change, it doesn't show usage information when started with neither arguments nor stdin anymore), it will now parse the stdin but that fails with an error:

setup_var.efi < test.txt

Error parsing input file:
Unexpected value:

Similarly, with: setup_var.efi <a test.txt (to force ASCII mode)

Error parsing input file:
Unexpected value: [Unprintable Character]

The unprintable character is likely the BOM 0xFEFF.

The different test.txt input files I tried had the following content:

Setup:0x1

Setup:0x1=0x1

As well as the following, to exclude the case when the BOM (mandatory per the EFI Shell Specification as I understand) could possibly be messing up the first input line:

#
Setup:0x1

#
Setup:0x1=0x1

The error messages were the same in all scenarios.

The file format was UTF-16 LE w/BOM (which should be compatible with UCS-2 mandated by the EFI spec). I also tried ASCII and UTF-16 BE w/BOM (just to be sure). The EFI shell seems to process ASCII and UTF-16 LE files normally, and I can redirect stdout, no problem with that, so I don't think it's a shell issue.

I also tried piping in the input:

echo Setup:0x1 | setup_var.efi
echo Setup:0x1 |a setup_var.efi
echo Setup:0x1=0x1 | setup_var.efi
echo Setup:0x1=0x1 |a setup_var.efi

Here there is no output at all, no error message – with the version I modified, otherwise it just shows usage information in all these cases.

In the first step, the problem seems to be that for stdin to be parsed, parse_input() must be called, which happens in src/main.rs:31 only if parse_args_from_str(), called via parse_args(), returns ParseError::NoArgs in src/args.rs:261. But this can never happen if the first argument is the executable name, even if drop_first_arg() returned true, which would explain why it would not work out of the box for me: the way it gets executed in my environment, the first argument is always going to be the executable name.

​3. While it does work with both querying and assignment, even with multiple arguments, it doesn't seem to be able handle size specification. For example:

setup_var.efi Setup:0x1(2)
Error parsing arguments:
Unexpected value: Setup:0x1(2): Unexpected value: 2)

The results are similar for "Setup:0x1(2)" and Setup:0x1^(2^), not that it should need to be escaped, I think.

The culprit is somewhere in parse_value_arg() in src/args.rs:437 but I stopped looking to ask this question, partly because it's difficult for me to tell what exactly is going on there: maybe it's because I'm not proficient in Rust but what also doesn't help is that there are 20+ locations where ParseError::InvalidValue can be triggered. So the "Unexpected value" prompt has little informational value.

---

I see you have tests for all those things that are not working for me. So I guess they are working for you? I'm not familiar with Rust. Maybe it's down to the toolchain I'm using?

• cargo 1.73.0 (9c4383fb5 2023-08-26)
• rustc 1.73.0 (cc66ad468 2023-10-03)
• rustup 1.26.0 (5af9b9484 2023-04-05)

I really don't understand how come I am running into so many issues with this, considering the code looks complete.

I'd like to get the new version working now that you wrote it, and made it available. I'm not expecting support or anything but if I were to fix this myself I think it'd end up with a major rewrite, which isn't really what I've been aiming for. So I wanted to ask first:

• If it's supposed to work as it is now, can you tell me what toolchain you use to compile it yourself, and which shell build it runs with? Or, if it's work in progress, which you're too busy to finish, I'd like to confirm this.

Thank you.