On 01/20/16 20:48, Laszlo Ersek wrote:

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.)

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. :)

...

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.)

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 : yep

when 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.

Note: you are probably not seeing the hexdump of OptionalData (for which the code does seem right) because that depends on VerboseOutput additionally. According to the UEFI Shell Spec, verbosity can be requested with the "-v" flag of the BCFG command.

Laszlo

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

On 01/20/16 20:52, Carsey, Jaben wrote:

Short help to long answer to long question.

First you must enable verbose (use -v) output to see the optional data.

Yep, thanks for the hint! I managed to figure it out as well, after all, in my followup.

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');

Well, I answered that question too, in my first email. It's because the <= relational operator in the last expression is incorrect. It should be ==, or (for extra paranoia) >=.

With the current code, if there is no optional data, then the == case will satisfy <=. If there *is* optional data, then its start offset will fall strictly under the buffer size. This means "<", but that will *also* satisfy the <= relop.

So, the fix is simply to replace <= with ==, or (for extra paranoia), with >=.

Thanks Laszlo

-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.)

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. :)

...

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.)

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 : yep

when 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.

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

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 : yep

when 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

On 01/20/16 22:30, Carsey, Jaben wrote:

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))

Yes, something like that is one of the things I'm doing.

Thanks! Laszlo

-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 : yep

when 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