cuda_init() creates a "mos6522-cuda" device, but it's never realized.
Affects machines mac99 with via=cuda (default) and g3beige.

pmu_init() creates a "mos6522-pmu" device, but it's never realized.
Affects machine mac99 with via=pmu and via=pmu-adb,

In theory, a device becomes real only on realize.  In practice, the
transition from unreal to real is a fuzzy one.  The work to make a
device real can be spread between realize methods (fine),
instance_init methods (wrong), and board code wiring up the device
(fine as long as it effectively happens on realize).  Depending on
what exactly is done where, a device can work even when we neglect
to realize it.

These onetwo appear to work.  Nevertheless, it's a clear misuse of the
interface.  Even when it works today (more or less by chance), it can
break tomorrow.

Fix by realizing them in cuda_realize() and pmu_realize(),
respectively.

Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/macio/cuda.c | 10 +++++-----
 hw/misc/macio/pmu.c  | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e0cc0aac5d..763a785f1a 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -33,6 +33,7 @@
 #include "hw/misc/macio/cuda.h"
 #include "qemu/timer.h"
 #include "sysemu/runstate.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
 {
     CUDAState *s = CUDA(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
+                             &error_abort);
+
     /* Pass IRQ from 6522 */
-    d = DEVICE(&s->mos6522_cuda);
-    ms = MOS6522(d);
     sbd = SYS_BUS_DEVICE(s);
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_cuda));
 
     qemu_get_timedate(&tm, 0);
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 9a9cd427e1..4264779396 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -40,6 +40,7 @@
 #include "hw/misc/macio/pmu.h"
 #include "qemu/timer.h"
 #include "sysemu/runstate.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -740,15 +741,14 @@ static void pmu_realize(DeviceState *dev, Error **errp)
 {
     PMUState *s = VIA_PMU(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    object_property_set_bool(OBJECT(&s->mos6522_pmu), true, "realized",
+                             &error_abort);
+
     /* Pass IRQ from 6522 */
-    d = DEVICE(&s->mos6522_pmu);
-    ms = MOS6522(d);
     sbd = SYS_BUS_DEVICE(s);
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_pmu));
 
     qemu_get_timedate(&tm, 0);
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
-- 
2.21.3

On Thu, 28 May 2020 at 12:13, Markus Armbruster <armbru@redhat.com> wrote:
>
> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
> Affects machines mac99 with via=cuda (default) and g3beige.
>
> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
> Affects machine mac99 with via=pmu and via=pmu-adb,
>
> In theory, a device becomes real only on realize.  In practice, the
> transition from unreal to real is a fuzzy one.  The work to make a
> device real can be spread between realize methods (fine),
> instance_init methods (wrong), and board code wiring up the device
> (fine as long as it effectively happens on realize).  Depending on
> what exactly is done where, a device can work even when we neglect
> to realize it.
>
> These onetwo appear to work.  Nevertheless, it's a clear misuse of the
> interface.  Even when it works today (more or less by chance), it can
> break tomorrow.
>
> Fix by realizing them in cuda_realize() and pmu_realize(),
> respectively.
>
> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/cuda.c | 10 +++++-----
>  hw/misc/macio/pmu.c  | 10 +++++-----
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..763a785f1a 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -33,6 +33,7 @@
>  #include "hw/misc/macio/cuda.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>  {
>      CUDAState *s = CUDA(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>
> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
> +                             &error_abort);

