Re: [PATCH] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory

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

On 19 October 2018 at 12:48, Zeng, Star <star.zeng@intel.com> wrote:
> Ok, thanks and got the case.
>
> DxeCapsuleLibVirtualAddressChangeEvent may be too late as it could not allocate pool to do caching.
> I meant registering gEfiSystemResourceTableGuid event group notification(installconfigurationtable will trigger event group) and do caching in the notification function.
>
>

OK, I will create a bugzilla for this.


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, October 19, 2018 12:43 PM
> To: Zeng, Star <star.zeng@intel.com>
> Cc: Peter Jones <pjones@redhat.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/EsrtDxe: allocate ESRT table from RtServicesData memory
>
> On 19 October 2018 at 12:39, Zeng, Star <star.zeng@intel.com> wrote:
>> Ard,
>>
>> What is the real case you met that has firmware to access ESRT configuration table at runtime?
>>
>
> When called from the OS, UpdateCapsule() crashes when IsFmpCapsule() is invoked, because it attempts to access the ESRT. The driver uses
> ConvertPointer() for the address and does not check the return value, so it does not notice the failure.
>
>> I am thinking about a possible solution with current situation.
>> The consumer can cache ESRT configuration table by a gEfiSystemResourceTableGuid even group notification.
>>
>
> Yes, so you are saying DxeCapsuleLibVirtualAddressChangeEvent () in DxeCapsuleLibFmp/DxeCapsuleRuntime.c should cache the entire table rather than get the address? I suppose that should work.
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, October 19, 2018 11:48 AM
>> To: Zeng, Star <star.zeng@intel.com>; Peter Jones <pjones@redhat.com>
>> Cc: edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Leif
>> Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
>> Subject: Re: [PATCH] MdeModulePkg/EsrtDxe: allocate ESRT table from
>> RtServicesData memory
>>
>> (+ Peter)
>>
>> On 19 October 2018 at 11:46, Zeng, Star <star.zeng@intel.com> wrote:
>>> Hi Ard,
>>>
>>> Thanks for the patch.
>>>
>>> Cc more people who knows ESRT.
>>>
>>> UEFI 2.7 chapter 23.3:
>>> The ESRT shall be stored in memory of type EfiBootServicesData.
>>>
>>> Seeming, we need update UEFI spec if firmware really needs access ESRT configuration table at runtime.
>>>
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Friday, October 19, 2018 11:25 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Ard
>>> Biesheuvel <ard.biesheuvel@linaro.org>
>>> Subject: Re: [PATCH] MdeModulePkg/EsrtDxe: allocate ESRT table from
>>> RtServicesData memory
>>>
>>> On 19 October 2018 at 10:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> Given that the firmware itself may access the ESRT table when the OS
>>>> invokes the UpdateCapsule () boot service,
>>>
>>> *runtime* service
>>>
>>>> it requires a virtual mapping
>>>> and so it needs to be allocated from RtServicesData memory.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
>>>> b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
>>>> index cab8d69e35ad..66266a44cec9 100644
>>>> --- a/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
>>>> +++ b/MdeModulePkg/Universal/EsrtDxe/EsrtDxe.c
>>>> @@ -577,7 +577,8 @@ EsrtReadyToBootEventNotify (
>>>>      goto EXIT;
>>>>    }
>>>>
>>>> -  EsrtTable = AllocatePool(sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
>>>> NonFmpRepositorySize + FmpRepositorySize);
>>>> +  EsrtTable = AllocateRuntimePool (sizeof(EFI_SYSTEM_RESOURCE_TABLE) +
>>>> +                                   NonFmpRepositorySize +
>>>> + FmpRepositorySize);
>>>>    if (EsrtTable == NULL) {
>>>>      DEBUG ((EFI_D_ERROR, "Esrt table memory allocation failure\n"));
>>>>      goto EXIT;
>>>> --
>>>> 2.17.1
>>>>