Re: [Qemu-devel] [PATCH 2/4] Fix bad error handling after memory_region_init_ram()

[Markus Armbruster <armbru@redhat.com>]

Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:

> On Fri, Sep 11, 2015 at 7:51 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Symptom:
>>
>>     $ qemu-system-x86_64 -m 10000000
>>     Unexpected error in ram_block_add() at /work/armbru/qemu/exec.c:1456:
>>     upstream-qemu: cannot set up guest memory 'pc.ram': Cannot allocate memory
>>     Aborted (core dumped)
>>
>> Root cause: commit ef701d7 screwed up handling of out-of-memory
>> conditions.  Before the commit, we report the error and exit(1), in
>> one place, ram_block_add().  The commit lifts the error handling up
>> the call chain some, to three places.  Fine.  Except it uses
>> &error_abort in these places, changing the behavior from exit(1) to
>> abort(), and thus undoing the work of commit 3922825 "exec: Don't
>> abort when we can't allocate guest memory".
>>
>> The three places are:
>>
>> * memory_region_init_ram()
>>
>>   Commit 4994653 (right after commit ef701d7) lifted the error
>>   handling further, through memory_region_init_ram(), multiplying the
>>   incorrect use of &error_abort.  Later on, imitation of existing
>>   (bad) code may have created more.
>>
>> * memory_region_init_ram_ptr()
>>
>>   The &error_abort is still there.
>>
>> * memory_region_init_rom_device()
>>
>>   Doesn't need fixing, because commit 33e0eb5 (soon after commit
>>   ef701d7) lifted the error handling further, and in the process
>>   changed it from &error_abort to passing it up the call chain.
>>   Correct, because the callers are realize() methods.
>>
>> Fix the error handling after memory_region_init_ram() with a
>> Coccinelle semantic patch:
>>
>>     @r@
>>     expression mr, owner, name, size, err;
>>     position p;
>>     @@
>>             memory_region_init_ram(mr, owner, name, size,
>>     (
>>     -                              &error_abort
>>     +                              &error_fatal
>>     |
>>                                    err@p
>>     )
>>                                   );
>>     @script:python@
>>         p << r.p;
>>     @@
>>     print "%s:%s:%s" % (p[0].file, p[0].line, p[0].column)
>>
>> When the last argument is &error_abort, it gets replaced by
>> &error_fatal.  This is the fix.
>>
>> If the last argument is anything else, its position is reported.  This
>> lets us check the fix is complete.  Four positions get reported:
>>
>> * ram_backend_memory_alloc()
>>
>>   Error is passed up the call chain, ultimately through
>>   user_creatable_complete().  As far as I can tell, it's callers all
>>   handle the error sanely.
>>
>> * fsl_imx25_realize(), fsl_imx31_realize(), dp8393x_realize()
>>
>
> This is super modern code that is the exception to the rule doing it right.

Progress :)

>>   DeviceClass.realize() methods, errors handled sanely further up the
>>   call chain.
>>
>> We're good.  Test case again behaves:
>>
>>     $ qemu-system-x86_64 -m 10000000
>>     qemu-system-x86_64: cannot set up guest memory 'pc.ram': Cannot allocate memory
>>     [Exit 1 ]
>>
>> The next commits will repair the rest of commit ef701d7's damage.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
> So the changes here fall into a few different categories.
>
> * Machine init code - error_fatal() is definately right (for the
> moment, unless we want to support complete machine hotplug or
> something crazy like that).
> * SysBusDevice::init functions - These should be propagatable, but we
> are really getting what we deserve with error_fatal(). They should be
> desysbusified then can be converted to realize. Out of scope of this
> series though.
> * Device Realize functions (incl a few SoCs). In these cases we should
> propagate for the sake of hotplug failure (or other reasons). I have
> flagged the easy ones below.
> * Common helper functions that are missing Error ** even though their
> callers have them. We should added them (particular in VGA).
>
> I think we should try and get the realize and helper ones right and do
> the machine init and SBD::init ones later.

Makes sense.  In fact, I got the obvious realize ones sketched in my
tree, but then forgot to mention it in my cover letter.  Let me compare
your notes to my sketch.