Still disagree with barfing on failure when we have a perfectly
good way to return the failure indication.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 28 May 2020 at 12:13, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
>> Affects machines mac99 with via=cuda (default) and g3beige.
>>
>> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
>> Affects machine mac99 with via=pmu and via=pmu-adb,
>>
>> In theory, a device becomes real only on realize.  In practice, the
>> transition from unreal to real is a fuzzy one.  The work to make a
>> device real can be spread between realize methods (fine),
>> instance_init methods (wrong), and board code wiring up the device
>> (fine as long as it effectively happens on realize).  Depending on
>> what exactly is done where, a device can work even when we neglect
>> to realize it.
>>
>> These onetwo appear to work.  Nevertheless, it's a clear misuse of the
>> interface.  Even when it works today (more or less by chance), it can
>> break tomorrow.
>>
>> Fix by realizing them in cuda_realize() and pmu_realize(),
>> respectively.
>>
>> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/macio/cuda.c | 10 +++++-----
>>  hw/misc/macio/pmu.c  | 10 +++++-----
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..763a785f1a 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -33,6 +33,7 @@
>>  #include "hw/misc/macio/cuda.h"
>>  #include "qemu/timer.h"
>>  #include "sysemu/runstate.h"
>> +#include "qapi/error.h"
>>  #include "qemu/cutils.h"
>>  #include "qemu/log.h"
>>  #include "qemu/module.h"
>> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>>  {
>>      CUDAState *s = CUDA(dev);
>>      SysBusDevice *sbd;
>> -    MOS6522State *ms;
>> -    DeviceState *d;
>>      struct tm tm;
>>
>> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
>> +                             &error_abort);
>
> Still disagree with barfing on failure when we have a perfectly
> good way to return the failure indication.

My patch is a strict improvement: it fixes a bug, and it does not add
ways to fail (the object_property_set_bool() above can't actually fail).

You're asking for additional improvement.  "One may always ask, and one
may always say no."

Since there is nothing to clean up here, I'll stick in the useless error
handling so we can move on.

If the error handling you ask for involved cleanup, I'd say no.

Incorrect unreachable cleanup is worse than &error_abort.  I'm not going
to waste time on unreachable and untestable error handling unless it's
utterly trivial, and I'm certainly not going to waste time on creating
more elaborate time bombs.  I *am* going to waste time managing
expectations, if I have to :)

I feel I have to now, because I feel I've once again stretched my
employer's (awesomely generous!) patience with me doing work that won't
ever go into any of our products to the limit.

On 5/28/20 1:04 PM, Markus Armbruster wrote:
> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
> Affects machines mac99 with via=cuda (default) and g3beige.
> 
> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
> Affects machine mac99 with via=pmu and via=pmu-adb,
> 
> In theory, a device becomes real only on realize.  In practice, the
> transition from unreal to real is a fuzzy one.  The work to make a
> device real can be spread between realize methods (fine),
> instance_init methods (wrong), and board code wiring up the device
> (fine as long as it effectively happens on realize).  Depending on
> what exactly is done where, a device can work even when we neglect
> to realize it.
> 
> These onetwo appear to work.  Nevertheless, it's a clear misuse of the
> interface.  Even when it works today (more or less by chance), it can
> break tomorrow.
> 
> Fix by realizing them in cuda_realize() and pmu_realize(),
> respectively.
> 
> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/cuda.c | 10 +++++-----
>  hw/misc/macio/pmu.c  | 10 +++++-----
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..763a785f1a 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -33,6 +33,7 @@
>  #include "hw/misc/macio/cuda.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>  {
>      CUDAState *s = CUDA(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>  
> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
> +                             &error_abort);

Either use local_err and return on error, or simpler realize it in
cuda_init()...

> +
>      /* Pass IRQ from 6522 */
> -    d = DEVICE(&s->mos6522_cuda);
> -    ms = MOS6522(d);
>      sbd = SYS_BUS_DEVICE(s);
> -    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
> +    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_cuda));
>  
>      qemu_get_timedate(&tm, 0);
>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
> diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
> index 9a9cd427e1..4264779396 100644
> --- a/hw/misc/macio/pmu.c
> +++ b/hw/misc/macio/pmu.c
> @@ -40,6 +40,7 @@
>  #include "hw/misc/macio/pmu.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -740,15 +741,14 @@ static void pmu_realize(DeviceState *dev, Error **errp)
>  {
>      PMUState *s = VIA_PMU(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>  
> +    object_property_set_bool(OBJECT(&s->mos6522_pmu), true, "realized",
> +                             &error_abort);

Ditto.

> +
>      /* Pass IRQ from 6522 */
> -    d = DEVICE(&s->mos6522_pmu);
> -    ms = MOS6522(d);
>      sbd = SYS_BUS_DEVICE(s);
> -    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
> +    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_pmu));
>  
>      qemu_get_timedate(&tm, 0);
>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
>