Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h

[Laszlo Ersek]

On 11/15/23 16:12, Gerd Hoffmann wrote:
> Add state structs and function declarations for the uefi-vars device.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/uefi/var-service.h | 119 ++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>  create mode 100644 include/hw/uefi/var-service.h
> 
> diff --git a/include/hw/uefi/var-service.h b/include/hw/uefi/var-service.h
> new file mode 100644
> index 000000000000..2b8d3052e59f
> --- /dev/null
> +++ b/include/hw/uefi/var-service.h
> @@ -0,0 +1,119 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi-vars device - state struct and function prototypes
> + */
> +#ifndef QEMU_UEFI_VAR_SERVICE_H
> +#define QEMU_UEFI_VAR_SERVICE_H
> +
> +#include "qemu/uuid.h"
> +#include "qemu/queue.h"
> +
> +#include "hw/uefi/var-service-edk2.h"
> +
> +#define MAX_BUFFER_SIZE (64 * 1024)
> +
> +typedef struct uefi_variable uefi_variable;
> +typedef struct uefi_var_policy uefi_var_policy;
> +typedef struct uefi_vars_state uefi_vars_state;
> +
> +struct uefi_variable {
> +    QemuUUID                          guid;
> +    uint16_t                          *name;
> +    uint32_t                          name_size;
> +    uint32_t                          attributes;
> +    void                              *data;
> +    uint32_t                          data_size;
> +    QTAILQ_ENTRY(uefi_variable)       next;
> +};
> +
> +struct uefi_var_policy {
> +    variable_policy_entry             *entry;
> +    uint32_t                          entry_size;
> +    uint16_t                          *name;
> +    uint32_t                          name_size;
> +    uint32_t                          hashmarks;
> +    QTAILQ_ENTRY(uefi_var_policy)     next;
> +};

- I wonder if the size fields should be size_t. uint32_t is not wrong
either; we'll just have to be careful when doing comparisons etc.

- care to explain (in a comment) hashmarks? I think it's related to the
wildcard policy stuff, but a hint would be appreciated.

> +
> +struct uefi_vars_state {
> +    MemoryRegion                      mr;
> +    uint16_t                          sts;
> +    uint32_t                          buf_size;
> +    uint32_t                          buf_addr_lo;
> +    uint32_t                          buf_addr_hi;

spelling out endianness here would be useful IMO

> +    uint8_t                           *buffer;
> +    QTAILQ_HEAD(, uefi_variable)      variables;
> +    QTAILQ_HEAD(, uefi_var_policy)    var_policies;
> +
> +    /* boot phases */
> +    bool                              end_of_dxe;
> +    bool                              ready_to_boot;
> +    bool                              exit_boot_service;

There are some variations of the 8 possible that don't make sense. at
the same time, a single enum could be too limiting. depends on what the
code will do with these.

> +    bool                              policy_locked;
> +
> +    /* storage accounting */
> +    uint64_t                          max_storage;
> +    uint64_t                          used_storage;
> +
> +    char                              *jsonfile;
> +    int                               jsonfd;
> +};
> +
> +/* vars-service-guid.c */
> +extern QemuUUID EfiGlobalVariable;
> +extern QemuUUID EfiImageSecurityDatabase;
> +extern QemuUUID EfiCustomModeEnable;
> +extern QemuUUID EfiSecureBootEnableDisable;
> +extern QemuUUID EfiSmmVariableProtocolGuid;
> +extern QemuUUID VarCheckPolicyLibMmiHandlerGuid;
> +extern QemuUUID EfiEndOfDxeEventGroupGuid;
> +extern QemuUUID EfiEventReadyToBootGuid;
> +extern QemuUUID EfiEventExitBootServicesGuid;

the spelling of these names appears a bit questionable:

- camelcase is idiomatic in edk2, but (I think?) not in QEMU, for variables

- the "Guid" suffix is inconsistently used / carried over from edk2

> +
> +/* vars-service-core.c */
> +extern const VMStateDescription vmstate_uefi_vars;
> +size_t uefi_strlen(const uint16_t *str, size_t len);
> +gboolean uefi_str_equal(const uint16_t *a, size_t alen,
> +                        const uint16_t *b, size_t blen);
> +char *uefi_ucs2_to_ascii(const uint16_t *ucs2, uint64_t ucs2_size);
> +void uefi_trace_variable(const char *action, QemuUUID guid,
> +                         const uint16_t *name, uint64_t name_size);
> +void uefi_trace_status(const char *action, efi_status status);
> +void uefi_vars_init(Object *obj, uefi_vars_state *uv);
> +void uefi_vars_realize(uefi_vars_state *uv, Error **errp);
> +void uefi_vars_hard_reset(uefi_vars_state *uv);
> +
> +/* vars-service-json.c */
> +void uefi_vars_json_init(uefi_vars_state *uv, Error **errp);
> +void uefi_vars_json_save(uefi_vars_state *uv);
> +void uefi_vars_json_load(uefi_vars_state *uv, Error **errp);
> +
> +/* vars-service-vars.c */
> +extern const VMStateDescription vmstate_uefi_variable;
> +uefi_variable *uefi_vars_find_variable(uefi_vars_state *uv, QemuUUID guid,
> +                                       const uint16_t *name,
> +                                       uint64_t name_size);
> +void uefi_vars_set_variable(uefi_vars_state *uv, QemuUUID guid,
> +                            const uint16_t *name, uint64_t name_size,
> +                            uint32_t attributes,
> +                            void *data, uint64_t data_size);
> +void uefi_vars_clear_volatile(uefi_vars_state *uv);
> +void uefi_vars_clear_all(uefi_vars_state *uv);
> +void uefi_vars_update_storage(uefi_vars_state *uv);
> +uint32_t uefi_vars_mm_vars_proto(uefi_vars_state *uv);
> +
> +/* vars-service-auth.c */
> +void uefi_vars_auth_init(uefi_vars_state *uv);
> +
> +/* vars-service-policy.c */
> +extern const VMStateDescription vmstate_uefi_var_policy;
> +efi_status uefi_vars_policy_check(uefi_vars_state *uv,
> +                                  uefi_variable *var,
> +                                  gboolean is_newvar);
> +void uefi_vars_policies_clear(uefi_vars_state *uv);
> +uefi_var_policy *uefi_vars_add_policy(uefi_vars_state *uv,
> +                                      variable_policy_entry *pe);
> +uint32_t uefi_vars_mm_check_policy_proto(uefi_vars_state *uv);
> +
> +#endif /* QEMU_UEFI_VAR_SERVICE_H */

I guess I'll have to see these in use to think anything of them.

(I prefer a more "functional" structuring for a series, where the thing
sort of builds & works from patch#1 onwards, it's only the actual
functionality that is introduced layer by layer. But, that's not an
objection; this patch certainly works as the collection of APIs the rest
is going to implement and call later.)

Again we'll have to keep an eye on the integer types.

with some comments inserted:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Laszlo