[...]
>> --- a/hw/arm/stm32f205_soc.c
>> +++ b/hw/arm/stm32f205_soc.c
>> @@ -71,7 +71,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>>      MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>>
>>      memory_region_init_ram(flash, NULL, "STM32F205.flash", FLASH_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>
>
> This should propagate

Yes.

>>      memory_region_init_alias(flash_alias, NULL, "STM32F205.flash.alias",
>>                               flash, 0, FLASH_SIZE);
>>
>> @@ -84,7 +84,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>>      memory_region_add_subregion(system_memory, 0, flash_alias);
>>
>>      memory_region_init_ram(sram, NULL, "STM32F205.sram", SRAM_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      vmstate_register_ram_global(sram);
>>      memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, sram);
>>

This, too.

[...]
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index a4e7b5c..37dc0b0 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -167,7 +167,7 @@ static void zynq_init(MachineState *machine)
>>
>>      /* 256K of on-chip memory */
>>      memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 << 10,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      vmstate_register_ram_global(ocm_ram);
>>      memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 2955f3b..43b3e5a 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -113,7 +113,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>          char *ocm_name = g_strdup_printf("zynqmp.ocm_ram_bank_%d", i);
>>
>>          memory_region_init_ram(&s->ocm_ram[i], NULL, ocm_name,
>> -                               XLNX_ZYNQMP_OCM_RAM_SIZE, &error_abort);
>> +                               XLNX_ZYNQMP_OCM_RAM_SIZE, &error_fatal);
>
> This should propagate.

Yes.

>>          vmstate_register_ram_global(&s->ocm_ram[i]);
>>          memory_region_add_subregion(get_system_memory(),
>>                                      XLNX_ZYNQMP_OCM_RAM_0_ADDRESS +
>> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
>> index 1b2c893..58eff50 100644
>> --- a/hw/block/onenand.c
>> +++ b/hw/block/onenand.c
>> @@ -786,7 +786,7 @@ static int onenand_initfn(SysBusDevice *sbd)
>>      s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT),
>>                      0xff, (64 + 2) << PAGE_SHIFT);
>>      memory_region_init_ram(&s->ram, OBJECT(s), "onenand.ram",
>> -                           0xc000 << s->shift, &error_abort);
>> +                           0xc000 << s->shift, &error_fatal);
>>      vmstate_register_ram_global(&s->ram);
>>      ram = memory_region_get_ram_ptr(&s->ram);
>>      s->boot[0] = ram + (0x0000 << s->shift);
>> diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
>> index 3cae480..b57051e 100644
>> --- a/hw/cris/axis_dev88.c
>> +++ b/hw/cris/axis_dev88.c
>> @@ -277,7 +277,7 @@ void axisdev88_init(MachineState *machine)
>>      /* The ETRAX-FS has 128Kb on chip ram, the docs refer to it as the
>>         internal memory.  */
>>      memory_region_init_ram(phys_intmem, NULL, "axisdev88.chipram", INTMEM_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      vmstate_register_ram_global(phys_intmem);
>>      memory_region_add_subregion(address_space_mem, 0x38000000, phys_intmem);
>>
>> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
>> index 34dcbc3..d2a0d97 100644
>> --- a/hw/display/cg3.c
>> +++ b/hw/display/cg3.c
>> @@ -281,7 +281,7 @@ static void cg3_initfn(Object *obj)
>>      CG3State *s = CG3(obj);
>>
>>      memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      memory_region_set_readonly(&s->rom, true);
>>      sysbus_init_mmio(sbd, &s->rom);
>>

Do this in cg3_realizefn() instead, so we can propagate?

>> @@ -310,7 +310,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>>      }
>>
>>      memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>>      vmstate_register_ram_global(&s->vram_mem);
>>      sysbus_init_mmio(sbd, &s->vram_mem);

This should propagate.

>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>> index 2288238..9c961da 100644
>> --- a/hw/display/qxl.c
>> +++ b/hw/display/qxl.c
>> @@ -1970,14 +1970,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
>>
>>      qxl->rom_size = qxl_rom_size();
>>      memory_region_init_ram(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
>> -                           qxl->rom_size, &error_abort);
>> +                           qxl->rom_size, &error_fatal);
>
> Propagate.

Yes.

>>      vmstate_register_ram(&qxl->rom_bar, &qxl->pci.qdev);
>>      init_qxl_rom(qxl);
>>      init_qxl_ram(qxl);
>>
>>      qxl->guest_surfaces.cmds = g_new0(QXLPHYSICAL, qxl->ssd.num_surfaces);
>>      memory_region_init_ram(&qxl->vram_bar, OBJECT(qxl), "qxl.vram",
>> -                           qxl->vram_size, &error_abort);
>> +                           qxl->vram_size, &error_fatal);
>>      vmstate_register_ram(&qxl->vram_bar, &qxl->pci.qdev);
>>      memory_region_init_alias(&qxl->vram32_bar, OBJECT(qxl), "qxl.vram32",
>>                               &qxl->vram_bar, 0, qxl->vram32_size);

This, too.

>> @@ -2079,7 +2079,7 @@ static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
>>      qxl->id = device_id++;
>>      qxl_init_ramsize(qxl);
>>      memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
>> -                           qxl->vga.vram_size, &error_abort);
>> +                           qxl->vga.vram_size, &error_fatal);
>
> Propagate.

