[Qemu-devel] [PATCH 0/2] fw_cfg: *prequel* to cleanup series

[Qemu-devel] [PATCH 0/2] fw_cfg: *prequel* to cleanup series

[Gabriel L. Somlo]

The fw_cfg cleanup series exposed a memory leak occurring on ppc and sparc
machine types.

This series adds a safe fw_cfg integer-type update function (1/2), and
modifies ppc and sparc callbacks to use it instead of the current leaky
method (2/2).

This should preferably go in *before* "fw_cfg: remove support for guest-side data writes", which would otherwise cause certain ppc and sparc command
lines to crash.

Once these are reviewed (also, see additional questions in 1/1), should I
re-spin the whole set, or keep these two as a separate, independent series?

Thanks,
  Gabriel

Gabriel L. Somlo (2):
  fw_cfg: add fw_cfg_modify_i16 (update) method
  fw_cfg: fix FW_CFG_BOOT_DEVICE value update on ppc and sparc[64]

 hw/nvram/fw_cfg.c         | 10 ++++++++++
 hw/ppc/mac_newworld.c     |  2 +-
 hw/ppc/mac_oldworld.c     |  2 +-
 hw/sparc/sun4m.c          |  2 +-
 hw/sparc64/sun4u.c        |  2 +-
 include/hw/nvram/fw_cfg.h |  1 +
 6 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] fw_cfg: add fw_cfg_modify_i16 (update) method

[Gabriel L. Somlo]

Allow the ability to modify the value of an existing 16-bit integer
fw_cfg item.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

Couple of thoughts:

   1. I'm thinking about pre-emptively creating _i16, _i32, and _i64
      versions, but right now (for fixing sparc and ppc) we only need
      the _i16 version. What to do ?

   2. Part of me wants to find the memory location containing the previous
      value and simply overwrite it, but I'll need to somehow ensure the
      blob being replaced was of the same size, etc., which could get hairy.
      So for now I'm going with the paranoid/safe version which allocates
      a new blob and frees the old one.

Thanks,
  Gabriel

 hw/nvram/fw_cfg.c         | 10 ++++++++++
 include/hw/nvram/fw_cfg.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 68eff77..08b5cc3 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -484,6 +484,16 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
+void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value)
+{
+    uint16_t *copy, *old;
+
+    copy = g_malloc(sizeof(value));
+    *copy = cpu_to_le16(value);
+    old = fw_cfg_modify_bytes_read(s, key, copy, sizeof(value));
+    g_free(old);
+}
+
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
 {
     uint32_t *copy;
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 6d8a8ac..bc6c4a0 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -67,6 +67,7 @@ typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
+void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
 void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] fw_cfg: add fw_cfg_modify_i16 (update) method

[Gabriel L. Somlo]

On Mon, Jun 08, 2015 at 02:10:44PM -0400, Gabriel L. Somlo wrote:
> Allow the ability to modify the value of an existing 16-bit integer
> fw_cfg item.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> Couple of thoughts:
> 
>    1. I'm thinking about pre-emptively creating _i16, _i32, and _i64
>       versions, but right now (for fixing sparc and ppc) we only need
>       the _i16 version. What to do ?

Presumably, I'll update the documentation to mention these update
functions as well (one more reason to do all three versions rather
than just _i16) :)

> 
>    2. Part of me wants to find the memory location containing the previous
>       value and simply overwrite it, but I'll need to somehow ensure the
>       blob being replaced was of the same size, etc., which could get hairy.
>       So for now I'm going with the paranoid/safe version which allocates
>       a new blob and frees the old one.
> 
> Thanks,
>   Gabriel
> 
>  hw/nvram/fw_cfg.c         | 10 ++++++++++
>  include/hw/nvram/fw_cfg.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 68eff77..08b5cc3 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -484,6 +484,16 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> +void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value)
> +{
> +    uint16_t *copy, *old;
> +
> +    copy = g_malloc(sizeof(value));
> +    *copy = cpu_to_le16(value);
> +    old = fw_cfg_modify_bytes_read(s, key, copy, sizeof(value));
> +    g_free(old);
> +}
> +
>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>  {
>      uint32_t *copy;
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 6d8a8ac..bc6c4a0 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -67,6 +67,7 @@ typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
>  void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
>  void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
> +void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value);
>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
>  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>  void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH 1/2] fw_cfg: add fw_cfg_modify_i16 (update) method

[Gerd Hoffmann]

On Mo, 2015-06-08 at 14:26 -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 08, 2015 at 02:10:44PM -0400, Gabriel L. Somlo wrote:
> > Allow the ability to modify the value of an existing 16-bit integer
> > fw_cfg item.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> > 
> > Couple of thoughts:
> > 
> >    1. I'm thinking about pre-emptively creating _i16, _i32, and _i64
> >       versions, but right now (for fixing sparc and ppc) we only need
> >       the _i16 version. What to do ?
> 
> Presumably, I'll update the documentation to mention these update
> functions as well (one more reason to do all three versions rather
> than just _i16) :)

Should be easy to add this once they are actually needed.
Documentation update is fine, can go incremental though.

cheers,
  Gerd

[Qemu-devel] [PATCH 2/2] fw_cfg: fix FW_CFG_BOOT_DEVICE update on ppc and sparc

[Gabriel L. Somlo]

On ppc, sparc, and sparc64, the value of the FW_CFG_BOOT_DEVICE 16bit
fw_cfg entry is repeatedly modified from a series of callbacks, which
currently results in the previous value's dynamically allocated memory
being leaked.

This patch switches updating to the new fw_cfg_modify_i16() call, which
does not cause memory leaks.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/ppc/mac_newworld.c | 2 +-
 hw/ppc/mac_oldworld.c | 2 +-
 hw/sparc/sun4m.c      | 2 +-
 hw/sparc64/sun4u.c    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index a365bf9..0f3e341 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -119,7 +119,7 @@ static const MemoryRegionOps unin_ops = {
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
-    fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
+    fw_cfg_modify_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
 }
 
 static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index f26133d..99879dd 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -52,7 +52,7 @@
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
-    fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
+    fw_cfg_modify_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
 }
 
 static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 8a3599c..68ac4d8 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -124,7 +124,7 @@ void DMA_register_channel (int nchan,
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
-    fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
+    fw_cfg_modify_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
 }
 
 static void nvram_init(Nvram *nvram, uint8_t *macaddr,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 6f34e87..30cfa0e 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -127,7 +127,7 @@ void DMA_register_channel (int nchan,
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
-    fw_cfg_add_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
+    fw_cfg_modify_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
 }
 
 static int sun4u_NVRAM_set_params(Nvram *nvram, uint16_t NVRAM_size,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] fw_cfg: *prequel* to cleanup series

[Gerd Hoffmann]

On Mo, 2015-06-08 at 14:10 -0400, Gabriel L. Somlo wrote:
> The fw_cfg cleanup series exposed a memory leak occurring on ppc and sparc
> machine types.
> 
> This series adds a safe fw_cfg integer-type update function (1/2), and
> modifies ppc and sparc callbacks to use it instead of the current leaky
> method (2/2).
> 
> This should preferably go in *before* "fw_cfg: remove support for guest-side data writes", which would otherwise cause certain ppc and sparc command
> lines to crash.
> 
> Once these are reviewed (also, see additional questions in 1/1), should I
> re-spin the whole set, or keep these two as a separate, independent series?

Keeping separate is fine, I'll order the patches via 'git rebase'.

cheers,
  Gerd