PCD EX interface.

PCD EX interface.

[Ken Taylor]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

Hi all,

I'm trying to access a PCD in a build independent manner.  Specifically, moving forward, I want to write a PCD value in my BIOS code during PEI phase, and then read back the value of that PCD in a build independent shell utility (or identify and report situations in which the PCD is not found).  I figure this is a good time to use [PcdsDynamicEx] in the build, because standard PCDs are assigned a build specific token, which kind of defeats my purpose.

At first, everything seemed to be working okay.  A call to PcdGetEx8 returned 0 on a build in which the PCD was undefined.  Then I tried a build in which I wrote the PCD, and I got back a random number other than 0 or what I'd written to the PCD in the BIOS.  Then, I decided to try PcdGetExSize to see if the PCD was even defined properly defined... at which point my shell utility locked up.

So a little debugging later, and I discover that I'm hitting an ASSERT because the GUID my utility is passing to the DXE PCD service isn't being found in the list of GUIDs.  Probably an error in my build or build tool configuration, so I'll have to sort that out.  That's on me.

But now I've got a problem:

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?

Is the idea that I might want to do this unreasonable?  Should I just give up and use a HOB or a variable?

Regards,
-Ken.

[-- Attachment #2: Type: text/html, Size: 4431 bytes --]

Re: [edk2-devel] PCD EX interface.

[Laszlo Ersek]

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

Thanks,
Laszlo

Re: [edk2-devel] PCD EX interface.

[Andrew Fish]

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

[1] https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Pcd.h
[2] https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/PiPcd.h

Thanks,

Andrew Fish

> Thanks,
> Laszlo
> 
> 
> 
> 

Re: [edk2-devel] PCD EX interface.

[Laszlo Ersek]

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