Yes.

>>      vmstate_register_ram(&qxl->vga.vram, &qxl->pci.qdev);
>>      qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
>>      qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
[...]
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -945,7 +945,7 @@ static void tcx_initfn(Object *obj)
>>      TCXState *s = TCX(obj);
>>
>>      memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>
> I guess this one is particularly difficult, and indicates the RAM init
> needs to move to realize.

Similar to cg3_initfn().

>>      memory_region_set_readonly(&s->rom, true);
>>      sysbus_init_mmio(sbd, &s->rom);
>>
>> @@ -1007,7 +1007,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
>>      char *fcode_filename;
>>
>>      memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
>> -                           s->vram_size * (1 + 4 + 4), &error_abort);
>> +                           s->vram_size * (1 + 4 + 4), &error_fatal);
>
> Propagate.

Yes.

>>      vmstate_register_ram_global(&s->vram_mem);
>>      memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
>>      vram_base = memory_region_get_ram_ptr(&s->vram_mem);
>> diff --git a/hw/display/vga.c b/hw/display/vga.c
>> index b35d523..9f68394 100644
>> --- a/hw/display/vga.c
>> +++ b/hw/display/vga.c
>> @@ -2139,7 +2139,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
>>
>
> Can this function accept error ** ? Most of the callers are realize fns.

Then it should take Error **errp.

>>      s->is_vbe_vmstate = 1;
>>      memory_region_init_ram(&s->vram, obj, "vga.vram", s->vram_size,
>> -                           &error_abort);
>> +                           &error_fatal);
>
> Then this becomes a propagation.

Yes.

>>      vmstate_register_ram(&s->vram, global_vmstate ? NULL : DEVICE(obj));
>>      xen_register_framebuffer(&s->vram);
>>      s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>> index 7f397d3..8e93509 100644
>> --- a/hw/display/vmware_vga.c
>> +++ b/hw/display/vmware_vga.c
>> @@ -1244,7 +1244,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
>>
>>      s->fifo_size = SVGA_FIFO_SIZE;
>>      memory_region_init_ram(&s->fifo_ram, NULL, "vmsvga.fifo", s->fifo_size,
>> -                           &error_abort);
>> +                           &error_fatal);
>
> Can add errp to this function from caller and propagate.

Yes.

[...]
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index c63f45d..c93426b 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -302,7 +302,7 @@ static void raven_realize(PCIDevice *d, Error **errp)
>>      d->config[0x34] = 0x00; // capabilities_pointer
>>
>>      memory_region_init_ram(&s->bios, OBJECT(s), "bios", BIOS_SIZE,
>> -                           &error_abort);
>> +                           &error_fatal);
>>      memory_region_set_readonly(&s->bios, true);
>>      memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
>>                                  &s->bios);

Propagate.

>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index ccea628..b0bf540 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2081,7 +2081,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
>>          snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev)));
>>      }
>>      pdev->has_rom = true;
>> -    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size, &error_abort);
>> +    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size, &error_fatal);
>
> Propagate.

Yes.

> Regards,
> Peter
>
>>      vmstate_register_ram(&pdev->rom, &pdev->qdev);
>>      ptr = memory_region_get_ram_ptr(&pdev->rom);
>>      load_image(path, ptr);

You got a few more than me, I got a few more than you; lovely :)

Three kinds of changes to be done, I think:

A. Fix use of &error_fatal in functions that already have an Error **
   parameter

   Propagate the error.  Trivial.  Could probably go through my tree as
   a single patch.  Should start with my sketch (appended) to save some
   typing.

B. Add Error ** parameter to functions called from functions that have an
   Error ** parameter

   Should be straightforward.  May want to route it through the
   relevant maintainer, though.

C. Move calls that can fail from .instance_init() to .realize()

   Route through the relevant maintainer.

If you'd like to pitch in, let me know, so we don't duplicate work.


Here's my (incomplete!) sketch:

diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index 4d26a7e..e3bec1d 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -71,7 +71,11 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
     MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
 
     memory_region_init_ram(flash, NULL, "STM32F205.flash", FLASH_SIZE,
-                           &error_fatal);
+                           &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     memory_region_init_alias(flash_alias, NULL, "STM32F205.flash.alias",
                              flash, 0, FLASH_SIZE);
 
@@ -83,8 +87,11 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion(system_memory, FLASH_BASE_ADDRESS, flash);
     memory_region_add_subregion(system_memory, 0, flash_alias);
 
