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

Gerd Hoffmann posted 16 patches 1 year ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>, Eric Blake <eblake@redhat.com>
[PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
Posted by Gerd Hoffmann 1 year ago
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;
+};
+
+struct uefi_vars_state {
+    MemoryRegion                      mr;
+    uint16_t                          sts;
+    uint32_t                          buf_size;
+    uint32_t                          buf_addr_lo;
+    uint32_t                          buf_addr_hi;
+    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;
+    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;
+
+/* 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 */
-- 
2.41.0
Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
Posted by Laszlo Ersek 1 year ago
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
Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
Posted by Gerd Hoffmann 1 year ago
  Hi,

> > +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.

I can't store size_t in vmstate ...

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

Yes.  It's the number of hashmarks, used when sorting entries by
priority.  Most specific match, i.e. smallest number of wildcard
characters, goes first.

I'll add a comment.

> > +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

Don't think this is needed, qemu handles this for us.  The memory
region is declared to be little endian, qemu will convert reads/writes
to native endian for us, so there are no endian worries for the register
interface (the transfer buffer in guest ram is a different story).

> > +    /* 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.

end-of-dxe: used by variable policies (they are enforced only after that
event).

ready-to-boot: not used yet (other than setting it when the event arrives).

exit-boot-service: access control (RT vs. BT etc).

> > +/* 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

Yes.  It's carried over from edk2.

We don't have to keep the names in qemu, in fact I've renamed the
structs because that would have been too much of a contrast to the
qemu code style, so the edk2 name is only noted in a comment in the
vars-service-edk2.h header file.

For the #defines and GUIDs I've kept the edk2 names as-is, so it's
easier to follow the control flow for people which are familiar with
edk2.  We have already have simliar stuff else, for example the struct
names in hw/usb/desc.h follow the usb spec not qemu code style.

take care,
  Gerd