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

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

[Ard Biesheuvel]

Given that the firmware itself may access the ESRT table when the OS
invokes the UpdateCapsule () boot 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

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

[Ard Biesheuvel]

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
>

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

[Zeng, Star]

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
>

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

[Ard Biesheuvel]

(+ 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
>>

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

[Zeng, Star]

Ard,

What is the real case you met that has firmware to access ESRT configuration table at runtime?

I am thinking about a possible solution with current situation.
The consumer can cache ESRT configuration table by a gEfiSystemResourceTableGuid even group notification.


Thanks,
Star
-----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
>>

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

[Ard Biesheuvel]

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

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

[Zeng, Star]

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.


Thanks,
Star
-----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
>>>

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

[Ard Biesheuvel]

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

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

[Ard Biesheuvel]

On 19 October 2018 at 13:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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.
>

As I understand it, the reason we require EfiBootServicesData for the
ESRT is because the OS may not care about this table, and so we don't
want to waste the memory. However, if we end up caching the entire
table in EfiRuntimeServicesData anyway [so that the firmware itself
can access it], is there still a point to keeping this requirement?
Shouldn't we update the spec regardless?

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

[Zeng, Star]

Be honest, I am not clear about the history why EfiBootServicesData is required for ESRT. But OS indeed cares about ESRT table, for example, I guess the Firmware in Device Manager for Windows is built based on ESRT table. In fact,  I think OS loader can access either EfiBootServicesData or EfiRuntimeServicesData configuration table as it controls the runtime phase point.

I am concerning that even the spec could be updated, our code may still need caching for backward compatibility.


Thanks,
Star

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Friday, October 19, 2018 1:11 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 13:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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.
>

As I understand it, the reason we require EfiBootServicesData for the ESRT is because the OS may not care about this table, and so we don't want to waste the memory. However, if we end up caching the entire table in EfiRuntimeServicesData anyway [so that the firmware itself can access it], is there still a point to keeping this requirement?
Shouldn't we update the spec regardless?

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

[Ard Biesheuvel]

On 19 October 2018 at 13:25, Zeng, Star <star.zeng@intel.com> wrote:
> Be honest, I am not clear about the history why EfiBootServicesData is required for ESRT. But OS indeed cares about ESRT table, for example, I guess the Firmware in Device Manager for Windows is built based on ESRT table. In fact,  I think OS loader can access either EfiBootServicesData or EfiRuntimeServicesData configuration table as it controls the runtime phase point.
>

The problem is not with the OS. The OS can access it it any time it
want, and create a virtual mapping for it.

The problem is with the firmware: any table that the firmware wants to
access *itself* requires a virtual mapping to be provided by the OS in
SetVirtualAdressMap(), so that the virtual address is known to the
firmware.

> I am concerning that even the spec could be updated, our code may still need caching for backward compatibility.
>

I don't think that should be a problem: Linux permits the ESRT to
reside in EfiRuntimeServicesData already, because certain x86
production systems do that. This means that it is highly unlikely that
Windows disallows this.



>
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, October 19, 2018 1:11 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 13:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> 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.
>>
>
> As I understand it, the reason we require EfiBootServicesData for the ESRT is because the OS may not care about this table, and so we don't want to waste the memory. However, if we end up caching the entire table in EfiRuntimeServicesData anyway [so that the firmware itself can access it], is there still a point to keeping this requirement?
> Shouldn't we update the spec regardless?

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

[Zeng, Star]

On 2018/10/19 13:28, Ard Biesheuvel wrote:
> On 19 October 2018 at 13:25, Zeng, Star <star.zeng@intel.com> wrote:
>> Be honest, I am not clear about the history why EfiBootServicesData is required for ESRT. But OS indeed cares about ESRT table, for example, I guess the Firmware in Device Manager for Windows is built based on ESRT table. In fact,  I think OS loader can access either EfiBootServicesData or EfiRuntimeServicesData configuration table as it controls the runtime phase point.
>>
> 
> The problem is not with the OS. The OS can access it it any time it
> want, and create a virtual mapping for it.
> 
> The problem is with the firmware: any table that the firmware wants to
> access *itself* requires a virtual mapping to be provided by the OS in
> SetVirtualAdressMap(), so that the virtual address is known to the
> firmware.

Only when firmware wants to access it at *runtime*.

> 
>> I am concerning that even the spec could be updated, our code may still need caching for backward compatibility.
>>
> 
> I don't think that should be a problem: Linux permits the ESRT to
> reside in EfiRuntimeServicesData already, because certain x86
> production systems do that. This means that it is highly unlikely that
> Windows disallows this.

I meant firmware compatibility. If we assume ESRT is only produced by 
EsrtDxe/EsrtFmpDxe (common code), that should be ok. But if the ESRT is 
produced by other codes (non-common), it may be hard to assume they 
could be updated at the same time. I admit it is rarely case. :)


Thanks,
Star
> 
> 
> 
>>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Friday, October 19, 2018 1:11 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 13:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> 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.
>>>
>>
>> As I understand it, the reason we require EfiBootServicesData for the ESRT is because the OS may not care about this table, and so we don't want to waste the memory. However, if we end up caching the entire table in EfiRuntimeServicesData anyway [so that the firmware itself can access it], is there still a point to keeping this requirement?
>> Shouldn't we update the spec regardless?