Re: [PATCH v3 09/23] hw/uefi: add var-service-core.c

[Alexander Graf <graf@amazon.com>]

On 11.02.25 10:23, Gerd Hoffmann wrote:
> This is the core code for guest <-> host communication.  This accepts
> request messages from the guest, dispatches them to the service called,
> and sends back the response message.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/uefi/var-service-core.c | 237 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 237 insertions(+)
>   create mode 100644 hw/uefi/var-service-core.c
>
> diff --git a/hw/uefi/var-service-core.c b/hw/uefi/var-service-core.c
> new file mode 100644
> index 000000000000..78a668e68fa2
> --- /dev/null
> +++ b/hw/uefi/var-service-core.c
> @@ -0,0 +1,237 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi vars device
> + */
> +#include "qemu/osdep.h"
> +#include "system/dma.h"
> +#include "migration/vmstate.h"
> +
> +#include "hw/uefi/var-service.h"
> +#include "hw/uefi/var-service-api.h"
> +#include "hw/uefi/var-service-edk2.h"
> +
> +#include "trace/trace-hw_uefi.h"
> +
> +static int uefi_vars_pre_load(void *opaque)
> +{
> +    uefi_vars_state *uv = opaque;
> +
> +    uefi_vars_clear_all(uv);
> +    uefi_vars_policies_clear(uv);
> +    g_free(uv->buffer);
> +    return 0;
> +}
> +
> +static int uefi_vars_post_load(void *opaque, int version_id)
> +{
> +    uefi_vars_state *uv = opaque;
> +
> +    uefi_vars_update_storage(uv);
> +    uv->buffer = g_malloc(uv->buf_size);
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_uefi_vars = {
> +    .name = "uefi-vars",
> +    .pre_load = uefi_vars_pre_load,
> +    .post_load = uefi_vars_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(sts, uefi_vars_state),
> +        VMSTATE_UINT32(buf_size, uefi_vars_state),
> +        VMSTATE_UINT32(buf_addr_lo, uefi_vars_state),
> +        VMSTATE_UINT32(buf_addr_hi, uefi_vars_state),
> +        VMSTATE_BOOL(end_of_dxe, uefi_vars_state),
> +        VMSTATE_BOOL(ready_to_boot, uefi_vars_state),
> +        VMSTATE_BOOL(exit_boot_service, uefi_vars_state),
> +        VMSTATE_BOOL(policy_locked, uefi_vars_state),
> +        VMSTATE_UINT64(used_storage, uefi_vars_state),
> +        VMSTATE_QTAILQ_V(variables, uefi_vars_state, 0,
> +                         vmstate_uefi_variable, uefi_variable, next),
> +        VMSTATE_QTAILQ_V(var_policies, uefi_vars_state, 0,
> +                         vmstate_uefi_var_policy, uefi_var_policy, next),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static uint32_t uefi_vars_cmd_mm(uefi_vars_state *uv)
> +{
> +    hwaddr    dma;
> +    mm_header *mhdr;
> +    uint64_t  size;
> +    uint32_t  retval;
> +
> +    dma = uv->buf_addr_lo | ((hwaddr)uv->buf_addr_hi << 32);
> +    mhdr = (mm_header *) uv->buffer;
> +
> +    if (!uv->buffer || uv->buf_size < sizeof(*mhdr)) {
> +        return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
> +    }
> +
> +    /* read header */
> +    dma_memory_read(&address_space_memory, dma,
> +                    uv->buffer, sizeof(*mhdr),
> +                    MEMTXATTRS_UNSPECIFIED);


Depending on DMA sounds appealing at first, but can fall apart in corner 
cases. I know of 2 cases where DMA failed for me in the EC2 equivalent 
of this:

1) SEV-SNP. If you want the hypervisor to implement UEFI variable 
services for you, the buffer region must always be in shared state. 
Ensuring that during boot time is tricky but doable. At runtime you no 
longer really have control over the sharability of pages.

2) Mac OS X. MacOS is the only OS I'm aware of that really makes use of 
relocation. They move your physical pages to random locations, give you 
a non-1:1 mapping to that and once you're in real OS land, you have no 
more knowledge at all about the physical location of anything. Maybe you 
can work around that by declaring the buffer region as MMIO space? But 
then it really should be a memory region in the device.


To address the 2 cases above, I ended up implementing a special "PIO 
mode" which does not rely on DMA at all:

https://github.com/aws/uefi/blob/main/edk2-stable202211/0023-edk2-stable202211-ExtVarStore-Add-support-for-PIO-transfer.patch

Also, I'm surprised you cut the variable service off at the SMM boundary 
instead of the RTS callback boundary. Why is that cleaner/better than 
implementing variables completely in QEMU? In the EC2 version, we just 
built a separate variable store implementation that completely replaces 
the edk2 variable store:

https://github.com/aws/uefi/blob/main/edk2-stable202211/0012-edk2-stable202211-nitro-Add-ExtVarStore-for-vmm-based-variable-storage.patch

It would be nice to agree on a single external variable store 
implementation :).


Alex