Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type

[Alistair Francis]

On Tue, Aug 25, 2015 at 12:43 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Aug 24, 2015 at 4:36 PM, Alistair Francis
> <address@hidden> wrote:
>> On Mon, Aug 17, 2015 at 4:37 PM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> On Mon, Aug 17, 2015 at 3:33 PM, Andreas Färber <address@hidden> wrote:
>>>> Am 18.08.2015 um 00:24 schrieb Alistair Francis:
>>>>> On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite
>>>>> <address@hidden> wrote:
>>>>>> On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis
>>>>>> <address@hidden> wrote:
>>>>>>> If the ObjectClass has no type return NULL instead of trying to compare
>>>>>>> the type name.
>>>>>>>
>>>>>>
>>>>>> What was the issue?
>>>>>
>>>>> There is a seg fault in object_class_dynamic_cast() because there is
>>>>> no type in the ObjectClass struct.
>>>>
>>>> That should never happen, ever since TYPE_OBJECT is no longer NULL.
>>>>
>>>>> It happens when it is trying to cast the "pci-device", which is called
>>>>> from the ahci_irq_lower() function. The function is testing if the
>>>>> device is a pci device, so it should return NULL if it isn't valid.
>>>
>>> Yes so I vaguely remember this now. It is about MSI interrupts which
>>> have nothing to do with sysbus implementation. My solution was to rip
>>> that PCI specific stuff out of AHCI completely in my branch. Should
>>> sysbus and PCI AHCI classes install their own separate logic for this
>>> part via a virtualised hook?
>>>
>>> On the topic though, I notice many PCI devices have this MSI specific
>>> logic in them. Is it possible for devs to just treat interrupts as
>>> pins and the PCI layers do the MSI vs non-MSI logic switch in core
>>> layers?
>>>
>>> If Andreas' idea don't work this is still a core QOM bug though. I
>>> think object_dynamic_cast should not have this segfault when passed a
>>> non implementing object.
>>>
>>> Regards,
>>> Peter
>>>
>>>>
>>>> It rather sounds as if some build-time dependency is wrong, which we
>>>> used to run into for the Container type before Paolo macrofied this.
>>>>
>>>> Please try again with a clean build - if it still occurs, we'll need a
>>>> reproducible test case to investigate what is going on rather than
>>>> papering over a latent bug.
>>
>> Hey,
>>
>> Sorry abut the delay, but I didn't get a chance to look at this last
>> week. I tried with a clean setup and still see the seg fault.
>>
>> I will try to look into it more this week, but if anyone is interested
>> here are the steps to reproduce:
>>
>> On the latest mainline QEMU, with my 2nd and 3rd patches applied
>> $ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu"
>> --disable-pie --disable-sdl --disable-werror # This is what is
>> required at work
>> $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none
>> -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot
>> u-boot on QEMU
>>
>> The image I'm using is available at: http://1drv.ms/1NxDXLo
>>
>
> So it's not a core bug. That container_of in ahci_lower_irq is
> incorrectly assuming that the passed AHCIState * is always for a PCI,
> which it is not in the sysbus case. So it's incorrectly getting the
> offset of QOM the object and the QOM cast is treating some invalid
> offset into the (or past) object as a QOM object base address.
>
> The simplest solution is a back pointer in AHCIState to the
> encapsulating device (would be a DeviceState *). The container_of is
> replaced with a nav of this pointer and then the conditional PCI cast
> can work.

This seems to fix the problem. It seems hacky though, I can't find a
better way to check the validity of the PCIDevice. Any ideas?

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 02d85fa..77e58a9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -137,8 +137,11 @@ static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
 static void ahci_irq_lower(AHCIState *s, AHCIDevice *dev)
 {
     AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
-    PCIDevice *pci_dev =
-        (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
+    PCIDevice *pci_dev = NULL;
+
+    if (s->parent_obj) {
+        pci_dev = PCI_DEVICE(d);
+    }

     DPRINTF(0, "lower irq\n");

diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index c055d6b..ac7d2de 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -287,6 +287,8 @@ struct AHCIDevice {
 };

 typedef struct AHCIState {
+    DeviceState *parent_obj;
+
     AHCIDevice *dev;
     AHCIControlRegs control_regs;
     MemoryRegion mem;

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>>
>>>> Thanks,
>>>> Andreas
>>>>
>>>> --
>>>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG 
>>>> Nürnberg)
>>>
>