Re: [PATCH] hw/misc/mos6522: Fix bad class definition of the MOS6522 device

[Thomas Huth]

On 14/11/2024 12.29, Mark Cave-Ayland wrote:
> On 14/11/2024 10:46, Thomas Huth wrote:
>
> > When compiling QEMU with --enable-cfi, the "q800" m68k machine
> > currently crashes very early, when the q800_machine_init() function
> > tries to wire the interrupts of the "via1" device.
> > This happens because TYPE_MOS6522_Q800_VIA1 is supposed to be a
> > proper SysBus device, but its parent (TYPE_MOS6522) has a mistake
> > in its class definition where it is only derived from DeviceClass,
> > and not from SysBusDeviceClass, so we end up in funny memory access
> > issues here. Using the right class hierarchy for the MOS6522 device
> > fixes the problem.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2675
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   include/hw/misc/mos6522.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> > index fba45668ab..920871a598 100644
> > --- a/include/hw/misc/mos6522.h
> > +++ b/include/hw/misc/mos6522.h
> > @@ -154,7 +154,7 @@ struct MOS6522State {
> >   OBJECT_DECLARE_TYPE(MOS6522State, MOS6522DeviceClass, MOS6522)
> >   struct MOS6522DeviceClass {
> > -    DeviceClass parent_class;
> > +    SysBusDeviceClass parent_class;
> >       ResettablePhases parent_phases;
> >       void (*portB_write)(MOS6522State *dev);
>
> Ooof. I suspect I started using DeviceClass first before switching to 
> SysBusDeviceClass later to implement reset functionality. Anyhow the patch 
> looks good: thanks Thomas!
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks!

By the way, fun fact why this problem was likely only triggering with CFI 
enabled: SysBusDeviceClass just extends DeviceClass by two function 
pointers, second one is connect_irq_notifier. This one gets called from 
sysbus_connect_irq() (which is called by q800_machine_init()).

But MOS6522DeviceClass is directly derived from DeviceClass, that memory 
space is filled by the ResettablePhases struct instead. So instead of 
calling an irq notifier, the sysbus_connect_irq() likely called the 
ResettableHoldPhase() function instead! It likely did not crash without CFI, 
since the parameters are similar enough, but with CFI, this got successfully 
flagged as an illegal call via function pointer :-)

 Thomas