diff mbox

[PATCHv8,2/3] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

Message ID 1499803333-9052-3-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Cave-Ayland July 11, 2017, 8:02 p.m. UTC
When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
able to wire it up differently, it is much more convenient for the caller to
instantiate the device and have the fw_cfg default files already preloaded
during realize.

Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
fw_cfg_io_realize() functions so it no longer needs to be called manually
when instantiating the device, and also rename it to fw_cfg_common_realize()
which better describes its new purpose.

Since it is now the responsibility of the machine to wire up the fw_cfg device
it is necessary to introduce a object_property_add_child() call into
fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
machine object as before.

Finally with the previous change to fw_cfg_find() we can now remove the
assert() preventing multiple fw_cfg devices being instantiated and replace
them with a simple call to fw_cfg_find() at realize time instead. This allows
us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/nvram/fw_cfg.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Comments

Eduardo Habkost July 11, 2017, 9:12 p.m. UTC | #1
On Tue, Jul 11, 2017 at 09:02:12PM +0100, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
> 
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device, and also rename it to fw_cfg_common_realize()
> which better describes its new purpose.
> 
> Since it is now the responsibility of the machine to wire up the fw_cfg device
> it is necessary to introduce a object_property_add_child() call into
> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
> machine object as before.
> 
> Finally with the previous change to fw_cfg_find() we can now remove the
> assert() preventing multiple fw_cfg devices being instantiated and replace
> them with a simple call to fw_cfg_find() at realize time instead. This allows
> us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
[...]
> @@ -1096,6 +1097,12 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
>      }
> +
> +    fw_cfg_common_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }

"if (local_err)" is redundant, as error_propagate() does nothing
if local_err is NULL.  "return" is also unnecessary here.  You
also don't need local_err/error_propagate() if you are not
looking at the value of local_err at all.  This means those 5
lines can be simply written as:

    fw_cfg_common_realize(dev, errp);

(Sorry for not noticing that in v7)

>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1162,6 +1169,12 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_common_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Same as above.


>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> -- 
> 1.7.10.4
>
diff mbox

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8ef889a..6e96b92 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -37,9 +37,6 @@ 
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
-#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"
@@ -909,17 +906,16 @@  static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 
 
 
-static void fw_cfg_init1(DeviceState *dev)
+static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
 
-    assert(!object_resolve_path(FW_CFG_PATH, NULL));
-
-    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
-
-    qdev_init_nofail(dev);
+    if (!fw_cfg_find()) {
+        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
+        return;
+    }
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
@@ -952,7 +948,9 @@  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     ios = FW_CFG_IO(dev);
@@ -990,7 +988,9 @@  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
@@ -1025,6 +1025,7 @@  FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1096,6 +1097,12 @@  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
+
+    fw_cfg_common_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1162,6 +1169,12 @@  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_common_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)