Laszlo,
I was thinking that we could add something like this up front in the file and then we could remove lots of the pointer math from the main code part to make the logical part of the code easier to understand.
#define BCFG_VAR_BUFFER_ATTS (x) (((EFI_LOAD_OPTION)(x))->Attributes) #define BCFG_VAR_BUFFER_LEN (x) (((EFI_LOAD_OPTION)(x))->FilePathListLength) #define BCFG_VAR_BUFFER_DESC (x) ((CHAR16*)(((UINT8)(x))+sizeof(UINT32)+sizeof(UINT16)) #define BCFG_VAR_BUFFER_DPATH(x) (((UINT8)(x))+sizeof(UINT32)+sizeof(UINT16)+StrSize(BCFG_VAR_BUFFER_DESC(x))) #define BCFG_VAR_BUFFER_OPT (x) (BCFG_VAR_BUFFER_DPATH(x)+BCFG_VAR_BUFFER_LEN(x))
-Jaben
-----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, January 20, 2016 1:26 PM To: Ryan Harkin ryan.harkin@linaro.org; Carsey, Jaben jaben.carsey@intel.com Cc: edk2-devel@lists.01.org edk2-devel@ml01.01.org; Linaro UEFI Mailman List linaro-uefi@lists.linaro.org Subject: Re: [edk2] Shell BCFG command - adding option data Importance: High
On 01/20/16 21:20, Ryan Harkin wrote:
On 20 January 2016 at 19:52, Carsey, Jaben jaben.carsey@intel.com
wrote:
Short help to long answer to long question.
First you must enable verbose (use -v) output to see the optional data.
I already tried using -v but I've never seen the optional data....
Indeed.
That's a separate bug, in the following call:
ShellPrintEx( -1, -1, NULL, L"%02x", Buffer[LoopVar2]);This function call has a copy & paste error. It must have been copied from the function call just above, still visible here:
Second - I agree something is wrong with that N saying there isn't any.
The question is why that math thinks that there is no more data in the
buffer.
The output you're looking for is this one: ShellPrintHiiEx( -1, -1, NULL, STRING_TOKEN(STR_BCFG_LOAD_OPTIONS), gShellBcfgHiiHandle, LoopVar, VariableName, (CHAR16*)(Buffer+6), DevPathString, (StrSize((CHAR16*)(Buffer+6)) + *(UINT16*)(Buffer+4) + 6) <=
BufferSize?L'N':L'Y');
The ShellPrintHiiEx() function has the following prototype:
EFI_STATUS EFIAPI ShellPrintHiiEx( IN INT32 Col OPTIONAL, IN INT32 Row OPTIONAL, IN CONST CHAR8 *Language OPTIONAL, IN CONST EFI_STRING_ID HiiFormatStringId, IN CONST EFI_HANDLE HiiFormatHandle, ... );
but the ShellPrintEx() function not only drops "HiiFormatStringId" and "HiiFormatHandle", it *also* drops the "Language" parameter:
EFI_STATUS EFIAPI ShellPrintEx( IN INT32 Col OPTIONAL, IN INT32 Row OPTIONAL, IN CONST CHAR16 *Format, ... )
Therefore, the NULL parameter must be removed from the call, like this:
ShellPrintEx( -1, -1, L"%02x", Buffer[LoopVar2]); }Otherwise NULL gets passed as Format, and the string intended as format string, and the arguments for that intended format string, are both ignored by ShellPrintEx().
Thanks Laszlo
Thanks for confirming.
-Jaben
-----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Wednesday, January 20, 2016 11:48 AM To: Ryan Harkin ryan.harkin@linaro.org; edk2-devel@lists.01.org
<edk2-
devel@ml01.01.org> Cc: Linaro UEFI Mailman List linaro-uefi@lists.linaro.org Subject: Re: [edk2] Shell BCFG command - adding option data Importance: High
Long answer to long question ahead :)
On 01/20/16 19:54, Ryan Harkin wrote:
Hello all,
I'm using Shell.efi, a magic binary that is included in my aarch64 FVP
binary.
Not a magic binary, except if your platform DSC relies on the prebuilt binary that is (for some unfathomable reason) checked into the source
tree.
OvmfPkg and ArmVirtPkg both build the UEFI shell from source. Please grep their respective DSC files for "Shell.inf".
This is exactly so that we can immediately see changes to the shell. (Plus it isn't really open source if you just embed the shell binary, despite the shell source residing in the exact same tree.)
Thanks for the pointers. I remember a long time ago that Olivier specifically changed ArmPlatformPkg to use prebuilt binaries for Shell. I *thought* it was because the source was removed from the repo, or something like that. But clearly it wasn't... I'll start building from source so I can debug.
Around those parts you will find the pathname that leads to the shell's source.
I'm trying to manipulate boot options using the BCFG command. And I've failed to work out how to set optional data.
Before I clone ShellPkg and trawl through the code, I thought I'd post here and see if someone can tell me what's going wrong.
First step, I created a boot option to run Shell.efi using the Intel BDS menu system. Thanks to Lazlo fixing cursor key support ;-)
You're welcome, but only if next time you'll spell my name right! ;)
It's L-a-s-z-l-o, and apparently it is the bane of native English speakers. Sorry about that. (In reality, it's László, but let's not go there. :)) You are not the first and not the last to misspell it, so I'm not even annoyed; just mildly amused. :)
Oh crud!! I know I'd made it up and meant to check the correct spelling before sending. Obviously, I didn't :-/
Sorry. I owe you a beer!
I added some simple optional data, "1 2 3 4".
Next, I deleted all the other boot entries that were created by default, leaving only my new boot option.
I created the option again, without optional data, giving it the description "xxxxxxxx" instead of "Shell.efi" (they're the same length) and here's what I see:
Then I started the first new Shell.efi option. It gives an error that is hardly unexpected:
'2' is not recognized as an internal or external command, operable program, or script file.
So I ask bcfg to list my boot options and it says:
Shell> bcfg boot dump Option: 00. Variable: Boot0006 Desc - Shell.efi DevPath - Fv(87940482-FC81-41C3-87E6-399CF85AC8A0)/\Shell.efi Optional- N Option: 01. Variable: Boot0000 Desc - xxxxxxxxx DevPath - Fv(87940482-FC81-41C3-87E6-399CF85AC8A0)/\Shell.efi Optional- N
It claims there is no optional data.
Yup, that's a bug.
In such cases we usually hunt down the format string, and go from there. Therefore one would grep for "Optional-".
Now, until a few months ago, this would have been made more tricky by the fact such format strings were stored in separate .UNI files (resource strings, practically, to be used together with HII). Because, those were encoded in UCS-2, hence for grepping them you needed a
handy
little script like
#!/bin/bash set -u -C
for F in $(git ls-files '*.uni'); do iconv -f UTF-16LE -t UTF-8 -- "$F" \ | grep -H --label="$F" --color=auto "$@" done
But, Jordan recently converted all (or most?) such UNI files to UTF-8, which helps us non-Windows-users keep our sanity, and helps git post plaintext (non-binary) patches for UNI files.
Now, the "Optional-" string can be found in the file
ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.uni
under the token name
STR_BCFG_LOAD_OPTIONS
You will see some strange format specifiers in that string, %B and %N for example. The solution to the riddle is the leading comment of the ShellPrintHiiEx() function, file "ShellPkg/Library/UefiShellLib/UefiShellLib.c". It explains that %B sets "blue" and %N sets "normal" output attributes.
Anyway, we can now grep for STR_BCFG_LOAD_OPTIONS. It yields the BcfgDisplayDump() function, in file
ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
The printing call is (breaking it up here for better readability):
ShellPrintHiiEx( -1, -1, NULL, STRING_TOKEN(STR_BCFG_LOAD_OPTIONS), gShellBcfgHiiHandle, LoopVar, VariableName, (CHAR16*)(Buffer+6), DevPathString, (StrSize((CHAR16*)(Buffer+6)) + *(UINT16*)(Buffer+4) + 6) <= BufferSize ? L'N' : L'Y' );The important part is the last (huge) expression, which fills in the "Y" or "N" character for the %c in the format string. (Ie. if there is optional data or not.)
Yes, found the source and I'd just got to this part, thought "Oh no", and then dinner was ready, so I made a lucky escape.
Now, Buffer is a (UINT8*) pointer to the beginning of the binary data underlying the Boot#### variable. The structure of these is EFI_LOAD_OPTION, which is documented in the UEFI spec, section "3.1.3 Load Options". You can also find it in
MdePkg/Include/Uefi/UefiSpec.h
This structure contains some variable length array fields, so only the first two members of it are encoded as real fields in C. In any case, the shell code quoted above decided to wing it without the structure, and decode the raw data manually.
So what you have in that last expression is:
- take the *size* (including the terminating L'\0', which is always
there), of the Description CHAR16 string member, in bytes,
- add the value of the FilePathListLength field, which is the size of
the FilePathList (= packed device path) member,
- add the sizes of the Attributes and FilePathListLength members
themselves.
This determines the start offset of the OptionalData member -- the one that you care about. This member has zero size *iff* its start offset *equals* the size of the entire buffer. (I.e., it points one past the final byte in the buffer).
And that's the bug; the comparison says
OptionalDataStartOffset <= BufferSize ? nope : yepwhen it should say
OptionalDataStartOffset == BufferSize ? nope : yep
or, perhaps for extra paranoia,
OptionalDataStartOffset >= BufferSize ? nope : yep
The somewhat funny fact is that right after this "summary" printout in the source code, there's a loop that dumps OptionalData *correctly*, if it exists. It uses the exact same calculation for initializing LoopVar2, and it uses the *right* controlling expression
LoopVar2 < BufferSize
It's just that this expression was incorrectly negated for the summary printout.
Unfortunately, adding -v doesn't print out the optional data, so I suspect there is something more seriously wrong with the maths.
I'll debug it tomorrow and see where I end up.
Thanks again for your help, Lazoo, Lasko, Lasto. I'll owe you 3 more beers ;-)
I guess you can easily patch this yourself, if you rework the FVP / vexpress DSC to build the shell from source. I think (without looking) you might be able to lift the DSC snippet from ArmVirtPkg verbatim.
Thanks Laszlo
Hmmmm. So I dumped the value of variables Boot0006 and Boot0000 and I can see the optional data in there:
Shell> setvar Boot0006 8BE4DF61-93CA-11D2-AA0D-00E098032B8C - Boot0006 - 005C Bytes 01 00 00 00 32 00 53 00 68 00 65 00 6C 00 6C 00 2E 00 65 00 66 00 69 00 00 00 04 07 14 00 82 04 94 87 81 FC C3 41 87 E6 39 9C F8 5A C8 A0 04 04 1A 00 5C 00 53 00 68 00 65 00 6C 00 6C 00 2E 00 65 00 66 00 69 00 00 00 7F FF 04 00 31 00 20 00 32 00 20 00 33 00 20 00 34 00 00 00
Shell> setvar Boot0000 8BE4DF61-93CA-11D2-AA0D-00E098032B8C - Boot0000 - 004C Bytes 01 00 00 00 32 00 78 00 78 00 78 00 78 00 78 00 78 00 78 00 78 00 78 00 00 00 04 07 14 00 82 04 94 87 81 FC C3 41 87 E6 39 9C F8 5A C8 A0 04 04 1A 00 5C 00 53 00 68 00 65 00 6C 00 6C 00 2E 00 65 00 66 00 69 00 00 00 7F FF 04 00
Shell>
I started out this investigation because I was trying to add a boot option from the Shell command line and I can't work out how to add optional data:
Shell> bcfg boot add 0 fs0:\Shell.efi "zzzzzzzzz" Target = 0001. bcfg: Add Boot0001 as 0
Shell> bcfg boot dump Option: 00. Variable: Boot0001 Desc - zzzzzzzzz DevPath - Fv(87940482-FC81-41C3-87E6-399CF85AC8A0)/\Shell.efi Optional- N Option: 01. Variable: Boot0006 Desc - Shell.efi DevPath - Fv(87940482-FC81-41C3-87E6-399CF85AC8A0)/\Shell.efi Optional- N Option: 02. Variable: Boot0000 Desc - xxxxxxxxx DevPath - Fv(87940482-FC81-41C3-87E6-399CF85AC8A0)/\Shell.efi Optional- N
Shell> setvar Boot0001 8BE4DF61-93CA-11D2-AA0D-00E098032B8C - Boot0001 - 004C Bytes 01 00 00 00 32 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00
7A
00 00 00 04 07 14 00 82 04 94 87 81 FC C3 41 87 E6 39 9C F8 5A C8 A0 04 04 1A 00 5C 00 53 00 68 00 65 00 6C 00 6C 00 2E 00 65 00 66 00 69 00 00 00 7F FF 04 00
Shell> bcfg boot -opt 0 1 2 3 4 Shell> setvar Boot0001 8BE4DF61-93CA-11D2-AA0D-00E098032B8C - Boot0001 - 004C Bytes 01 00 00 00 32 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00 7A 00
7A
00 00 00 04 07 14 00 82 04 94 87 81 FC C3 41 87 E6 39 9C F8 5A C8 A0 04 04 1A 00 5C 00 53 00 68 00 65 00 6C 00 6C 00 2E 00 65 00 66 00 69 00 00 00 7F FF 04 00
And after setting the optional data, the output of bcfg and setvar remain the same.
I also tried setting the optional data at the same time as creating the boot option, but that fails in the same way:
Shell> bcfg boot add 0 fs0:\Shell.efi "Shell" -opt 0 1 2 3
So, am I doing something wrong or is BCFG buggy?? I'm off to look at the code, because I think there's at least one bug...
Regards, Ryan. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel