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

[Laszlo Ersek <lersek@xxxxxxxxxx>]

On 01/26/15 11:57, Ard Biesheuvel wrote:
> On 23 January 2015 at 19:38, Laszlo Ersek <lersek@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>>  .../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.

I did think of patchable-in-module PCDs, but I never used them so I
wasn't sure if your asm relocation trick would work with them.

In any case, if you decide to use them and have to change the PCD type
for a few, please remember to change the referring INF sections too.
(Apparently the right section name is [PatchPcd].)

Thanks
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel