| Message ID | 20241114104653.963812-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/misc/mos6522: Fix bad class definition of the MOS6522 device | expand |
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> ATB, Mark.
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
On 14/11/24 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(-) Fixes: 51f233ec92 ("misc: introduce new mos6522 VIA device") and Cc qemu-stable? Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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);
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(-)