[v1] macfb: fixes for booting MacOS

[PATCH 01/12] macfb: handle errors that occur during realize

Posted by Mark Cave-Ayland 4 years, 4 months ago

Make sure any errors that occur within the macfb realize chain are detected
and handled correctly to prevent crashes and to ensure that error messages are
reported back to the user.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 76808b69cc..2b747a8de8 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
     MacfbState *ms = &s->macfb;
 
     macfb_common_realize(dev, ms, errp);
+    if (*errp) {
+        return;
+    }
+
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
 }
@@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
     MacfbState *ms = &s->macfb;
 
     ndc->parent_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
 
     macfb_common_realize(dev, ms, errp);
+    if (*errp) {
+        return;
+    }
+
     memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
     memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
 }
-- 
2.20.1

Re: [PATCH 01/12] macfb: handle errors that occur during realize

Posted by BALATON Zoltan 4 years, 4 months ago

On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/display/macfb.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c

There's one more in macfb_common_realize() after:

memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram", MACFB_VRAM_SIZE, errp);

otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>


> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>     MacfbState *ms = &s->macfb;
>
>     macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>     sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
> }
> @@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>     MacfbState *ms = &s->macfb;
>
>     ndc->parent_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
>
>     macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>     memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>     memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
> }
>

Re: [PATCH 01/12] macfb: handle errors that occur during realize

Posted by Mark Cave-Ayland 4 years, 4 months ago

On 02/10/2021 12:36, BALATON Zoltan wrote:

> On Sat, 2 Oct 2021, Mark Cave-Ayland wrote:
>> Make sure any errors that occur within the macfb realize chain are detected
>> and handled correctly to prevent crashes and to ensure that error messages are
>> reported back to the user.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/display/macfb.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 76808b69cc..2b747a8de8 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
> 
> There's one more in macfb_common_realize() after:
> 
> memory_region_init_ram_nomigrate(&s->mem_vram, OBJECT(s), "macfb-vram", 
> MACFB_VRAM_SIZE, errp);
> 
> otherwise
> 
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Ah yes, IIRC from the last patchset the outcome of the discussion with Markus was 
that these functions should use &error_abort. I'll make the same change here for v2.


ATB,

Mark.

Re: [PATCH 01/12] macfb: handle errors that occur during realize

Posted by Philippe Mathieu-Daudé 4 years, 4 months ago

On 10/2/21 12:59, Mark Cave-Ayland wrote:
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }

See a related discussion:
https://lore.kernel.org/qemu-devel/87bl47ll9l.fsf@dusky.pond.sub.org/

Re: [PATCH 01/12] macfb: handle errors that occur during realize

Posted by Mark Cave-Ayland 4 years, 4 months ago

On 02/10/2021 14:47, Philippe Mathieu-Daudé wrote:

> On 10/2/21 12:59, Mark Cave-Ayland wrote:
>> Make sure any errors that occur within the macfb realize chain are detected
>> and handled correctly to prevent crashes and to ensure that error messages are
>> reported back to the user.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 76808b69cc..2b747a8de8 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>>       MacfbState *ms = &s->macfb;
>>   
>>       macfb_common_realize(dev, ms, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
> 
> See a related discussion:
> https://lore.kernel.org/qemu-devel/87bl47ll9l.fsf@dusky.pond.sub.org/

Interesting, thanks for the link. I will update macfb_common_realize() to return a 
boolean for v2.


ATB,

Mark.

Re: [PATCH 01/12] macfb: handle errors that occur during realize

Posted by Laurent Vivier 4 years, 4 months ago

Le 02/10/2021 à 12:59, Mark Cave-Ayland a écrit :
> Make sure any errors that occur within the macfb realize chain are detected
> and handled correctly to prevent crashes and to ensure that error messages are
> reported back to the user.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 76808b69cc..2b747a8de8 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -379,6 +379,10 @@ static void macfb_sysbus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_ctrl);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &ms->mem_vram);
>  }
> @@ -391,8 +395,15 @@ static void macfb_nubus_realize(DeviceState *dev, Error **errp)
>      MacfbState *ms = &s->macfb;
>  
>      ndc->parent_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
>  
>      macfb_common_realize(dev, ms, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      memory_region_add_subregion(&nd->slot_mem, DAFB_BASE, &ms->mem_ctrl);
>      memory_region_add_subregion(&nd->slot_mem, VIDEO_BASE, &ms->mem_vram);
>  }
> 

Perhaps as suggested by Philippe you change macfb_common_realize() to return a value and an error?

Otherwise:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>