[02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h

A bunch of #defines and structs copied over from edk2,
mostly needed to decode and encode the messages in the
communication buffer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/uefi/var-service-edk2.h | 184 +++++++++++++++++++++++++++++
 1 file changed, 184 insertions(+)
 create mode 100644 include/hw/uefi/var-service-edk2.h

On 11/15/23 16:12, Gerd Hoffmann wrote:
> A bunch of #defines and structs copied over from edk2,
> mostly needed to decode and encode the messages in the
> communication buffer.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/uefi/var-service-edk2.h | 184 +++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
>  create mode 100644 include/hw/uefi/var-service-edk2.h
> 
> diff --git a/include/hw/uefi/var-service-edk2.h b/include/hw/uefi/var-service-edk2.h
> new file mode 100644
> index 000000000000..354b74d1d71c
> --- /dev/null
> +++ b/include/hw/uefi/var-service-edk2.h
> @@ -0,0 +1,184 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi-vars device - structs and defines from edk2
> + *
> + * Note: The edk2 UINTN type has been mapped to uint64_t,
> + *       so the structs are compatible with 64bit edk2 builds.

(1) What is the failure mode if the guest runs a 32-bit DXE phase and
tries to access this device?

> + */
> +#ifndef QEMU_UEFI_VAR_SERVICE_EDK2_H
> +#define QEMU_UEFI_VAR_SERVICE_EDK2_H
> +
> +#include "qemu/uuid.h"
> +
> +#define MAX_BIT                   0x8000000000000000ULL
> +#define ENCODE_ERROR(StatusCode)  (MAX_BIT | (StatusCode))
> +#define EFI_SUCCESS               0

(2) Probably better to make this 0ULL, so that its type is consistent
with that of the error codes.

(3) BTW, these error codes are not edk2-specific; they are from the UEFI
spec (Appendix D, "Status Codes"). I'm mentioning it because it might
clarify the commit message.

> +#define EFI_INVALID_PARAMETER     ENCODE_ERROR(2)
> +#define EFI_UNSUPPORTED           ENCODE_ERROR(3)
> +#define EFI_BAD_BUFFER_SIZE       ENCODE_ERROR(4)
> +#define EFI_BUFFER_TOO_SMALL      ENCODE_ERROR(5)

(4) any particular reason for skipping NOT_READY (6) and DEVICE_ERROR (7)?

(If this file only defines status codes that the code actually uses,
that's best!)

> +#define EFI_WRITE_PROTECTED       ENCODE_ERROR(8)
> +#define EFI_OUT_OF_RESOURCES      ENCODE_ERROR(9)
> +#define EFI_NOT_FOUND             ENCODE_ERROR(14)
> +#define EFI_ACCESS_DENIED         ENCODE_ERROR(15)
> +#define EFI_ALREADY_STARTED       ENCODE_ERROR(20)
> +
> +#define EFI_VARIABLE_NON_VOLATILE                           0x01
> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS                     0x02
> +#define EFI_VARIABLE_RUNTIME_ACCESS                         0x04
> +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD                  0x08
> +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS             0x10  // deprecated
> +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS  0x20
> +#define EFI_VARIABLE_APPEND_WRITE                           0x40
> +
> +/* SecureBootEnable */

(5) L"SecureBootEnable"

(6) mentioning the variable namespace GUID might be worthwhile as well
(gEfiSecureBootEnableDisableGuid -- F0A30BC7-AF08-4556-99C4-001009C93A44)

(7) this variable is an edk2 extension indeed; might be worth mentioning

> +#define SECURE_BOOT_ENABLE         1
> +#define SECURE_BOOT_DISABLE        0
> +
> +/* SecureBoot */

(8) L"SecureBoot"

(9) this one is standard, so the namespace GUID is not important to
mention -- but maybe mention that it is standard

> +#define SECURE_BOOT_MODE_ENABLE    1
> +#define SECURE_BOOT_MODE_DISABLE   0
> +
> +/* CustomMode */

the usual comments:

(10) L"CustomMode"

(11) GUID: gEfiCustomModeEnableGuid -- C076EC0C-7028-4399-A072-71EE5C448B9F

(12) edk2 extension

> +#define CUSTOM_SECURE_BOOT_MODE    1
> +#define STANDARD_SECURE_BOOT_MODE  0
> +
> +/* SetupMode */

(13) L"SetupMode"

(14) standard

> +#define SETUP_MODE                 1
> +#define USER_MODE                  0
> +
> +typedef uint64_t efi_status;
> +typedef struct mm_header mm_header;
> +
> +/* EFI_MM_COMMUNICATE_HEADER */
> +struct mm_header {
> +    QemuUUID  guid;
> +    uint64_t  length;
> +};

(15) QEMU_PACKED

> +
> +/* --- EfiSmmVariableProtocol ---------------------------------------- */

(16) this is a bit too cryptic like this; what we mean here is that
mm_header.guid is gEfiSmmVariableProtocolGuid
(ED32D533-99E6-4209-9CC0-2D72CDD998A7) for the following functions

(17) do you want to define SMM_VARIABLE_COMMUNICATE_HEADER as well?
(because the function codes make sense inside that header) -- ah wait,
those are below; OK

(18) slight inconsistency: the function macros are named SMM_*, but the
types are named mm_*.

However... that inconsistency seems to be there in edk2 to as well; we
don't have (for example) "MM_VARIABLE_FUNCTION_GET_VARIABLE"

> +
> +#define SMM_VARIABLE_FUNCTION_GET_VARIABLE            1
> +#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME  2
> +#define SMM_VARIABLE_FUNCTION_SET_VARIABLE            3
> +#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO     4
> +#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT           5
> +#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE       6
> +#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE           8
> +#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE       11
> +
> +typedef struct mm_variable mm_variable;
> +typedef struct mm_variable_access mm_variable_access;
> +typedef struct mm_next_variable mm_next_variable;
> +typedef struct mm_next_variable mm_lock_variable;
> +typedef struct mm_variable_info mm_variable_info;
> +typedef struct mm_get_payload_size mm_get_payload_size;
> +
> +/* SMM_VARIABLE_COMMUNICATE_HEADER */
> +struct mm_variable {
> +    uint64_t  function;
> +    uint64_t  status;
> +};

(19) you have a typedef for efi_status above; no need to decode it to
uint64_t here (for the status field)

(20) This too should be QEMU_PACKED. (Will not make a difference in
practice, but this is wire protocol, so best be clear.)

BTW I'm just noticing that SMM_VARIABLE_COMMUNICATE_HEADER in edk2's
"MdeModulePkg/Include/Guid/SmmVariableCommon.h" is *not* marked packed.
That's a bug in edk2, of course (documentation bug).

(21) a comment about the general request structure would be helpful:
mm_header + mm_variable + function payload (any).

> +
> +/* SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE */

(22) worth mentioning functions 1 and 3?

> +struct QEMU_PACKED mm_variable_access {

I can see you packed this! :)

> +    QemuUUID  guid;
> +    uint64_t  data_size;

correct, but we'll have to be careful with the range checks / buffer
size checks later, because UEFI_VARS_REG_BUFFER_SIZE is only 32-bit!

> +    uint64_t  name_size;
> +    uint32_t  attributes;
> +    /* Name */
> +    /* Data */
> +};
> +
> +/* SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME */

(23) worth mentioning function 2, or is that obvious from the name?

> +struct mm_next_variable {
> +    QemuUUID  guid;
> +    uint64_t  name_size;
> +    /* Name */
> +};

(24) should be packed

> +
> +/* SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO */
> +struct QEMU_PACKED mm_variable_info {
> +    uint64_t max_storage_size;
> +    uint64_t free_storage_size;
> +    uint64_t max_variable_size;
> +    uint32_t attributes;
> +};

(25) mention function 4?

> +
> +/* SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE */

(26) mention function 11?

> +struct mm_get_payload_size {
> +    uint64_t  payload_size;
> +};

(27) should be packed (just for documentation / exluding trailing padding)

> +
> +/* --- VarCheckPolicyLibMmiHandler ----------------------------------- */

(28) Please be clearer: this means that we place GUID
DA1B0D11-D1A7-46C4-9DC9-F3714875C6EB into mm_header.guid.

(29) Also record that the generic structure is mm_header +
mm_check_policy + command payload (if any).

> +
> +#define VAR_CHECK_POLICY_COMMAND_DISABLE     0x01
> +#define VAR_CHECK_POLICY_COMMAND_IS_ENABLED  0x02
> +#define VAR_CHECK_POLICY_COMMAND_REGISTER    0x03
> +#define VAR_CHECK_POLICY_COMMAND_DUMP        0x04
> +#define VAR_CHECK_POLICY_COMMAND_LOCK        0x05
> +
> +typedef struct mm_check_policy mm_check_policy;
> +typedef struct mm_check_policy_is_enabled mm_check_policy_is_enabled;
> +typedef struct mm_check_policy_dump_params mm_check_policy_dump_params;
> +
> +/* VAR_CHECK_POLICY_COMM_HEADER */
> +struct QEMU_PACKED mm_check_policy {
> +    uint32_t  signature;
> +    uint32_t  revision;
> +    uint32_t  command;
> +    uint64_t  result;

(30) field "result" should have "efi_status"

(31) define the macros for (a) signature (VCPC) and (b) revision (1)?

> +};
> +
> +/* VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS */

(32) mention command 2? (Just guessing)

> +struct QEMU_PACKED mm_check_policy_is_enabled {
> +    uint8_t   state;
> +};

(33) given that this is a "BOOLEAN" in the original, we might want to
introduce a typedef "edk2_boolean" for uint8_t. Just an idea.

> +
> +/* VAR_CHECK_POLICY_COMM_DUMP_PARAMS */

(34) mention command 4?

> +struct QEMU_PACKED mm_check_policy_dump_params {
> +    uint32_t  page_requested;
> +    uint32_t  total_size;
> +    uint32_t  page_size;
> +    uint8_t   has_more;

(35) consider updating the last field's type to "edk2_boolean".

> +};
> +
> +/* --- Edk2VariablePolicyProtocol ------------------------------------ */

(36) the proper spelling is "EdkiiVariablePolicyProtocol" (note: roman
numeral "ii" rather than arabic numeral 2)

(37) Mentioning this protocol here is confusing, IMO. Please correct me
if I'm wrong, but the protocol appears to be used between DXE
components, and not on the DXE<->SMM boundary.

Instead, we need the following ABIs as payloads for
VAR_CHECK_POLICY_COMMAND_REGISTER.

Therefore I would remove the EdkiiVariablePolicyProtocol mention above
altogether, and instead clarify that these are for:

- mm_header + mm_check_policy (command 3) + variable_policy_entry,
  or

- mm_header + mm_check_policy (command 3) +
  variable_policy_entry (policy 3) + variable_lock_on_var_state

> +
> +#define VARIABLE_POLICY_ENTRY_REVISION  0x00010000
> +
> +#define VARIABLE_POLICY_TYPE_NO_LOCK            0
> +#define VARIABLE_POLICY_TYPE_LOCK_NOW           1
> +#define VARIABLE_POLICY_TYPE_LOCK_ON_CREATE     2
> +#define VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE  3
> +
> +typedef struct variable_policy_entry variable_policy_entry;
> +typedef struct variable_lock_on_var_state variable_lock_on_var_state;
> +
> +/* VARIABLE_POLICY_ENTRY */
> +struct variable_policy_entry {

(38) should be packed

> +    uint32_t      version;
> +    uint16_t      size;
> +    uint16_t      offset_to_name;
> +    QemuUUID      namespace;
> +    uint32_t      min_size;
> +    uint32_t      max_size;

(39) also define the macros for "no min size" and "no max size"?

> +    uint32_t      attributes_must_have;
> +    uint32_t      attributes_cant_have;

(40) define the macros that invalidate these fields?

> +    uint8_t       lock_policy_type;
> +    uint8_t       padding[3];
> +    /* LockPolicy */
> +    /* Name */
> +};
> +
> +/* VARIABLE_LOCK_ON_VAR_STATE_POLICY */
> +struct variable_lock_on_var_state {

(41) should be packed

> +    QemuUUID      namespace;
> +    uint8_t       value;
> +    uint8_t       padding;
> +    /* Name */

(42) probably worth mentioning the link

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md

around these parts too, not just in patch 8.

> +};
> +
> +
> +#endif /* QEMU_UEFI_VAR_SERVICE_EDK2_H */

Laszlo