Re: [SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address of file

[Laszlo Ersek]

Ben,

On 02/05/17 18:09, [e-mail] wrote:
> From: Ben Warren
>
> This command is similar to ADD_POINTER, but instead of patching
> memory, it writes the pointer back to QEMU over the DMA interface.
> Signed-off-by: Ben Warren
>
> src/fw/romfile_loader.c | 37 +++++++++++++++++++++++++++++++++++++
>  src/fw/romfile_loader.h | 16 ++++++++++------
>  2 files changed, 47 insertions(+), 6 deletions(-)

while working on the OVMF patches for WRITE_POINTER, something else
occurred to me.

As I mentioned in the QEMU thread,

https://www.mail-archive.com/[e-mail]/msg428495.html

the VMGENID device (or more generally, each device that produces
WRITE_POINTER command(s)) should have a reset handler that makes the
device forget the GPA(s) that the firmware communicated to it, via
WRITE_POINTER. This is because after system reset, all earlier GPAs
become meaningless.

What I want to mention here is the corollary to the above. ACPI S3
resume is basically a reset, but the GPAs remain valid, because system
memory is preserved. Therefore, at S3 resume, the guest firmware has to
replay all the WRITE_POINTER commands. The QEMU reset handler will
forcibly forget about the GPAs (which is correct), so the firmware has
to re-store them.

I think it can be a separate patch for SeaBIOS (but preferably in this
series). I plan to do this in OVMF as well.

Thanks!
Laszlo
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index f4b17ff..d0ae42b 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -5,6 +5,7 @@
>  #include "romfile.h" // struct romfile_s
>  #include "malloc.h" // Zone*, _malloc
>  #include "output.h" // warn_*
> +#include "paravirt.h" // qemu_cfg_write_file
>
> struct romfile_loader_file {
>      struct romfile_s *file;
> @@ -98,7 +99,39 @@ static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry,
>      pointer += (unsigned long)src_file->data;
>      pointer = cpu_to_le64(pointer);
>      memcpy(dest_file->data + offset, &pointer, entry->pointer_size);
> return;
>
> +err:
> warn_internalerror();
>
> +}
>
> +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry,
>                                   struct romfile_loader_files *files)
>
> +{
>
> struct romfile_s *dest_file;
>
> struct romfile_loader_file *src_file;
>
> unsigned offset = le32_to_cpu(entry->pointer_offset);
>
> u64 pointer = 0;
>
> /* Writing back to a file that may not be loaded in RAM */
>
> dest_file = romfile_find(entry->pointer_dest_file);
>
> src_file = romfile_loader_find(entry->pointer_src_file, files);
>
> if (!dest_file || !src_file || !src_file->data ||
>    offset + entry->pointer_size < offset ||
>    offset + entry->pointer_size > dest_file->size ||
>    entry->pointer_size < 1 || entry->pointer_size > 8 ||
>    entry->pointer_size & (entry->pointer_size - 1)) {
>    goto err;
>
> }
>
> pointer = (unsigned long)src_file->data;
>
> pointer = cpu_to_le64(pointer);
>
> /* Only supported on QEMU */
>
> if (qemu_cfg_write_file(&pointer, dest_file, offset,
>                        entry->pointer_size) != entry->pointer_size) {
>    goto err;
>
> }
> return;
>
> err:
>      warn_internalerror();
> @@ -161,6 +194,10 @@ int romfile_loader_execute(const char *name)
>                          break;
>                  case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
>                          romfile_loader_add_checksum(entry, files);
>                    break;
>            case ROMFILE_LOADER_COMMAND_WRITE_POINTER:
>                    romfile_loader_write_pointer(entry, files);
>                    break;
>            default:
>                    /* Skip commands that we don't recognize. */
>                    break;
>
> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> index 15eab2a..0c0782c 100644
> --- a/src/fw/romfile_loader.h
> +++ b/src/fw/romfile_loader.h
> @@ -25,10 +25,13 @@ struct romfile_loader_entry_s {
>          };
>      /*
>     * COMMAND_ADD_POINTER - patch the table (originating from
>     * @dest_file) at @pointer_offset, by adding a pointer to the table
>     * COMMAND_ADD_POINTER &
>     * COMMAND_WRITE_POINTER - patch memory (originating from
>     * @dest_file) at @pointer.offset, by adding a pointer to the memory
>     * originating from @src_file. 1,2,4 or 8 byte unsigned
>     * addition is used depending on @pointer_size.
>     * addition is used depending on @pointer.size.
>     * Instead of patching memory, COMMAND_WRITE_POINTER writes the changes
>     * to @dest_file back to the host via DMA
>     */
>    struct {
>        char pointer_dest_file[ROMFILE_LOADER_FILESZ];
>
> @@ -57,9 +60,10 @@ struct romfile_loader_entry_s {
>  };
>
> enum {
> ROMFILE_LOADER_COMMAND_ALLOCATE     = 0x1,
> ROMFILE_LOADER_COMMAND_ADD_POINTER  = 0x2,
> ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> ROMFILE_LOADER_COMMAND_ALLOCATE          = 0x1,
> ROMFILE_LOADER_COMMAND_ADD_POINTER       = 0x2,
> ROMFILE_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
> ROMFILE_LOADER_COMMAND_WRITE_POINTER     = 0x4,
>
> };
>
> enum {