[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/4] ahci.c: Don't assume AHCIState's parent
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState |
Date: |
Thu, 27 Aug 2015 16:25:48 -0700 |
On Thu, Aug 27, 2015 at 3:58 PM, Alistair Francis
<address@hidden> wrote:
> ()
>
> On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite
> <address@hidden> wrote:
>> On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis
>> <address@hidden> wrote:
>>> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
>>> <address@hidden> wrote:
>>>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
>>>> <address@hidden> wrote:
>>>>> The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
>>>>
>>>> "struct"
>>>
>>> Good catch, will fix
>>>
>>>>
>>>>> as a parent. The ahci_irq_lower() and ahci_irq_raise() functions
>>>>> assume that it is always AHCIPCIState, which is not always the
>>>>> case, which causes a seg fault. Verify what the container of AHCIState
>>>>> is before setting the PCIDevice struct.
>>>>>
>>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>>> ---
>>>>>
>>>>> hw/ide/ahci.c | 29 +++++++++++++++++++++++------
>>>>> hw/ide/ahci.h | 2 ++
>>>>> 2 files changed, 25 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>>> index 02d85fa..b45b21d 100644
>>>>> --- a/hw/ide/ahci.c
>>>>> +++ b/hw/ide/ahci.c
>>>>> @@ -121,9 +121,17 @@ static uint32_t ahci_port_read(AHCIState *s, int
>>>>> port, int offset)
>>>>>
>>>>> static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
>>>>> {
>>>>> - AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
>>>>> - PCIDevice *pci_dev =
>>>>> - (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
>>>>
>>>> Why can't you just convert this to use use s->container? ...
>>>>
>>>>> + DeviceState *dev_state = DEVICE(s->container);
>>>>
>>>> Unneeded cast. s->container is already DeviceState, but I'm not sure
>>>> you need this local variable at all.
>>>
>>> I'll remove the cast. The variable is just included to save line space
>>> (and complexity).
>>>
>>>>
>>>>> + PCIDevice *pci_dev = NULL;
>>>>> + ObjectClass *ret;
>>>>> +
>>>>> + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>>>>> + ret = object_class_dynamic_cast(object_get_class(OBJECT(dev_state)),
>>>>> + TYPE_PCI_DEVICE);
>>>>
>>>> ... I don't understand why this needs to be done as a class cast, does
>>>> the original object cast approach not work?
>>>
>>> The problem with the standard ones is that they contain asserts. So you
>>> can't
>>> do a test to see if the cast is possible, which is what I want to do.
>>>
>>
>> But the original code uses object_dynamic_cast which should be a
>> non-asserting cast attempt. What was the assertion you were getting?
>
> Oh! I see what you mean. I can use the object_dynamic_cast() instead.
> I thought you meant to use the macros that are defined.
>
> If there are no other comments I will re-send the series.
>
Nothing more from me :)
Regards,
Peter
> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>> Thanks,
>>>
>>> Alistair
>>>
>>>>
>>>> Regards,
>>>> Peter
>>>>
>>>>> + if (ret) {
>>>>> + /* AHCIState parent is AHCIPCIState */
>>>>> + pci_dev = PCI_DEVICE(dev_state);
>>>>> + }
>>>>>
>>>>> DPRINTF(0, "raise irq\n");
>>>>>
>>>>> @@ -136,9 +144,17 @@ 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);
>>>>> + DeviceState *dev_state = DEVICE(s->container);
>>>>> + PCIDevice *pci_dev = NULL;
>>>>> + ObjectClass *ret;
>>>>> +
>>>>> + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>>>>> + ret = object_class_dynamic_cast(object_get_class(OBJECT(dev_state)),
>>>>> + TYPE_PCI_DEVICE);
>>>>> + if (ret) {
>>>>> + /* AHCIState parent is AHCIPCIState */
>>>>> + pci_dev = PCI_DEVICE(dev_state);
>>>>> + }
>>>>>
>>>>> DPRINTF(0, "lower irq\n");
>>>>>
>>>>> @@ -1436,6 +1452,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev,
>>>>> AddressSpace *as, int ports)
>>>>> s->as = as;
>>>>> s->ports = ports;
>>>>> s->dev = g_new0(AHCIDevice, ports);
>>>>> + s->container = qdev;
>>>>> ahci_reg_init(s);
>>>>> /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for
>>>>> now */
>>>>> memory_region_init_io(&s->mem, OBJECT(qdev), &ahci_mem_ops, s,
>>>>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>>>>> index c055d6b..c9b3805 100644
>>>>> --- a/hw/ide/ahci.h
>>>>> +++ b/hw/ide/ahci.h
>>>>> @@ -287,6 +287,8 @@ struct AHCIDevice {
>>>>> };
>>>>>
>>>>> typedef struct AHCIState {
>>>>> + DeviceState *container;
>>>>> +
>>>>> AHCIDevice *dev;
>>>>> AHCIControlRegs control_regs;
>>>>> MemoryRegion mem;
>>>>> --
>>>>> 1.7.1
>>>>>
>>>>
>>
[Qemu-devel] [PATCH v3 3/4] xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort, Alistair Francis, 2015/08/27