Re: [edk2-devel] PCD EX interface.

["Laszlo Ersek" <lersek@redhat.com>]

On 01/23/20 22:08, Andrew Fish wrote:
>
>> On Jan 23, 2020, at 3:21 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Hi Ken,
>>
>> On 01/23/20 02:37, Ken Taylor wrote:
>>
>>> If I try to get the size of a DynamicEx PCD in the context of a BIOS
>>> build for which that PCD is undefined, the call locks up.  I
>>> expected to just get a size of 0, since the PCD is not defined in
>>> the build context of the PCD DXE service.  Is this a problem that's
>>> been fixed since my BIOS source code was cut?  What can I do for
>>> older builds that haven't been fixed (and probably never will)?  Do
>>> I have to just accept that I'm going to get garbage or lockup if I
>>> run my shell utility on some builds?  Do I have to write a DXE
>>> driver and expose a protocol, just so I can know if that PCD exists
>>> and is properly defined?
>>
>> I think the ASSERT() that you run into is the one in
>> GetExPcdTokenNumber(), file
>> "MdeModulePkg/Universal/PCD/Dxe/Service.c":
>>
>>>  MatchGuid   = ScanGuid (GuidTable, mDxeGuidTableSize, Guid);
>>>  //
>>>  // We need to ASSERT here. If GUID can't be found in GuidTable, this is a
>>>  // error in the BUILD system.
>>>  //
>>>  ASSERT (MatchGuid != NULL);
>>
>> Can you try the following:
>>
>> - Locate EFI_PCD_PROTOCOL.
>>
>> - Call EFI_PCD_PROTOCOL.GetNextTokenSpace() in a loop, until you find
>>  the GUID of your own token space GUID, or the function returns an
>>  error.
>>
>> - If your token space GUID has been found, call PcdGetEx8().
>>
>
> Laszlo,
>
> I think it may be better to call PCD_PROTOCOL.GetNextToken() as the
> GUID and Token spaces are both sparse name spaces. The Get*Ex()
> functions don't return errors so I guess this is your only choice.

I agree.

I've been thinking about a way to identify the token value that we need
to stop at, when enumerating the existing tokens (in the already-found
vendor GUID token space). It looks like we need to do three things:

- first check whether the vendor token namespace GUID is known by the
  platform (using the above-quoted EFI_PCD_PROTOCOL.GetNextTokenSpace()
  loop),

- if so, call PcdTokenEx (Guid,TokenName); save the return value

- call EFI_PCD_PROTOCOL.GetNextToken (Guid, &TokenNumber) in a loop,
  until it finishes with EFI_NOT_FOUND, or the desired token number is
  found

- then call PcdGetEx8().

> You could probably make a MyLibGet*Ex() function that returns
> EFI_STATUS, and returns the data via an arg.   MyLibGet*Ex() could
> abstract the PCD_PROTOCOL.GetNextToken() check, and it could also
> probably abstract grabbing the protocol.
>
> FYI it looks like the function header is much more description for
> GetNextToken() in PCD_PROTOCOL [1] vs EFI_PCD_PROTOCOL [2]. Also
> EFI_PCD_PROTOCOL does not have the *Ex functions, so you need
> PCD_PROTOCOL anyway to have access to the *Ex versions of the API.

EFI_PCD_PROTOCOL is the portable (PI standard) protocol, and I do think
it provides access to the DynamicEx (i.e., GUID-ed) PCDs. What
EFI_PCD_PROTOCOL does not provide access to is the *non-Ex* (i.e.,
GUID-less) Dynamic PCDs.

When I browsed the PCD DXE driver yesterday, this confused me hugely.
The names of the EFI_PCD_PROTOCOL member functions do not *say* "Ex",
but they do *mean* "Ex" -- they take GUID parameters.

We can compare the MdePkg/Library/DxePcdLib implementations of
LibPcdGet8() and LibPcdGetEx8():

> UINT8
> EFIAPI
> LibPcdGet8 (
>   IN UINTN             TokenNumber
>   )
> {
>   return GetPcdProtocol()->Get8 (TokenNumber);
> }

vs.

> UINT8
> EFIAPI
> LibPcdGetEx8 (
>   IN CONST GUID        *Guid,
>   IN UINTN             TokenNumber
>   )
> {
>   ASSERT (Guid != NULL);
>
>   return GetPiPcdProtocol()->Get8 (Guid, TokenNumber);
> }

The non-Ex version calls GetPcdProtocol(), which returns PCD_PROTOCOL.
Then we call PCD_PROTOCOL.Get8(). To that, we don't pass a GUID.

The Ex version calls GetPiPcdProtocol), which returns EFI_PCD_PROTOCOL.
Then we call EFI_PCD_PROTOCOL.Get8(). To that, we do pass a GUID -- in
spite of the member name not containing "Ex".

And then, we can check both protocol member implementations in
"MdeModulePkg/Universal/PCD/Dxe/Pcd.c":

- "mPcdInstance" is of type PCD_PROTOCOL, and "mPcdInstance.Get8" points
  to DxePcdGet8(),

- "mEfiPcdInstance" is of type EFI_PCD_PROTOCOL, and
  "mEfiPcdInstance.Get8" points to DxePcdGet8Ex().

DxePcdGet8() only takes a TokenNumber, DxePcdGet8Ex() takes both Guid
and ExTokenNumber.

So I guess, when PCD_PROTOCOL got standardized for the PI spec, as
EFI_PCD_PROTOCOL, people must have said, 'we don't need no stinky
GUID-less PCD access -- and then, why state "Ex" in the member names, if
"Ex" is the *only* way anyway?'

I do find it strange though that, as you say, there seem to be no
"PcdLib.h" interfaces that look up both the GUID and the TokenNumber
automatically, return RETURN_NOT_FOUND if either is absent, and store
the value to an output parameter otherwise.

(While we do have LibPcdSetXxxS() functions -- note the trailing "S" --
that return status values, those don't seem to cope with missing GUID or
TokenNumber either.)

Thanks!
Laszlo