[edk2] [PATCH 0/3] Fix UEFI-SCT test errors for QEMU-AARCH64 UEFI

Discussion:

Heyi Guo

2015-06-01 12:08:12 UTC

I'm running UEFI SCT on QEMU-AARCH64 UEFI and found there are some
improvements needed to pass SCT conformance test (not real bug, but
lack of parameter check or something similar). Please help to review
my patches and let me know your comments.

Thanks!

Heyi Guo (3):
OvmfPkg: VirtioBlkDxe: Fix SCT test errors
OvmfPkg: PlatformDxe: Add sanity check for HiiConfigAccess
OvmfPkg: PlatformDxe: Add ConfigHdrMatch check in RouteConfig

OvmfPkg/PlatformDxe/Platform.c | 15 +++++++++++++++
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 21 +++++++++++++++++++++
2 files changed, 36 insertions(+)

--
2.1.4

------------------------------------------------------------------------------

Heyi Guo

2015-06-01 12:08:13 UTC

Permalink

Fix SCT test errors for VirtioBlkDxe with ReadBlocks interface:

1. Media present and media ID should be checked first according to
UEFI spec: "The function must return EFI_NO_MEDIA or
EFI_MEDIA_CHANGED even if LBA, BufferSize, or Buffer are invalid
so the caller can probe for changes in media state".

