[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: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState |
Date: |
Thu, 27 Aug 2015 15:06:37 -0700 |
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.
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