[edk2,3/3] OvmfPkg: PlatformDxe: Add ConfigHdrMatch check in RouteConfig

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 <heyi.guo@linaro.org>
---
 OvmfPkg/PlatformDxe/Platform.c | 5 +++++
 1 file changed, 5 insertions(+)

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

On 06/01/2015 11:13 PM, Laszlo Ersek wrote:
> On 06/01/15 14:08, Heyi Guo wrote:
>> 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 <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
>>     //
>>
> 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
------------------------------------------------------------------------------