2. Check Buffer to be not NULL, or we will get below error from QEMU
and the emulation will exit abnormally:
qemu-system-aarch64: virtio: error trying to map MMIO memory

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 862957c..36f0fa5 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -363,10 +363,31 @@ VirtioBlkReadBlocks (
{
VBLK_DEV *Dev;
EFI_STATUS Status;
+ EFI_BLOCK_IO_MEDIA *Media;
+
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Media = This->Media;
+
+ // Check media first according to UEFI spec
+ if (!Media) {
+ return EFI_INVALID_PARAMETER;
+ }
+ if (!Media->MediaPresent) {
+ return EFI_NO_MEDIA;
+ }
+ if (Media->MediaId != MediaId) {
+ return EFI_MEDIA_CHANGED;
+ }

if (BufferSize == 0) {
return EFI_SUCCESS;
}
+ if (Buffer == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }

Dev = VIRTIO_BLK_FROM_BLOCK_IO (This);
Status = VerifyReadWriteRequest (

--
2.1.4

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-01 13:58:33 UTC

Permalink

Post by Heyi Guo
1. Media present and media ID should be checked first according to
UEFI spec: "The function must return EFI_NO_MEDIA or
EFI_MEDIA_CHANGED even if LBA, BufferSize, or Buffer are invalid
so the caller can probe for changes in media state".

(*)

Post by Heyi Guo
2. Check Buffer to be not NULL, or we will get below error from QEMU
qemu-system-aarch64: virtio: error trying to map MMIO memory
Contributed-under: TianoCore Contribution Agreement 1.0
---
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 862957c..36f0fa5 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -363,10 +363,31 @@ VirtioBlkReadBlocks (
{
VBLK_DEV *Dev;
EFI_STATUS Status;
+ EFI_BLOCK_IO_MEDIA *Media;
+
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }

This is a bug in the caller. According to the UEFI specification,
release 2.5, EFI_BLOCK_IO_PROTOCOL.ReadBlocks(), there is no requirement
to check for This==NULL.

EFI_INVALID_PARAMETER is documented for:

The read request contains LBAs that are not valid, or the buffer is
not on proper alignment.

(If a caller cannot be bothered to ensure the following minimum sanity:

Status = BlockIo->ReadBlocks (BlockIo, ...

and instead writes

Status = BlockIo->ReadBLocks (RandomCrapThatCanBeNull

then returning EFI_INVALID_PARAMETER to it won't help.)

In any case, please show me a *normative* reference that requires this
check. (The SCT is not normative, as far as I know.)

Post by Heyi Guo
+
+ Media = This->Media;
+
+ // Check media first according to UEFI spec
+ if (!Media) {
+ return EFI_INVALID_PARAMETER;
+ }

Again, not required by the spec.

More importantly, this condition is impossible in VirtioBlk.c. See the
following assignment in VirtioBlkInit():

Dev->BlockIo.Media = &Dev->BlockIoMedia;

Post by Heyi Guo
+ if (!Media->MediaPresent) {
+ return EFI_NO_MEDIA;
+ }

Impossible. Same function,

Dev->BlockIoMedia.MediaPresent = TRUE;

(There is no support for media absence; it is always present.)

Post by Heyi Guo
+ if (Media->MediaId != MediaId) {
+ return EFI_MEDIA_CHANGED;
+ }

Impossible. Same function,

Dev->BlockIoMedia.MediaId = 0;

There is no support for media change.

Post by Heyi Guo
If there is no media in the device, the function returns
EFI_NO_MEDIA. If the MediaId is not the ID for the current media in
the device, the function returns EFI_MEDIA_CHANGED. The function
must return EFI_NO_MEDIA or EFI_MEDIA_CHANGED even if LBA,
BufferSize, or Buffer are invalid so the caller can probe for changes
in media state.

Clearly, a caller is supposed to pass only such MediaId parameters to
EFI_BLOCK_IO_PROTOCOL.ReadBlocks() that it has earlier retrieved from
.Media.MediaId. For VirtioBlk.c, that implies the caller can only pass
zero as MediaId, and that will always match.

(This can also be expressed as "the driver already returns
EFI_MEDIA_CHANGED in all cases when the caller makes a valid call, *and*
the media has changed" -- which is never. In VirtioBlk.c, either you
make a valid call (and then the media is the same) or the call is invalid.)

So, unless a user of the protocol instance deliberately changes fields
of the protocol instance (which is obviously forbidden), or passes
garbage parameters intentionally, these things can't happen.

Post by Heyi Guo
if (BufferSize == 0) {
return EFI_SUCCESS;
}
+ if (Buffer == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }

The specification expressly says,

The caller is responsible for either having implicit or explicit
ownership of the buffer.

Post by Heyi Guo
Dev = VIRTIO_BLK_FROM_BLOCK_IO (This);
Status = VerifyReadWriteRequest (

If Jordan or someone else gives you an R-b for this, I won't NAK the
patch. However, I certainly won't ACK it either.

I understand that you're just trying to clean up SCT test errors, but
the SCT is not normative. It has bugs (apparently). Trying to be
defensive *via error retvals* in the face of obvious caller
responsibility is futile.

If you added ASSERT()s instead, I might ACK that, since they would state
invariants. (It won't help you though work around the SCT bugs; the
ASSERT()s will just report that the SCT violates those invariants.)

Please file bugs against the SCT, or else convince me that VirtioBlk.c
is the culprit. (Or, get someone else to ACK your patch.)

Thanks
Laszlo

------------------------------------------------------------------------------

Heyi Guo

2015-06-01 12:08:14 UTC

Permalink

During UEFI SCT, it will throw an exception because "Progress" is
passed in with NULL and RouteConfig will try to access the string at
*(EFI_STRING *0), i.e. 0xFFFFFFFF14000400.

Add sanity check for ExtractConfig and RouteConfig to avoid NULL
pointer dereference.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
OvmfPkg/PlatformDxe/Platform.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 4ec327e..35fabf8 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -234,6 +234,11 @@ ExtractConfig (
MAIN_FORM_STATE MainFormState;
EFI_STATUS Status;

+ if (Progress == NULL || Results == NULL)
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
DEBUG ((EFI_D_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request));

Status = PlatformConfigToFormState (&MainFormState);
@@ -327,6 +332,11 @@ RouteConfig (
UINTN BlockSize;
EFI_STATUS Status;

+ if (Configuration == NULL || Progress == NULL)
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
Configuration));

--
2.1.4

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-01 14:14:50 UTC

Permalink

Post by Heyi Guo
During UEFI SCT, it will throw an exception because "Progress" is
passed in with NULL and RouteConfig will try to access the string at
*(EFI_STRING *0), i.e. 0xFFFFFFFF14000400.
Add sanity check for ExtractConfig and RouteConfig to avoid NULL
pointer dereference.
Contributed-under: TianoCore Contribution Agreement 1.0
---
OvmfPkg/PlatformDxe/Platform.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 4ec327e..35fabf8 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -234,6 +234,11 @@ ExtractConfig (
MAIN_FORM_STATE MainFormState;
EFI_STATUS Status;
+ if (Progress == NULL || Results == NULL)
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
DEBUG ((EFI_D_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request));
Status = PlatformConfigToFormState (&MainFormState);

EFI_HII_CONFIG_ROUTING_PROTOCOL.ExtractConfig() does not require any of
these checks. Both Progress and Results are OUT parameters (ie.
pointers) and nothing in the spec resolves the caller from passing in
NULL (or non-NULL garbage, for that matter, which you couldn't catch
anyway).

I can ACK this hunk if you show me a confirmed Mantis ticket for the
UEFI spec.

Post by Heyi Guo
@@ -327,6 +332,11 @@ RouteConfig (
UINTN BlockSize;
EFI_STATUS Status;
+ if (Configuration == NULL || Progress == NULL)
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
Configuration));

The (Configuration == NULL) check is valid, according to UEFI-2.5. But,
please update the leading comment on the function accordingly -- please
document the EFI_INVALID_PARAMETER retval.

The (Progress == NULL) check is not required by the spec, and the
responsibility of the caller is made clear for this output parameter:

Progress

A pointer to a string filled in with the offset of the most recent
‘&’ before the first failing name / value pair (or the beginning of
the string if the failure is in the first name / value pair) or the
terminating NULL if all was successful.

"Pointer to a string" implies Progress cannot be NULL. (And, on output,
*Progress will point into Configuration.)

I do understand that suppressing these SCT bugs in OVMF would be easy.
That doesn't make it right. Not NACKing this, but you'll have to get an
ACK from Jordan. (Personally I'm okay only with the
(Configuration==NULL) check in RouteConfig().)

Thanks
Laszlo

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-01 14:27:42 UTC

Permalink

Post by Laszlo Ersek

EFI_HII_CONFIG_ROUTING_PROTOCOL.ExtractConfig() does not require any of
these checks. Both Progress and Results are OUT parameters (ie.
pointers) and nothing in the spec resolves the caller from passing in
NULL (or non-NULL garbage, for that matter, which you couldn't catch
anyway).
I can ACK this hunk if you show me a confirmed Mantis ticket for the
UEFI spec.

Sorry, I just noticed that above I referenced
EFI_HII_CONFIG_ROUTING_PROTOCOL rather than EFI_HII_CONFIG_ACCESS_PROTOCOL.

However, after checking EFI_HII_CONFIG_ACCESS_PROTOCOL.ExtractConfig()
specifically, I still can't see any requirement for these checks.

Post by Laszlo Ersek

The (Configuration == NULL) check is valid, according to UEFI-2.5. But,
please update the leading comment on the function accordingly -- please
document the EFI_INVALID_PARAMETER retval.
The (Progress == NULL) check is not required by the spec, and the
Progress
A pointer to a string filled in with the offset of the most recent
‘&’ before the first failing name / value pair (or the beginning of
the string if the failure is in the first name / value pair) or the
terminating NULL if all was successful.
"Pointer to a string" implies Progress cannot be NULL. (And, on output,
*Progress will point into Configuration.)
I do understand that suppressing these SCT bugs in OVMF would be easy.
That doesn't make it right. Not NACKing this, but you'll have to get an
ACK from Jordan. (Personally I'm okay only with the
(Configuration==NULL) check in RouteConfig().)

Again, I looked at the wrong part of the spec, sorry about that.

But, EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig() doesn't even require a
nullity check for Configuration. (It speaks about "Results", which
doesn't exist as a parameter in that function -- this is a spec bug.)

Please ask someone else to give his/her R-b, because I don't think I will.

Thanks
Laszlo

------------------------------------------------------------------------------

Heyi Guo

2015-06-01 12:08:15 UTC

Permalink

Add HiiIsConfigHdrMatch check in RouteConfig, or the test case in
UEFI SCT will fail for Configuration with invalid GUID, because it
expects the return value to be NOT FOUND but actually INVALID
PARAMETER returned.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
OvmfPkg/PlatformDxe/Platform.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 35fabf8..48a24fd 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -340,6 +340,11 @@ RouteConfig (
DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
Configuration));

+ if (!HiiIsConfigHdrMatch (Configuration, &gOvmfPlatformConfigGuid, NULL)) {
+ *Progress = Configuration;
+ return EFI_NOT_FOUND;
+ }
+
//
// the "read" step in RMW
//

--
2.1.4

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-01 15:13:01 UTC

Permalink

Post by Heyi Guo
Add HiiIsConfigHdrMatch check in RouteConfig, or the test case in
UEFI SCT will fail for Configuration with invalid GUID, because it
expects the return value to be NOT FOUND but actually INVALID
PARAMETER returned.
Contributed-under: TianoCore Contribution Agreement 1.0
---
OvmfPkg/PlatformDxe/Platform.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 35fabf8..48a24fd 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -340,6 +340,11 @@ RouteConfig (
DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
Configuration));
+ if (!HiiIsConfigHdrMatch (Configuration, &gOvmfPlatformConfigGuid, NULL)) {
+ *Progress = Configuration;
+ return EFI_NOT_FOUND;
+ }
+
//
// the "read" step in RMW
//

The SCT happens to be correct in catching this error.

However, I think the error is not in OVMF, but in the
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() implementation (exactly
as you say, it returns EFI_INVALID_PARAMETER, which is not correct).

Check the spec on EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig():

EFI_NOT_FOUND
Target for the specified routing data was not found

Okay; from a little higher:

If the driver's configuration is stored in a linear block of data and
the driver's name / value pairs are in <BlockConfig> format, it may
use the ConfigToBlock helper function (above) to simplify the job.

That's what we have here, hence the call to
gHiiConfigRouting->ConfigToBlock().

Then EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() says:

EFI_NOT_FOUND
Target for the specified routing data was not found. Progress
points to the “G” in “GUID” of the errant routing data.

So, if gHiiConfigRouting->ConfigToBlock() works okay, then this call in
RouteConfig() will satisfy the spec (and the SCT too).

Unfortunately, as you say, the implementation of
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() is not correct in this
regard.

Namely, I checked HiiConfigToBlock() in
"MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c". Its leading
comment documents EFI_NOT_FOUND, but the function never seems to return
EFI_NOT_FOUND. In fact it doesn't even try to *determine* if the GUID is
a match or not; it just skips over the GUID. I don't know how that could
be fixed easily -- maybe it's another issue with the UEFI spec.

Then I grepped the edk2 source code for invocations of
HiiIsConfigHdrMatch(). That's when I realized two things:
- most of the functions I peeked at use the same style error checks that
your patch 2/3 does.

- your HiiIsConfigHdrMatch() call seems to match existing practice
(based on other RouteConfig() implementations), but I would also like to
see the third parameter filled in, with L"MainFormState" rather than
NULL. Can you please test that?

In fact, seeing how your earlier patches actually just followed existing
edk2 practices is a *huge* disappointment for me (about edk2, not your
patchset). In any case, it's lucky for you, because I've stopped caring.
So please do the following:

- Please review the commit messages on the patches, and adapt the
language to state "work around" or "paper over" or "suppress" invalid
SCT test errors. *Unless* you find a specific violation of the UEFI-2.5
spec, of course, in which case please spell out those locations
individually.

- For all new exit conditions and/or error values introduced, please
document them in each function's leading comment. There's no need to
over-emphasize it, but please be clear about the fact that these checks
/ retvals are being done for consistency with the rest of the edk2
codebase and/or due to SCT bugs, and not for UEFI spec conformance.
(Unless that's the case.)

- Please open-code the L"MainFormState" CHAR16 string as the third
argument of the HiiIsConfigHdrMatch() call in patch #3 (and please test
it too, with OVMF as well -- see if it remains possible to change the
preferred resolution).

With those changes I'll accept your patches, grudgingly.

This "paper over broken tools" has been going on since forever in OVMF,
the most common example being the *invalid* warning messages emitted by
various MSVC compilers, breaking the build for no good reason. Now the
SCT is being added to that list. I guess I'll just have to accept this
(very demoralizing) status quo.

Thanks
Laszlo

------------------------------------------------------------------------------

Heyi Guo

2015-06-02 02:58:34 UTC

Permalink

Hi Laszlo,

First, thanks for your time to review the code and reply with such
details (always learned from your replies), and it is OK to NAK these
patches if you are not happy to do so :)

If we are sure it is the bug of SCT, I agree it is better to modify SCT
rather than to work around in edk2 OVMF. Actually I'm preparing some
patches for SCT in the same time.

Yes, most of the code in the patches is coming from the reference of
other modules in edk2. I didn't think carefully enough on these example
codes, until your replies give me a chance to do so.

1. For BLOCK_IO_PROTOCOL MediaId issue, I agree it is not necessary to
check MediaId; I think it is necessary for removable media, isn't it? If
so, we can add a precondition for this test case in SCT.
2. For BLOCK_IO_PROTOCOL NULL buffer issue, I agree it is the caller's
responsibility according to UEFI 2.5.

So it is OK to NAK the 1/3 patch.

3. For HiiConfigAccess parameter check, the SPEC has below declaration
for RouteConfig on page 2100 (page 2149 overall):

In the 2nd row, there is an NULL check for Results, but this is clearly
a typo and probably "Progress"; however the SPEC doesn't keep consistent
with all interfaces (I didn't find similar statement for ExtractConfig).

4. For HiiConfigToBlock interface, the SPEC indicates "If present, the
function skips GUID, NAME, and PATH in <ConfigResp>", so it may be
reasonable for HiiConfigAccess->RouteConfig to check the GUID and NAME
field, but I didn't find it implicitly declared in the SPEC.

I may discuss the issues with you before sending patches directly, but I
thought the code would tell the story clearer and we can discuss based
on it.

Thanks.

Heyi Guo

Post by Laszlo Ersek

The SCT happens to be correct in catching this error.
However, I think the error is not in OVMF, but in the
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() implementation (exactly
as you say, it returns EFI_INVALID_PARAMETER, which is not correct).
EFI_NOT_FOUND
Target for the specified routing data was not found
If the driver's configuration is stored in a linear block of data and
the driver's name / value pairs are in <BlockConfig> format, it may
use the ConfigToBlock helper function (above) to simplify the job.
That's what we have here, hence the call to
gHiiConfigRouting->ConfigToBlock().
EFI_NOT_FOUND
Target for the specified routing data was not found. Progress
points to the G in GUID of the errant routing data.
So, if gHiiConfigRouting->ConfigToBlock() works okay, then this call in
RouteConfig() will satisfy the spec (and the SCT too).
Unfortunately, as you say, the implementation of
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() is not correct in this
regard.
Namely, I checked HiiConfigToBlock() in
"MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c". Its leading
comment documents EFI_NOT_FOUND, but the function never seems to return
EFI_NOT_FOUND. In fact it doesn't even try to *determine* if the GUID is
a match or not; it just skips over the GUID. I don't know how that could
be fixed easily -- maybe it's another issue with the UEFI spec.
Then I grepped the edk2 source code for invocations of
- most of the functions I peeked at use the same style error checks that
your patch 2/3 does.
- your HiiIsConfigHdrMatch() call seems to match existing practice
(based on other RouteConfig() implementations), but I would also like to
see the third parameter filled in, with L"MainFormState" rather than
NULL. Can you please test that?
In fact, seeing how your earlier patches actually just followed existing
edk2 practices is a *huge* disappointment for me (about edk2, not your
patchset). In any case, it's lucky for you, because I've stopped caring.
- Please review the commit messages on the patches, and adapt the
language to state "work around" or "paper over" or "suppress" invalid
SCT test errors. *Unless* you find a specific violation of the UEFI-2.5
spec, of course, in which case please spell out those locations
individually.
- For all new exit conditions and/or error values introduced, please
document them in each function's leading comment. There's no need to
over-emphasize it, but please be clear about the fact that these checks
/ retvals are being done for consistency with the rest of the edk2
codebase and/or due to SCT bugs, and not for UEFI spec conformance.
(Unless that's the case.)
- Please open-code the L"MainFormState" CHAR16 string as the third
argument of the HiiIsConfigHdrMatch() call in patch #3 (and please test
it too, with OVMF as well -- see if it remains possible to change the
preferred resolution).
With those changes I'll accept your patches, grudgingly.
This "paper over broken tools" has been going on since forever in OVMF,
the most common example being the *invalid* warning messages emitted by
various MSVC compilers, breaking the build for no good reason. Now the
SCT is being added to that list. I guess I'll just have to accept this
(very demoralizing) status quo.
Thanks
Laszlo

Laszlo Ersek

2015-06-02 07:19:06 UTC

Permalink

Post by Heyi Guo
Hi Laszlo,
First, thanks for your time to review the code and reply with such
details (always learned from your replies), and it is OK to NAK these
patches if you are not happy to do so :)
If we are sure it is the bug of SCT, I agree it is better to modify SCT
rather than to work around in edk2 OVMF. Actually I'm preparing some
patches for SCT in the same time.
Yes, most of the code in the patches is coming from the reference of
other modules in edk2. I didn't think carefully enough on these example
codes, until your replies give me a chance to do so.
1. For BLOCK_IO_PROTOCOL MediaId issue, I agree it is not necessary to
check MediaId; I think it is necessary for removable media, isn't it? If
so, we can add a precondition for this test case in SCT.

Cool. Not sure who your reviewer will be for the SCT patches (in fact I
don't know where the SCT *lives*). Anyway, if you run into trouble with
that, feel free to resubmit your edk2 series (with the things I asked
for in the 3/3 review).

Post by Heyi Guo
2. For BLOCK_IO_PROTOCOL NULL buffer issue, I agree it is the caller's
responsibility according to UEFI 2.5.
So it is OK to NAK the 1/3 patch.
3. For HiiConfigAccess parameter check, the SPEC has below declaration
In the 2nd row, there is an NULL check for Results, but this is clearly
a typo and probably "Progress";

okay

Post by Heyi Guo
however the SPEC doesn't keep consistent
with all interfaces (I didn't find similar statement for ExtractConfig).

Yep.

Post by Heyi Guo
4. For HiiConfigToBlock interface, the SPEC indicates "If present, the
function skips GUID, NAME, and PATH in <ConfigResp>",

Okay, that's an inconsistency in the spec then. If
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() shall ignore GUID, NAME
and PATH (ie. the routing info), then the error condition

EFI_NOT_FOUND Target for the specified routing data was not found.
Progress points to the “G” in “GUID” of the errant
routing data.

makes no sense. Can you perhaps raise this on the uswg list? (I saw you
posted another report the other day.)

Post by Heyi Guo
so it may be
reasonable for HiiConfigAccess->RouteConfig to check the GUID and NAME
field, but I didn't find it implicitly declared in the SPEC.

Right, it's probably reasonable, and it follows existent edk2 practice.

Post by Heyi Guo
I may discuss the issues with you before sending patches directly, but I
thought the code would tell the story clearer and we can discuss based
on it.

Ah, you mean you "could have discussed" it in advance. Don't bother; the
code always tells the story best (combined with detailed commit messages
& comments).

Thanks
Laszlo

Post by Heyi Guo
Thanks.
Heyi Guo

Post by Laszlo Ersek

The SCT happens to be correct in catching this error.
However, I think the error is not in OVMF, but in the
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() implementation (exactly
as you say, it returns EFI_INVALID_PARAMETER, which is not correct).
EFI_NOT_FOUND
Target for the specified routing data was not found
If the driver's configuration is stored in a linear block of data and
the driver's name / value pairs are in <BlockConfig> format, it may
use the ConfigToBlock helper function (above) to simplify the job.
That's what we have here, hence the call to
gHiiConfigRouting->ConfigToBlock().
EFI_NOT_FOUND
Target for the specified routing data was not found. Progress
points to the “G” in “GUID” of the errant routing data.
So, if gHiiConfigRouting->ConfigToBlock() works okay, then this call in
RouteConfig() will satisfy the spec (and the SCT too).
Unfortunately, as you say, the implementation of
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() is not correct in this
regard.
Namely, I checked HiiConfigToBlock() in
"MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c". Its leading
comment documents EFI_NOT_FOUND, but the function never seems to return
EFI_NOT_FOUND. In fact it doesn't even try to *determine* if the GUID is
a match or not; it just skips over the GUID. I don't know how that could
be fixed easily -- maybe it's another issue with the UEFI spec.
Then I grepped the edk2 source code for invocations of
- most of the functions I peeked at use the same style error checks that
your patch 2/3 does.
- your HiiIsConfigHdrMatch() call seems to match existing practice
(based on other RouteConfig() implementations), but I would also like to
see the third parameter filled in, with L"MainFormState" rather than
NULL. Can you please test that?
In fact, seeing how your earlier patches actually just followed existing
edk2 practices is a *huge* disappointment for me (about edk2, not your
patchset). In any case, it's lucky for you, because I've stopped caring.
- Please review the commit messages on the patches, and adapt the
language to state "work around" or "paper over" or "suppress" invalid
SCT test errors. *Unless* you find a specific violation of the UEFI-2.5
spec, of course, in which case please spell out those locations
individually.
- For all new exit conditions and/or error values introduced, please
document them in each function's leading comment. There's no need to
over-emphasize it, but please be clear about the fact that these checks
/ retvals are being done for consistency with the rest of the edk2
codebase and/or due to SCT bugs, and not for UEFI spec conformance.
(Unless that's the case.)
- Please open-code the L"MainFormState" CHAR16 string as the third
argument of the HiiIsConfigHdrMatch() call in patch #3 (and please test
it too, with OVMF as well -- see if it remains possible to change the
preferred resolution).
With those changes I'll accept your patches, grudgingly.
This "paper over broken tools" has been going on since forever in OVMF,
the most common example being the *invalid* warning messages emitted by
various MSVC compilers, breaking the build for no good reason. Now the
SCT is being added to that list. I guess I'll just have to accept this
(very demoralizing) status quo.
Thanks
Laszlo

------------------------------------------------------------------------------