Re: [Qemu-devel] [PATCH v2 5/5] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

On Fri, Mar 20, 2015 at 02:01:25PM -0400, Gabriel L. Somlo wrote:
> On Thu, Mar 19, 2015 at 06:38:53PM +0100, Laszlo Ersek wrote:
> > Hmmmm... that's messy, again. fw_cfg is built into the qemu binary only
> > if you have CONFIG_SOFTMMU. I guess something like this should work:
> > 
> > #ifdef CONFIG_SOFTMMU
> >     /* okay, fw_cfg_find() is linked into the binary */
> > 
> >     if (fw_cfg_find()) {
> >         /* okay, the board has set up fw_cfg in its MachineClass init
> >          * function
> >          */
> > 
> >         if (qemu_opts_foreach(qemu_find_opts("fw_cfg"), parse_fw_cfg,
> >                               NULL, 1)) {
> >             exit(1);
> >         }
> >     }
> > #endif
> > 
> > You should definitely wait & see what Markus & Gerd think about the above.
> 
> I didn't use #ifdef around fw_cfg_find() -- I think the underlying
> object_resolve_path() will return NULL whether fw_cfg support isn't
> compiled in or if the fw_cfg device hasn't been initialized, so
> that should not be necessary [*].
> 
> [*] Oh, wait. Right now, fw_cfg_find() is a function defined inside
> fw_cfg.c, so if that won't get compiled, we lose... How about turning
> it into a macro or inline function in fw_cfg.h instead ? Something like
> 
>     static inline FWCfgState *fw_cfg_find(void)
>     {
>         return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>     }
> 
> should work nicely -- any thoughts ?

That patch, by the way, would look something like below. At this
point, "#ifdef CONFIG_SOFTMMU" definitely looks cleaner, although
now we'd have the (fw_cfg <-> softmmu) relationship codified *both*
here, and in hw/Makefile.objs ("devices-dirs-$(CONFIG_SOFTMMU) += nvram/"),
which is why I'm not 100% thrilled by the idea...

Thanks,
--Gabriel


diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8d4ea25..7a7439a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,14 +31,10 @@
 #include "qemu/config-file.h"
 
 #define FW_CFG_SIZE 2
-#define FW_CFG_NAME "fw_cfg"
-#define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
-#define TYPE_FW_CFG     "fw_cfg"
 #define TYPE_FW_CFG_IO  "fw_cfg_io"
 #define TYPE_FW_CFG_MEM "fw_cfg_mem"
 
-#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
@@ -633,11 +629,6 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr 
data_addr)
 }
 
 
-FWCfgState *fw_cfg_find(void)
-{
-    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
-}
-
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b2e10c2..10c7771 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -7,6 +7,7 @@
 
 #include "exec/hwaddr.h"
 #include "qemu/typedefs.h"
+#include "qom/object.h"
 #endif
 
 #define FW_CFG_SIGNATURE        0x00
@@ -81,7 +82,16 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr 
data_addr);
 FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
                                  uint32_t data_width);
 
-FWCfgState *fw_cfg_find(void);
+#define FW_CFG_NAME "fw_cfg"
+#define FW_CFG_PATH "/machine/" FW_CFG_NAME
+#define TYPE_FW_CFG "fw_cfg"
+
+#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
+
+static inline FWCfgState *fw_cfg_find(void)
+{
+    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
+}
 
 #endif /* NO_QEMU_PROTOS */