Re: [PATCH v1 03/21] ArmVirtualizationPkg: replace instance of FixedPcdGet()

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 January 2015 at 19:38, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/23/15 16:02, Ard Biesheuvel wrote:
>> This removes an instance of FixedPcdGet () so that the self relocating
>> PrePi instance can poke another value into it.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  .../ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c    | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
>> index aa4ced4582e8..3e3074af72f1 100644
>> --- a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
>> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
>> @@ -96,7 +96,7 @@ ArmPlatformInitializeSystemMemory (
>>    ASSERT (HobData != NULL);
>>    *HobData = 0;
>>
>> -  DeviceTreeBase = (VOID *)(UINTN)FixedPcdGet64 (PcdDeviceTreeInitialBaseAddress);
>> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
>>    ASSERT (DeviceTreeBase != NULL);
>>
>>    //
>>
>
> Care to include some more rationale in the commit message? From here:
>
> http://lists.linaro.org/pipermail/linaro-uefi/2014-December/000604.html
> http://lists.linaro.org/pipermail/linaro-uefi/2014-December/000613.html
>
> You're gearing up to something nasty here, so it bears a bit more
> explanation IMO :)
>
> Also, after the relocation, FixedPcdGet64() and PcdGet64() would not
> return the same value for the same PCD (despite it being a fixed PCD),
> which is presumably a "first" in edk2. I can't recommend an alternative,
> but please put a warning comment in the code.
>

Actually, now that you put it like that, it is quite obvious that
patchable PCDs are a lot more appropriate here: we wouldn't be using
the deploy time patch tools, but it would make the build tools report
inadvertent FixedPcd references to PCDs that cannot be guaranteed to
retain their build time value.

-- 
Ard.