Open GeographicCone opened 1 year ago
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).
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.$
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 :)
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.
Setup
as the default VarStoreNameJust 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.
(
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.
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!
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:
Thank you.
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:
-w
had to be prepended before the value to be written (i.e. no-w
, no write).-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 offset0x0CA9
fromSetup
(backward-compatible)setup_var.efi SaSetup:0x0F(2)
Read a word (two bytes) at offset0x0F
fromSaSetup
setup_var.efi AMITSESetup:0x40=0x01
Write0x01
toAMITSESetup
at offset0x40
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:
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:
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