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

From: Gerd Hoffmann <kraxel@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	"Eric Blake" <eblake@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Michael Roth" <michael.roth@amd.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	graf@amazon.com, "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Markus Armbruster" <armbru@redhat.com>
Subject: Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
Date: Wed, 22 Nov 2023 16:12:32 +0100	[thread overview]
Message-ID: <gwrbpgv2ylc5fdfy4f6pjctft3kf3w75ksxvxocyc2whdtruvf@suvmqrdwbghb> (raw)
In-Reply-To: <a41a545b-c78e-f261-6f8e-e4d7ffe94767@redhat.com>

  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