-    memory_region_init_ram(sram, NULL, "STM32F205.sram", SRAM_SIZE,
-                           &error_fatal);
+    memory_region_init_ram(sram, NULL, "STM32F205.sram", SRAM_SIZE, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram_global(sram);
     memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, sram);
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 43b3e5a..2d57f27 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -113,7 +113,11 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         char *ocm_name = g_strdup_printf("zynqmp.ocm_ram_bank_%d", i);
 
         memory_region_init_ram(&s->ocm_ram[i], NULL, ocm_name,
-                               XLNX_ZYNQMP_OCM_RAM_SIZE, &error_fatal);
+                               XLNX_ZYNQMP_OCM_RAM_SIZE, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         vmstate_register_ram_global(&s->ocm_ram[i]);
         memory_region_add_subregion(get_system_memory(),
                                     XLNX_ZYNQMP_OCM_RAM_0_ADDRESS +
diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index d2a0d97..adf847f 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -294,6 +294,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     CG3State *s = CG3(dev);
+    Error *err = NULL;
     int ret;
     char *fcode_filename;
 
@@ -310,7 +311,11 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
     }
 
     memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size,
-                           &error_fatal);
+                           &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
     vmstate_register_ram_global(&s->vram_mem);
     sysbus_init_mmio(sbd, &s->vram_mem);
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 9c961da..d8a381f 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1931,6 +1931,7 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
 static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 {
     uint8_t* config = qxl->pci.config;
+    Error *err = NULL;
     uint32_t pci_device_rev;
     uint32_t io_size;
 
@@ -1970,14 +1971,22 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     qxl->rom_size = qxl_rom_size();
     memory_region_init_ram(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
-                           qxl->rom_size, &error_fatal);
+                           qxl->rom_size, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram(&qxl->rom_bar, &qxl->pci.qdev);
     init_qxl_rom(qxl);
     init_qxl_ram(qxl);
 
     qxl->guest_surfaces.cmds = g_new0(QXLPHYSICAL, qxl->ssd.num_surfaces);
     memory_region_init_ram(&qxl->vram_bar, OBJECT(qxl), "qxl.vram",
-                           qxl->vram_size, &error_fatal);
+                           qxl->vram_size, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram(&qxl->vram_bar, &qxl->pci.qdev);
     memory_region_init_alias(&qxl->vram32_bar, OBJECT(qxl), "qxl.vram32",
                              &qxl->vram_bar, 0, qxl->vram32_size);
@@ -2075,11 +2084,16 @@ static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
 {
     static int device_id = 1;
     PCIQXLDevice *qxl = PCI_QXL(dev);
+    Error *err = NULL;
 
     qxl->id = device_id++;
     qxl_init_ramsize(qxl);
     memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
-                           qxl->vga.vram_size, &error_fatal);
+                           qxl->vga.vram_size, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram(&qxl->vga.vram, &qxl->pci.qdev);
     qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
     qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 4635800..623683f 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -1002,12 +1002,17 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     TCXState *s = TCX(dev);
     ram_addr_t vram_offset = 0;
+    Error *err = NULL;
     int size, ret;
     uint8_t *vram_base;
     char *fcode_filename;
 
     memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
-                           s->vram_size * (1 + 4 + 4), &error_fatal);
+                           s->vram_size * (1 + 4 + 4), &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram_global(&s->vram_mem);
     memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
     vram_base = memory_region_get_ram_ptr(&s->vram_mem);
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index c93426b..9af1323 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -294,6 +294,7 @@ static void raven_pcihost_initfn(Object *obj)
 static void raven_realize(PCIDevice *d, Error **errp)
 {
     RavenPCIState *s = RAVEN_PCI_DEVICE(d);
+    Error *err = NULL;
     char *filename;
     int bios_size = -1;
 
@@ -301,8 +302,11 @@ static void raven_realize(PCIDevice *d, Error **errp)
     d->config[0x0D] = 0x10; // latency_timer
     d->config[0x34] = 0x00; // capabilities_pointer
 
-    memory_region_init_ram(&s->bios, OBJECT(s), "bios", BIOS_SIZE,
-                           &error_fatal);
+    memory_region_init_ram(&s->bios, OBJECT(s), "bios", BIOS_SIZE, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     memory_region_set_readonly(&s->bios, true);
     memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
                                 &s->bios);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0bf540..88acbeb 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2020,6 +2020,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size)
 static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
                                Error **errp)
 {
+    Error *err = NULL;
     int size;
     char *path;
     void *ptr;
@@ -2081,7 +2082,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
         snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev)));
     }
     pdev->has_rom = true;
-    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size, &error_fatal);
+    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     vmstate_register_ram(&pdev->rom, &pdev->qdev);
     ptr = memory_region_get_ram_ptr(&pdev->rom);
     load_image(path, ptr);
-- 
2.4.3