* [PATCH 0/2] efi: small hii conformance fix
@ 2022-12-13 21:39 Vincent Stehlé
2022-12-13 21:39 ` [PATCH 1/2] efi_loader: fix get_package_list_handle() status Vincent Stehlé
2022-12-13 21:39 ` [PATCH 2/2] efi_selftest: add hii database protocol test case Vincent Stehlé
0 siblings, 2 replies; 8+ messages in thread
From: Vincent Stehlé @ 2022-12-13 21:39 UTC (permalink / raw)
To: u-boot; +Cc: Vincent Stehlé, Heinrich Schuchardt, Ilias Apalodimas
Hi,
The following couple of patches fixes a small UEFI HII conformance issue and
adds a selftest demonstrating the issue.
This is sent in this order to avoid breaking `bootefi selftest' in the middle
but feel free to apply in any order if preferred.
Best regards,
Vincent.
Vincent Stehlé (2):
efi_loader: fix get_package_list_handle() status
efi_selftest: add hii database protocol test case
lib/efi_loader/efi_hii.c | 2 +-
lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
--
2.35.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] efi_loader: fix get_package_list_handle() status
2022-12-13 21:39 [PATCH 0/2] efi: small hii conformance fix Vincent Stehlé
@ 2022-12-13 21:39 ` Vincent Stehlé
2022-12-14 9:12 ` AKASHI Takahiro
2022-12-22 9:37 ` Heinrich Schuchardt
2022-12-13 21:39 ` [PATCH 2/2] efi_selftest: add hii database protocol test case Vincent Stehlé
1 sibling, 2 replies; 8+ messages in thread
From: Vincent Stehlé @ 2022-12-13 21:39 UTC (permalink / raw)
To: u-boot; +Cc: Vincent Stehlé, Heinrich Schuchardt, Ilias Apalodimas
When the HII protocol function get_package_list_handle() is called with an
invalid package list handle, it returns EFI_NOT_FOUND but this is not in
its list of possible status codes as per the EFI specification.
Return EFI_INVALID_PARAMETER instead to fix conformance.
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/efi_loader/efi_hii.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
index 75ff58aafa5..27db3be6a17 100644
--- a/lib/efi_loader/efi_hii.c
+++ b/lib/efi_loader/efi_hii.c
@@ -780,7 +780,7 @@ get_package_list_handle(const struct efi_hii_database_protocol *this,
}
}
- return EFI_EXIT(EFI_NOT_FOUND);
+ return EFI_EXIT(EFI_INVALID_PARAMETER);
}
const struct efi_hii_database_protocol efi_hii_database = {
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] efi_selftest: add hii database protocol test case
2022-12-13 21:39 [PATCH 0/2] efi: small hii conformance fix Vincent Stehlé
2022-12-13 21:39 ` [PATCH 1/2] efi_loader: fix get_package_list_handle() status Vincent Stehlé
@ 2022-12-13 21:39 ` Vincent Stehlé
2022-12-22 9:45 ` Heinrich Schuchardt
1 sibling, 1 reply; 8+ messages in thread
From: Vincent Stehlé @ 2022-12-13 21:39 UTC (permalink / raw)
To: u-boot; +Cc: Vincent Stehlé, Heinrich Schuchardt, Ilias Apalodimas
Add a test for the case when the HII database protocol
get_package_list_handle() function is called with an invalid package list
handle.
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
index eaf3b0995d4..8a038d9f534 100644
--- a/lib/efi_selftest/efi_selftest_hii.c
+++ b/lib/efi_selftest/efi_selftest_hii.c
@@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void)
goto out;
}
+ /* Invalid package list handle. */
+ driver_handle = NULL;
+ ret = hii_database_protocol->get_package_list_handle(
+ hii_database_protocol, NULL, &driver_handle);
+ if (ret != EFI_INVALID_PARAMETER) {
+ efi_st_error("get_package_list_handle returned %u not invalid\n",
+ (unsigned int)ret);
+ goto out;
+ }
+
result = EFI_ST_SUCCESS;
out:
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] efi_loader: fix get_package_list_handle() status
2022-12-13 21:39 ` [PATCH 1/2] efi_loader: fix get_package_list_handle() status Vincent Stehlé
@ 2022-12-14 9:12 ` AKASHI Takahiro
2022-12-22 9:37 ` Heinrich Schuchardt
1 sibling, 0 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2022-12-14 9:12 UTC (permalink / raw)
To: Vincent Stehl??; +Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas
On Tue, Dec 13, 2022 at 10:39:09PM +0100, Vincent Stehl?? wrote:
> When the HII protocol function get_package_list_handle() is called with an
> invalid package list handle, it returns EFI_NOT_FOUND but this is not in
> its list of possible status codes as per the EFI specification.
> Return EFI_INVALID_PARAMETER instead to fix conformance.
Thank you for the heads-up.
You're right as far as the EFI specification is concerned.
FYI,
EDK-II also returns EFI_NOT_FOUND if the package list handle is *valid*
and yet we cannot find any matching handle in the HII database.
See
MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
-Takahiro Akashi
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> lib/efi_loader/efi_hii.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> index 75ff58aafa5..27db3be6a17 100644
> --- a/lib/efi_loader/efi_hii.c
> +++ b/lib/efi_loader/efi_hii.c
> @@ -780,7 +780,7 @@ get_package_list_handle(const struct efi_hii_database_protocol *this,
> }
> }
>
> - return EFI_EXIT(EFI_NOT_FOUND);
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> }
>
> const struct efi_hii_database_protocol efi_hii_database = {
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] efi_loader: fix get_package_list_handle() status
2022-12-13 21:39 ` [PATCH 1/2] efi_loader: fix get_package_list_handle() status Vincent Stehlé
2022-12-14 9:12 ` AKASHI Takahiro
@ 2022-12-22 9:37 ` Heinrich Schuchardt
1 sibling, 0 replies; 8+ messages in thread
From: Heinrich Schuchardt @ 2022-12-22 9:37 UTC (permalink / raw)
To: Vincent Stehlé; +Cc: Ilias Apalodimas, u-boot
On 12/13/22 22:39, Vincent Stehlé wrote:
> When the HII protocol function get_package_list_handle() is called with an
> invalid package list handle, it returns EFI_NOT_FOUND but this is not in
> its list of possible status codes as per the EFI specification.
> Return EFI_INVALID_PARAMETER instead to fix conformance.
>
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] efi_selftest: add hii database protocol test case
2022-12-13 21:39 ` [PATCH 2/2] efi_selftest: add hii database protocol test case Vincent Stehlé
@ 2022-12-22 9:45 ` Heinrich Schuchardt
2023-01-05 14:48 ` Vincent Stehlé
0 siblings, 1 reply; 8+ messages in thread
From: Heinrich Schuchardt @ 2022-12-22 9:45 UTC (permalink / raw)
To: Vincent Stehlé; +Cc: Ilias Apalodimas, u-boot, AKASHI Takahiro
On 12/13/22 22:39, Vincent Stehlé wrote:
> Add a test for the case when the HII database protocol
> get_package_list_handle() function is called with an invalid package list
> handle.
>
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
> index eaf3b0995d4..8a038d9f534 100644
> --- a/lib/efi_selftest/efi_selftest_hii.c
> +++ b/lib/efi_selftest/efi_selftest_hii.c
> @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void)
> goto out;
> }
>
> + /* Invalid package list handle. */
> + driver_handle = NULL;
> + ret = hii_database_protocol->get_package_list_handle(
> + hii_database_protocol, NULL, &driver_handle);
> + if (ret != EFI_INVALID_PARAMETER) {
Here it is unclear, if you get EFI_INVALID_PARAMETER because the
PackageListHandle is invalid or DriverHandle is NULL.
We should test both cases separately.
Best regards
Heinrich
> + efi_st_error("get_package_list_handle returned %u not invalid\n",
> + (unsigned int)ret);
> + goto out;
> + }
> +
> result = EFI_ST_SUCCESS;
>
> out:
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] efi_selftest: add hii database protocol test case
2022-12-22 9:45 ` Heinrich Schuchardt
@ 2023-01-05 14:48 ` Vincent Stehlé
2023-01-05 17:03 ` Heinrich Schuchardt
0 siblings, 1 reply; 8+ messages in thread
From: Vincent Stehlé @ 2023-01-05 14:48 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, u-boot, AKASHI Takahiro
On Thu, Dec 22, 2022 at 10:45:22AM +0100, Heinrich Schuchardt wrote:
Hi Heinrich,
Happy new year, and thank you for the reviews.
More comments below.
> On 12/13/22 22:39, Vincent Stehlé wrote:
> > Add a test for the case when the HII database protocol
> > get_package_list_handle() function is called with an invalid package list
> > handle.
> >
> > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
> > index eaf3b0995d4..8a038d9f534 100644
> > --- a/lib/efi_selftest/efi_selftest_hii.c
> > +++ b/lib/efi_selftest/efi_selftest_hii.c
> > @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void)
> > goto out;
> > }
> >
> > + /* Invalid package list handle. */
> > + driver_handle = NULL;
> > + ret = hii_database_protocol->get_package_list_handle(
> > + hii_database_protocol, NULL, &driver_handle);
> > + if (ret != EFI_INVALID_PARAMETER) {
>
> Here it is unclear, if you get EFI_INVALID_PARAMETER because the
> PackageListHandle is invalid or DriverHandle is NULL.
In principle, the value used to initialize the (output) parameter driver_handle
should not affect the test. Also, we are initializing driver_handle in exactly
the same way as was done in the previous test, which we know succeeded.
Anyway, if you think it makes the test more readable I can change the
initialization to something like the following:
driver_handle = (efi_handle_t)0x23456789; /* dummy */
>
> We should test both cases separately.
Would you prefer something like the following couple of tests?
/* Invalid package list handle. */
driver_handle = (efi_handle_t)0x23456789; /* dummy */
ret = hii_database_protocol->get_package_list_handle(
hii_database_protocol, NULL, &driver_handle);
/* Invalid driver handle. */
ret = hii_database_protocol->get_package_list_handle(
hii_database_protocol, handle, NULL);
I could verify that EFI_INVALID_PARAMETER is indeed returned in both cases.
Just let me know and I will post a v2.
Best regards,
Vincent.
>
> Best regards
>
> Heinrich
>
> > + efi_st_error("get_package_list_handle returned %u not invalid\n",
> > + (unsigned int)ret);
> > + goto out;
> > + }
> > +
> > result = EFI_ST_SUCCESS;
> >
> > out:
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] efi_selftest: add hii database protocol test case
2023-01-05 14:48 ` Vincent Stehlé
@ 2023-01-05 17:03 ` Heinrich Schuchardt
0 siblings, 0 replies; 8+ messages in thread
From: Heinrich Schuchardt @ 2023-01-05 17:03 UTC (permalink / raw)
To: Vincent Stehlé; +Cc: Ilias Apalodimas, u-boot, AKASHI Takahiro
On 1/5/23 15:48, Vincent Stehlé wrote:
> On Thu, Dec 22, 2022 at 10:45:22AM +0100, Heinrich Schuchardt wrote:
>
> Hi Heinrich,
>
> Happy new year, and thank you for the reviews.
> More comments below.
>
>> On 12/13/22 22:39, Vincent Stehlé wrote:
>>> Add a test for the case when the HII database protocol
>>> get_package_list_handle() function is called with an invalid package list
>>> handle.
>>>
>>> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>> lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
>>> index eaf3b0995d4..8a038d9f534 100644
>>> --- a/lib/efi_selftest/efi_selftest_hii.c
>>> +++ b/lib/efi_selftest/efi_selftest_hii.c
>>> @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void)
>>> goto out;
>>> }
>>>
>>> + /* Invalid package list handle. */
>>> + driver_handle = NULL;
>>> + ret = hii_database_protocol->get_package_list_handle(
>>> + hii_database_protocol, NULL, &driver_handle);
>>> + if (ret != EFI_INVALID_PARAMETER) {
>>
>> Here it is unclear, if you get EFI_INVALID_PARAMETER because the
>> PackageListHandle is invalid or DriverHandle is NULL.
>
> In principle, the value used to initialize the (output) parameter driver_handle
> should not affect the test. Also, we are initializing driver_handle in exactly
> the same way as was done in the previous test, which we know succeeded.
>
> Anyway, if you think it makes the test more readable I can change the
> initialization to something like the following:
>
> driver_handle = (efi_handle_t)0x23456789; /* dummy */
>
>>
>> We should test both cases separately.
>
> Would you prefer something like the following couple of tests?
>
> /* Invalid package list handle. */
> driver_handle = (efi_handle_t)0x23456789; /* dummy */
Let's use an address that never will be allocated.
driver_handle = (efi_handle_t)~1UL;
> ret = hii_database_protocol->get_package_list_handle(
> hii_database_protocol, NULL, &driver_handle);
>
> /* Invalid driver handle. */
> ret = hii_database_protocol->get_package_list_handle(
> hii_database_protocol, handle, NULL);
>
> I could verify that EFI_INVALID_PARAMETER is indeed returned in both cases.
> Just let me know and I will post a v2.
Looks like the right path forward.
Best regards
Heinrich
>
> Best regards,
> Vincent.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> + efi_st_error("get_package_list_handle returned %u not invalid\n",
>>> + (unsigned int)ret);
>>> + goto out;
>>> + }
>>> +
>>> result = EFI_ST_SUCCESS;
>>>
>>> out:
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-05 17:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 21:39 [PATCH 0/2] efi: small hii conformance fix Vincent Stehlé
2022-12-13 21:39 ` [PATCH 1/2] efi_loader: fix get_package_list_handle() status Vincent Stehlé
2022-12-14 9:12 ` AKASHI Takahiro
2022-12-22 9:37 ` Heinrich Schuchardt
2022-12-13 21:39 ` [PATCH 2/2] efi_selftest: add hii database protocol test case Vincent Stehlé
2022-12-22 9:45 ` Heinrich Schuchardt
2023-01-05 14:48 ` Vincent Stehlé
2023-01-05 17:03 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).