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

2023-11-22 Thread Gerd Hoffmann
  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




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

2023-11-17 Thread 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 
> ---
>  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 ..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,
> +