Re: [SeaBIOS] [PATCH v6 4/5] QEMU fw_cfg: Add functions for accessing files by key

[Laszlo Ersek]

On 02/20/17 21:14, [e-mail] wrote:
> From: Ben Warren
>
> Due to memory contraints, when resuming from S3 the fw_cfg "files" API
> isn't available.  This adds a simple API to get a file 'key', and to
> write to the file using the key as a reference.
> Signed-off-by: Ben Warren
>
> src/fw/paravirt.c | 32 ++++++++++++++++++++++++++------
>  src/fw/paravirt.h |  2 ++
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 4618647..225b08b 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -329,6 +329,17 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
>      return file->size;
>  }
>
> +// Bare-bones function for writing a file knowing only its unique
> +// identifying key (select)
> +int
> +qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len)
> +{
> qemu_cfg_select(key);
> qemu_cfg_skip(offset);
> qemu_cfg_write(src, len);
> return len;
>
> +}
>
> int
>  qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
>  {
> @@ -339,15 +350,12 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
>          warn_internalerror();
>          return -1;
>      }
> struct qemu_romfile_s *qfile;
> qfile = container_of(file, struct qemu_romfile_s, file);
> u16 key = qemu_get_romfile_key(file);
> if (offset == 0) {
>     /* Do it in one transfer */
>    qemu_cfg_write_entry(src, qfile->select, len);
>    qemu_cfg_write_entry(src, key, len);
> } else {
>    qemu_cfg_select(qfile->select);
>    qemu_cfg_skip(offset);
>    qemu_cfg_write(src, len);
>    qemu_cfg_write_file_simple(src, key, offset, len);
> }
>    return len;
>
> }

One of the ideas that I mentioned here was to move not just the second
branch of the "if" to qemu_cfg_write_file_simple(), but the entire "if"
-- both branches. Because, qemu_cfg_write_entry() looks suitable for S3
too, and if that kind of optimization makes sense for normal boot, then
it makes sense for S3 resume as well.

Anyway, this is not a functional problem, I won't obsess about it.

Reviewed-by: Laszlo Ersek

Thanks
Laszlo
> @@ -370,6 +378,18 @@ qemu_romfile_add(char *name, int select, int skip, int size)
>  }
>
> u16
> +qemu_get_romfile_key(struct romfile_s *file)
> +{
> struct qemu_romfile_s *qfile;
> if (file->copy != qemu_cfg_read_file) {
>    warn_internalerror();
>    return 0;
> }
> qfile = container_of(file, struct qemu_romfile_s, file);
> return qfile->select;
>
> +}
>
> +u16
>  qemu_get_present_cpus_count(void)
>  {
>      u16 smp_count = 0;
> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> index fb220d8..16f3d9a 100644
> --- a/src/fw/paravirt.h
> +++ b/src/fw/paravirt.h
> @@ -56,5 +56,7 @@ void qemu_cfg_init(void);
>
> u16 qemu_get_present_cpus_count(void);
>  int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len);
> +int qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len);
> +u16 qemu_get_romfile_key(struct romfile_s *file);
>
> #endif

[PATCH v6 4/5] QEMU fw_cfg: Add functions for accessing files by key

[Ben Warren]

> On Feb 20, 2017, at 1:06 PM, Laszlo Ersek  wrote:
>
> On 02/20/17 21:14, [e-mail]  wrote:
> > From: Ben Warren
> >
> > Due to memory contraints, when resuming from S3 the fw_cfg "files" API
> > isn't available.  This adds a simple API to get a file 'key', and to
> > write to the file using the key as a reference.
> > Signed-off-by: Ben Warren
> >
> > src/fw/paravirt.c | 32 ++++++++++++++++++++++++++------
> > src/fw/paravirt.h |  2 ++
> > 2 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> > index 4618647..225b08b 100644
> > --- a/src/fw/paravirt.c
> > +++ b/src/fw/paravirt.c
> > @@ -329,6 +329,17 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> >     return file->size;
> > }
> >
> > +// Bare-bones function for writing a file knowing only its unique
> > +// identifying key (select)
> > +int
> > +qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len)
> > +{
> > qemu_cfg_select(key);
> > qemu_cfg_skip(offset);
> > qemu_cfg_write(src, len);
> > return len;
> >
> > +}
> >
> > int
> > qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
> > {
> > @@ -339,15 +350,12 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
> >         warn_internalerror();
> >         return -1;
> >     }
> > struct qemu_romfile_s *qfile;
> > qfile = container_of(file, struct qemu_romfile_s, file);
> > u16 key = qemu_get_romfile_key(file);
> > if (offset == 0) {
> >    /* Do it in one transfer */
> >    qemu_cfg_write_entry(src, qfile->select, len);
> >    qemu_cfg_write_entry(src, key, len);
> > } else {
> >    qemu_cfg_select(qfile->select);
> >    qemu_cfg_skip(offset);
> >    qemu_cfg_write(src, len);
> >    qemu_cfg_write_file_simple(src, key, offset, len);
> > }
> >   return len;
> >
> > }
>
> One of the ideas that I mentioned here was to move not just the second
> branch of the "if" to qemu_cfg_write_file_simple(), but the entire "if"
> -- both branches. Because, qemu_cfg_write_entry() looks suitable for S3
> too, and if that kind of optimization makes sense for normal boot, then
> it makes sense for S3 resume as well.
>
> Anyway, this is not a functional problem, I won't obsess about it.
>
> Reviewed-by: Laszlo Ersek <[e-mail] >

Sorry, I misinterpreted your idea.  It makes a lot of sense, so we may as well do it now.  New patch coming.
> Thanks
> Laszlo

—Ben
<snip>