Re: [SeaBIOS] [PATCH v6 5/5] QEMU fw_cfg: Write fw_cfg back on S3 resume

[Laszlo Ersek]

On 02/20/17 21:14, [e-mail] wrote:
> From: Ben Warren
>
> Any pointers to BIOS-allocated memory that were written back to QEMU
> fw_cfg files are replayed when resuming from S3 sleep.
> Signed-off-by: Ben Warren
>
> src/fw/romfile_loader.c | 33 +++++++++++++++++++++++++++++++++
>  src/fw/romfile_loader.h |  2 ++
>  src/resume.c            |  4 ++++
>  3 files changed, 39 insertions(+)
>
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index 30e7b58..14bc908 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -4,6 +4,7 @@
>  #include "string.h" // strcmp
>  #include "romfile.h" // struct romfile_s
>  #include "malloc.h" // Zone*, _malloc
> +#include "list.h" // struct hlist_node
>  #include "output.h" // warn_*
>  #include "paravirt.h" // qemu_cfg_write_file
>
> @@ -16,6 +17,16 @@ struct romfile_loader_files {
>      struct romfile_loader_file files[];
>  };
>
> +// Data structures for storing "write pointer" entries for possible replay
> +struct romfile_wr_pointer_entry {
> u64 pointer;
> u32 offset;
> u16 key;
> u8 ptr_size;
> struct hlist_node node;
>
> +};
> +static struct hlist_head romfile_pointer_list;
>
> static struct romfile_loader_file *
>  romfile_loader_find(const char *name,
>                      struct romfile_loader_files *files)
> @@ -29,6 +40,19 @@ romfile_loader_find(const char *name,
>      return NULL;
>  }
>
> +// Replay "write pointer" entries back to QEMU
> +void romfile_fw_cfg_resume(void)
> +{
> if (!CONFIG_QEMU)
>    return;
> struct romfile_wr_pointer_entry *entry;
> hlist_for_each_entry(entry, &romfile_pointer_list, node) {
>    qemu_cfg_write_file_simple(&entry->pointer, entry->key,
>                               entry->offset, entry->ptr_size);
> }
>
> +}
>
> static void romfile_loader_allocate(struct romfile_loader_entry_s *entry,
>                                      struct romfile_loader_files *files)
>  {
> @@ -163,6 +187,15 @@ static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry,
>                              entry->wr_pointer.size) != entry->wr_pointer.size) {
>          goto err;
>      }
> /* Store the info so it can replayed later if necessary */
> struct romfile_wr_pointer_entry *store = malloc_high(sizeof(*store));
> store->pointer = pointer;
> store->key = qemu_get_romfile_key(dest_file);

I suggested to remove the error checking here, because
qemu_get_romfile_key() couldn't fail.

However, after Kevin's suggestion for qemu_get_romfile_key(), i.e., to
verify file->copy, that function can still fail.

... But, by the time we get here, we've already used the selector key
implicitly in the call to qemu_cfg_write_file(). And,
qemu_cfg_write_file() does the file->copy check, returns errors
appropriately, and we do check its retval.

So I agree that checking qemu_get_romfile_key()'s retval in addition
would buy us nothing; it's safe like this.

Reviewed-by: Laszlo Ersek

Thanks
Laszlo
> store->offset = dst_offset;
> store->ptr_size = entry->wr_pointer.size;
> hlist_add_head(&store->node, &romfile_pointer_list);
> return;
> err:
>    warn_internalerror();
>
> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> index 4dc50ab..fcd4ab2 100644
> --- a/src/fw/romfile_loader.h
> +++ b/src/fw/romfile_loader.h
> @@ -86,4 +86,6 @@ enum {
>
> int romfile_loader_execute(const char *name);
>
> +void romfile_fw_cfg_resume(void);
>
> #endif
> diff --git a/src/resume.c b/src/resume.c
> index e67cfce..99fa34f 100644
> --- a/src/resume.c
> +++ b/src/resume.c
> @@ -17,6 +17,7 @@
>  #include "string.h" // memset
>  #include "util.h" // dma_setup
>  #include "tcgbios.h" // tpm_s3_resume
> +#include "fw/romfile_loader.h" // romfile_fw_cfg_resume
>
> // Handler for post calls that look like a resume.
>  void VISIBLE16
> @@ -105,6 +106,9 @@ s3_resume(void)
>      tpm_s3_resume();
>      s3_resume_vga();
>
> /* Replay any fw_cfg entries that go back to the host */
>
> romfile_fw_cfg_resume();
>
> make_bios_readonly();
>
> // Invoke the resume vector.