Re: [PATCH v2] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support

[Gerd Hoffman]

  Hi,

> +Fw-cfg Files
> +************
> +
> +Guests drive vmfwupdate through special ``fw-cfg`` files that control its 
> flow
> +followed by a standard system reset operation. When vmfwupdate is available,
> +it provides the following ``fw-cfg`` files:
> +
> +* ``vmfwupdate/cap`` (``u64``) - Read-only Little Endian encoded bitmap of 
> additional
> +  capabilities the interface supports. List of available capabilities:
> +
> +     ``VMFWUPDATE_CAP_BIOS_RESIZE        0x0000000000000001``
> +
> +* ``vmfwupdate/bios-size`` (``u32``) - Little Endian encoded size of the 
> BIOS region.
> +  Read-only by default. Optionally Read-write if ``vmfwupdate/cap`` contains
> +  ``VMFWUPDATE_CAP_BIOS_RESIZE``. On write, the BIOS region may resize. 
> Guests are
> +  required to read the value after writing and compare it with the requested 
> size
> +  to determine whether the resize was successful. Note, x86 BIOS regions 
> always
> +  start at 4GiB - bios-size.
> +
> +* ``vmfwupdate/opaque`` (``1024 bytes``) - A 1KiB buffer that survives the 
> BIOS replacement
> +  flow. Can be used by the guest to propagate guest physical addresses of 
> payloads
> +  to its BIOS stage. It’s recommended to make the new BIOS clear this file 
> on boot
> +  if it exists. Contents of this file are under control by the hypervisor. 
> In an
> +  environment that considers the hypervisor outside of its trust boundary, 
> guests
> +  are advised to validate its contents before consumption.
> +
> +* ``vmfwupdate/disable`` (``u8``) - Indicates whether the interface is 
> disabled.
> +  Returns 0 for enabled, 1 for disabled. Writing any value disables it. 
> Writing is
> +  only allowed if the value is 0. When the interface is disabled, the 
> replace file
> +  is ignored on reset. This value resets to 0 on system reset.
> +
> +* ``vmfwupdate/bios-addr`` (``u64``) - A 64bit Little Endian encoded guest 
> physical address
> +  at the beginning of the replacement BIOS region. The provided payload must 
> reside
> +  in shared memory. 0 on system reset.

I'd suggest to make all integers u64 (little endian).

Any specific reason why vmfwupdate/opaque is 1024 bytes?
How about using 4096 (aka PAGE_SIZE) bytes?

> +On reset, the hypervisor evaluates whether ``vmfwupdate/disable`` is ``1``. 
> If it is, it ignores
> +any other vmfwupdate values and performs a standard system reset.
> +
> +If ``vmfwupdate/disable`` is ``0``, the hypervisor checks if bios-addr is 
> ``0``. If it is, it
> +performs a standard system reset.

What is 'standard system reset' in SEV-SNP mode?  I think qemu does not
support it right now and will simply poweroff the guest instead.  Will
that continue to be the case?  Or should qemu re-create the VM context
in that case, simliar to the firmware update case, except that qemu
would have to use the default firmware image then?

Will the vmfwupdate be supported without SEV-SNP too?  Might be useful
for development + testing.  If so the spec should clarify what happens
in that case, because the concept of "shared memory" does not exist
then.

take care,
  Gerd