Hi Heinrich, 2021年6月30日(水) 23:55 Heinrich Schuchardt <[email protected]>:
> >> This does not catch attributes == EFI_VARIABLE_NON_VOLATILE where test > >> case 5.2.1.4.5 requires EFI_INVALID_PARAMETER. > > > > Hmm, but this could pass the SCT runtime test. > > So attributes == EFI_VARIABLES_NON_VOLATILE should fail? > > Actually, I referred to the VariableServiceQueryVariableInfo()@EDK2 > > and UEFI spec, > > and I couldn't see such conditions. > > The SCT specification requires in 5.2.1.4.5.: > > "1. Call QueryVariableInfoservice with the Attributes: > > * EFI_VARIABLE_NON_VOLATILE > * EFI_VARIABLE_RUNTIME_ACCESS > * EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS > > The returned code must be EFI_INVALID_PARAMETER." Ah, I see. so either NON_VOLATILE and RUNTIME_ACCESS must be used with BOOTSERVICE_ACCESS. > > See patch > > [PATCH edk2-test 1/1] uefi-sct/SctPkg: uefi-sct: > QueryVariableInfo(EFI_VARIABLE_NON_VOLATILE) > https://edk2.groups.io/g/devel/message/77367 > > > > >> > >> Shouldn't we check > >> > >> !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) > >> > >> instead? > > > > Ah, right, because this function is only used for the bootservice. > > (I found that runtime service uses another function) > > A variable without EFI_VARIABLE_BOOTSERVICE_ACCESS could neither be > accessed before nor after ExitBootServices(). So this has to be invalid. OK, so BOOTSERVICE_ACCESS is required. Hmm, but in that case, if we confirm BOOTSERVICE_ACCESS bit is set, should we still need to check NON_VOLATILE and RUNTIME_ACCESS? I mean if (!(attr & BOOTSERVICE_ACCESS)) return INVALID_PARAM; else if (...) /* at this point, attr must have the BOOTSERVICE_ACCESS */ Thus, attributes shouldn't be equal to any of; > * EFI_VARIABLE_NON_VOLATILE > * EFI_VARIABLE_RUNTIME_ACCESS > * EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS So, I think we only need " !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) " this check. Best regards, > > Best regards > > Heinrich > > > > >> > >>> + ret = EFI_INVALID_PARAMETER; > >>> + } else if ((attributes & (EFI_VARIABLE_NON_VOLATILE | > >>> EFI_VARIABLE_HARDWARE_ERROR_RECORD)) == > >>> EFI_VARIABLE_HARDWARE_ERROR_RECORD) { > >>> + /* HW error occurs only on non-volatile variables */ > >> > >> We don't support EFI_VARIABLE_HARDWARE_ERROR_RECORD at all. So shouldn't > >> flag EFI_VARIABLE_HARDWARE_ERROR_RECORD result in EFI_UNSUPPORTED? > > > > OK, I'll do. > > > > BTW, from the UEFI spec, EFI_VARIABLE_APPEND_WRITE should be ignored > > in the QueryVariableInfo because that flag is used for SetVariables > > (as overwrite flag). > > Thus, if attributes == EFI_VARIABLE_APPEND_WRITE, should we return > > EFI_INVALID_PARAMETER? > > > >> > >>> + ret = EFI_INVALID_PARAMETER; > >>> + } else { > >>> + ret = efi_query_variable_info_int(attributes, > >>> maximum_variable_storage_size, > >>> remaining_variable_storage_size, > >>> maximum_variable_size); > >> > >> CHECK: Alignment should match open parenthesis > >> #44: FILE: lib/efi_loader/efi_var_common.c:184: > >> + ret = efi_query_variable_info_int(attributes, > >> maximum_variable_storage_size, > > > > OK, let me fix that. > > > > Thank you, > > > >> > >> Best regards > >> > >> Heinrich > >> > >>> + } > >>> > >>> return EFI_EXIT(ret); > >>> } > >>> > >> > > > > > > -- > > Masami Hiramatsu > > > -- Masami Hiramatsu
