[v5] QEMU: Add capability to write back fw_cfg address to QEMU

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

Posted by ben@skyportsystems.com 8 years, 10 months ago

From: Ben Warren <ben@skyportsystems.com>

When resuming from S3, only fw_cfg file keys are known.

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 src/fw/paravirt.c | 23 +++++++++++++++++++++++
 src/fw/paravirt.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 4618647..e513dd5 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
     return len;
 }
 
+// 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;
+}
+
 static void
 qemu_romfile_add(char *name, int select, int skip, int size)
 {
@@ -370,6 +381,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;
+    qfile = container_of(file, struct qemu_romfile_s, file);
+    if (!qfile) {
+        warn_internalerror();
+        return 0;
+    }
+    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
-- 
2.7.4


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

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

Posted by Laszlo Ersek 8 years, 9 months ago

On 02/18/17 07:21, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> When resuming from S3, only fw_cfg file keys are known.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  src/fw/paravirt.c | 23 +++++++++++++++++++++++
>  src/fw/paravirt.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 4618647..e513dd5 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
>      return len;
>  }
>  
> +// 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;
> +}

Does anything counter-indicate the customization seen in
qemu_cfg_write_file() as well, that is, call qemu_cfg_write_entry() if
offset is zero?

If not, then I think you could even split out that part of
qemu_cfg_write_file() as qemu_cfg_write_file_simple(), and call it from
qemu_cfg_write_file().

> +
>  static void
>  qemu_romfile_add(char *name, int select, int skip, int size)
>  {
> @@ -370,6 +381,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;
> +    qfile = container_of(file, struct qemu_romfile_s, file);

If the input pointer "file" was valid, then container_of() cannot
produce a NULL pointer. So I suggest to drop the code that depends on that.

> +    if (!qfile) {
> +        warn_internalerror();
> +        return 0;
> +    }
> +    return qfile->select;
> +}

This could be reused in qemu_cfg_write_file() too, so that that function
would remain:
- initial checks
- qemu_cfg_write_file_simple( ..., qemu_get_romfile_key(), ...)

I think patch #1 is fine as is, so I'd keep this refactoring in patch #4.

Thanks
Laszlo

> +
> +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
> 


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

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

Posted by Igor Mammedov 8 years, 9 months ago

On Fri, 17 Feb 2017 22:21:58 -0800
ben@skyportsystems.com wrote:

> From: Ben Warren <ben@skyportsystems.com>
> 
> When resuming from S3, only fw_cfg file keys are known.
maybe something like this:

When resuming from S3 due to memory constrains fw_cfg files API
isn't available, so add simple API to get file 'key' and
write to file by using saved at boot time 'key'.

+ with Laszlo's suggestions on code consolidation

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  src/fw/paravirt.c | 23 +++++++++++++++++++++++
>  src/fw/paravirt.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 4618647..e513dd5 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
>      return len;
>  }
>  
> +// 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;
> +}
> +
>  static void
>  qemu_romfile_add(char *name, int select, int skip, int size)
>  {
> @@ -370,6 +381,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;
> +    qfile = container_of(file, struct qemu_romfile_s, file);
> +    if (!qfile) {
> +        warn_internalerror();
> +        return 0;
> +    }
> +    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


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

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

Posted by Kevin O'Connor 8 years, 9 months ago

On Fri, Feb 17, 2017 at 10:21:58PM -0800, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> When resuming from S3, only fw_cfg file keys are known.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  src/fw/paravirt.c | 23 +++++++++++++++++++++++
>  src/fw/paravirt.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 4618647..e513dd5 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
>      return len;
>  }
>  
> +// 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;
> +}
> +
>  static void
>  qemu_romfile_add(char *name, int select, int skip, int size)
>  {
> @@ -370,6 +381,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;
> +    qfile = container_of(file, struct qemu_romfile_s, file);
> +    if (!qfile) {
> +        warn_internalerror();
> +        return 0;
> +    }

This should verify that file->copy==qemu_cfg_read_file before calling
container_of.  As Laszlo points out, there's no point in checking
!qfile after container_of.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios