hw/misc/mos6522: Fix bad class definition of the MOS6522 device

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

Posted by Thomas Huth 1 week, 2 days ago

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);
-- 
2.47.0

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

Posted by Philippe Mathieu-Daudé 5 days, 1 hour ago

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(-)

Patch queued to hw-misc, thanks.

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

Posted by Philippe Mathieu-Daudé 1 week, 1 day ago

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>

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

Posted by Mark Cave-Ayland 1 week, 2 days ago

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.

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

Posted by Thomas Huth 1 week, 1 day ago

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