Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
Paolo Bonzini writes: > On 27/05/20 17:05, Peter Maydell wrote: >> I disagree with these. We're in a realize function, the API >> says "on errors, report them via the Error* you got passed", >> so we should do that, not blow up. &error_abort only makes >> sense if (a) we have no better way to report errors than >> to abort (which isn't the case here) or (b) if we can guarantee >> that in fact the thing we're doing won't ever fail >> (which we can't here without knowing more about the internal >> implementation details of the MOS6522 device than we >> really ought to). > > Note however that before replacing &error_abort with error propagation > you need to make sure that you are "un-realizing" yourself properly. So > it may be better to have inferior (but clearly visible) error > propagation behavior, than untested (and perhaps untestable) buggy code > that looks great on the surface. This is exactly why I have to stop at &error_abort in this series. It's 24 patches of fixes to enable 50+ patches of refactoring, with more in the pipeline. If I stray even deeper into the weeds, my pipeline is going to explode %-}
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
Peter Maydell writes: > On Wed, 27 May 2020 at 15:12, Markus Armbruster wrote: >> * PATCH 08: in a realize method. Can't actually fail, so let's use >> &error_abort. >> >> * PATCH 09 (this one): likewise. > > I disagree with these. We're in a realize function, the API > says "on errors, report them via the Error* you got passed", > so we should do that, not blow up. &error_abort only makes > sense if (a) we have no better way to report errors than > to abort (which isn't the case here) or (b) if we can guarantee > that in fact the thing we're doing won't ever fail I detest impossible (and therefore untestable) error paths. > (which we can't here without knowing more about the internal > implementation details of the MOS6522 device than we > really ought to). At least the child devices are all defined in the same file. My second line of defense: my patches are strict improvments. If you want further improvements, I'd prefer you propose them as patches on top of mine.
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
On 27/05/20 17:05, Peter Maydell wrote: > I disagree with these. We're in a realize function, the API > says "on errors, report them via the Error* you got passed", > so we should do that, not blow up. &error_abort only makes > sense if (a) we have no better way to report errors than > to abort (which isn't the case here) or (b) if we can guarantee > that in fact the thing we're doing won't ever fail > (which we can't here without knowing more about the internal > implementation details of the MOS6522 device than we > really ought to). Note however that before replacing &error_abort with error propagation you need to make sure that you are "un-realizing" yourself properly. So it may be better to have inferior (but clearly visible) error propagation behavior, than untested (and perhaps untestable) buggy code that looks great on the surface. Paolo
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
On Wed, 27 May 2020 at 15:12, Markus Armbruster wrote: > * PATCH 08: in a realize method. Can't actually fail, so let's use > &error_abort. > > * PATCH 09 (this one): likewise. I disagree with these. We're in a realize function, the API says "on errors, report them via the Error* you got passed", so we should do that, not blow up. &error_abort only makes sense if (a) we have no better way to report errors than to abort (which isn't the case here) or (b) if we can guarantee that in fact the thing we're doing won't ever fail (which we can't here without knowing more about the internal implementation details of the MOS6522 device than we really ought to). thanks -- PMM
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
Peter Maydell writes: [...] > Since we init the device with sysbus_init_child_obj() and > we're in a position here to propagate any realize error > back up to our caller, it would be nicer to do this via > setting the realize property rather than qdev_init_nofail(). I checked all my patches for new uses of qdev_init_nofail(): * PATCH 02: in realize(), but right next to existing uses. I'm not making it worse. * PATCH 03: in helper functions used by board init functions. Okay. * PATCH 08: in a realize method. Can't actually fail, so let's use &error_abort. * PATCH 09 (this one): likewise. * PATCH 18: in a realize method, and in a realize helper that can't fail. One of the callers uses qdev_init_nofail() already. Let's Use &error_abort. [...]
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
Peter Maydell writes:
> On Mon, 18 May 2020 at 06:12, 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,
>>
>> I wonder how this ever worked. If the "device becomes real only on
>> realize" thing actually works, then we've always been missing these
>> devices, yet nobody noticed.
>
> Same remark as elsewhere: the devices aren't missing, we just
> got lucky that using them in a half-initialized state happens
> to work for these devices. Could you update the commit messages
> when you reroll this series, please?
Of course. What about something like this:
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. 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: 6dca62af95e0b7020aa00d0ca9b2c421f341
>> Cc: Laurent Vivier
>> Signed-off-by: Markus Armbruster
>> ---
>> hw/misc/macio/cuda.c | 8 +++-
>> hw/misc/macio/pmu.c | 8 +++-
>> 2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..6d4d135f71 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error
>> **errp)
>> {
>> CUDAState *s = CUDA(dev);
>> SysBusDevice *sbd;
>> -MOS6522State *ms;
>> -DeviceState *d;
>> struct tm tm;
>>
>> +qdev_init_nofail(DEVICE(&s->mos6522_cuda));
>
> Since we init the device with sysbus_init_child_obj() and
> we're in a position here to propagate any realize error
> back up to our caller, it would be nicer to do this via
> setting the realize property rather than qdev_init_nofail().
The error handling will be unreachable.
The proper way to say "error not possible" is of course &error_abort,
not qdev_init_nofail()'s &error_fatal.
Many realize methods misuse the Error interface this way. NULL instead
of &error_abort is also common. Cleaning this up is yet another big
task. But that's no excuse for making it bigger now.
Laurent, would you prefer unreachable error handling or &error_abort?
> That's what this patch from March does:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg04260.html
> (we seem to have let that series drop accidentally,
> probably because it was halfway through release and
> because it touches several architectures at once).
Pity.
Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
On Mon, 18 May 2020 at 06:12, 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,
>
> I wonder how this ever worked. If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> devices, yet nobody noticed.
Same remark as elsewhere: the devices aren't missing, we just
got lucky that using them in a half-initialized state happens
to work for these devices. Could you update the commit messages
when you reroll this series, please?
> Fix by realizing them in cuda_realize() and pmu_realize(),
> respectively.
>
> Fixes: 6dca62af95e0b7020aa00d0ca9b2c421f341
> Cc: Laurent Vivier
> Signed-off-by: Markus Armbruster
> ---
> hw/misc/macio/cuda.c | 8 +++-
> hw/misc/macio/pmu.c | 8 +++-
> 2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..6d4d135f71 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error **errp)
> {
> CUDAState *s = CUDA(dev);
> SysBusDevice *sbd;
> -MOS6522State *ms;
> -DeviceState *d;
> struct tm tm;
>
> +qdev_init_nofail(DEVICE(&s->mos6522_cuda));
Since we init the device with sysbus_init_child_obj() and
we're in a position here to propagate any realize error
back up to our caller, it would be nicer to do this via
setting the realize property rather than qdev_init_nofail().
That's what this patch from March does:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg04260.html
(we seem to have let that series drop accidentally,
probably because it was halfway through release and
because it touches several architectures at once).
thanks
-- PMM
[PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
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,
I wonder how this ever worked. If the "device becomes real only on
realize" thing actually works, then we've always been missing these
devices, yet nobody noticed.
Fix by realizing them in cuda_realize() and pmu_realize(),
respectively.
Fixes: 6dca62af95e0b7020aa00d0ca9b2c421f341
Cc: Laurent Vivier
Signed-off-by: Markus Armbruster
---
hw/misc/macio/cuda.c | 8 +++-
hw/misc/macio/pmu.c | 8 +++-
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e0cc0aac5d..6d4d135f71 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error **errp)
{
CUDAState *s = CUDA(dev);
SysBusDevice *sbd;
-MOS6522State *ms;
-DeviceState *d;
struct tm tm;
+qdev_init_nofail(DEVICE(&s->mos6522_cuda));
+
/* 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..e29ca5e6cc 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -740,15 +740,13 @@ static void pmu_realize(DeviceState *dev, Error **errp)
{
PMUState *s = VIA_PMU(dev);
SysBusDevice *sbd;
-MOS6522State *ms;
-DeviceState *d;
struct tm tm;
+qdev_init_nofail(DEVICE(&s->mos6522_pmu));
+
/* 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.1
