| Message ID | 20190308013222.12524-9-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy | expand |
On 03/08/19 02:32, Philippe Mathieu-Daudé wrote: > Each implementation (I/O and MEM) calls fw_cfg_file_slots_allocate() > then fw_cfg_common_realize(). > Simplify by moving the fw_cfg_file_slots_allocate() call into > fw_cfg_common_realize() where it belongs. The patch does more than that, namely: > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/nvram/fw_cfg.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 0fb020edce..ca58d279a4 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -929,19 +929,26 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) > qemu_register_reset(fw_cfg_machine_reset, s); > } > > - > +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp); > > 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; > + Error *local_err = NULL; > > if (!fw_cfg_find()) { > error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); > return; > } > > + fw_cfg_file_slots_allocate(s, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); > fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); > fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics); > @@ -1108,7 +1115,7 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp) > FWCfgIoState *s = FW_CFG_IO(dev); > Error *local_err = NULL; > > - fw_cfg_file_slots_allocate(FW_CFG(s), &local_err); > + fw_cfg_common_realize(dev, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1125,8 +1132,6 @@ 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, errp); > } it moves the fw_cfg_common_realize() call from the ends of the IO/MEM realize functions to their middles; in particular to the point before memory regions and (when appropriate) MMIO resources are initialized. In other words, the patch reorders all of the current actions in fw_cfg_common_realize() against said MemoryRegion & SysBusDevice resource actions. Why is this safe? (I'm not claiming it is unsafe, but I'd like to see it explained in the commit message.) Note: the fw_cfg_common_realize() you are moving around was introduced in commit 38f3adc34de8 ("fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers", 2017-07-17), by Mark Cave-Ayland. I see Mark is CC'd already, so I hope he can comment. Thanks, Laszlo > > static void fw_cfg_io_class_init(ObjectClass *klass, void *data) > @@ -1162,7 +1167,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp) > const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops; > Error *local_err = NULL; > > - fw_cfg_file_slots_allocate(FW_CFG(s), &local_err); > + fw_cfg_common_realize(dev, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1189,8 +1194,6 @@ 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, errp); > } > > static void fw_cfg_mem_class_init(ObjectClass *klass, void *data) >
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 0fb020edce..ca58d279a4 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -929,19 +929,26 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) qemu_register_reset(fw_cfg_machine_reset, s); } - +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp); 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; + Error *local_err = NULL; if (!fw_cfg_find()) { error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); return; } + fw_cfg_file_slots_allocate(s, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics); @@ -1108,7 +1115,7 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp) FWCfgIoState *s = FW_CFG_IO(dev); Error *local_err = NULL; - fw_cfg_file_slots_allocate(FW_CFG(s), &local_err); + fw_cfg_common_realize(dev, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -1125,8 +1132,6 @@ 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, errp); } static void fw_cfg_io_class_init(ObjectClass *klass, void *data) @@ -1162,7 +1167,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp) const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops; Error *local_err = NULL; - fw_cfg_file_slots_allocate(FW_CFG(s), &local_err); + fw_cfg_common_realize(dev, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -1189,8 +1194,6 @@ 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, errp); } static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
Each implementation (I/O and MEM) calls fw_cfg_file_slots_allocate() then fw_cfg_common_realize(). Simplify by moving the fw_cfg_file_slots_allocate() call into fw_cfg_common_realize() where it belongs. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/nvram/fw_cfg.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)