qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast retu


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Date: Wed, 26 Aug 2015 14:02:27 -0700

On Wed, Aug 26, 2015 at 1:36 PM, Alistair Francis
<address@hidden> wrote:
> 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.

I assume you have the appropriate setters for the new variable
elsewhere in the code as well?

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

So there a few problems in the way of a correct solution. The caller
for ahci_lower_irq does not have access to the QOM object pointer,
it's been abstracted away by AHCIState (which is not a QOM object). So
you would need to replumb the call path to ahci_lower_irq to pass the
QOM object. This would let you drop the container_of completely.

The next step would be to virtualise ahci_lower_irq, as this is
implementation dependent (assume specific devices really do need to
control the use of PCI MSI?), one implementation for sysbus, one for
PCI. This is blocked by the re-plumbing described above as the
virtualised called itself will need a ptr to the QOM object.

But I think the back ptr is an acceptable solution for the meantime,
this is a clear bug in Sysbus AHCI and should probably even go to
qemu-stable.

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

I would make the parent obj compulsory for all AHCIState
implementations and drop the NULL guard.

> +        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;

This name is really for QOM inline parents. We decided a while back to
use "parent" for the QOM parents and "container" for non-parental
containers. Memory regions use the .container field for a similar
purpose.

Regards,
Peter

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]