From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWXif-00028h-GE for qemu-devel@nongnu.org; Mon, 31 Aug 2015 18:38:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWXib-0004Cy-FX for qemu-devel@nongnu.org; Mon, 31 Aug 2015 18:38:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWXib-0004Co-7i for qemu-devel@nongnu.org; Mon, 31 Aug 2015 18:38:25 -0400 References: <3b6ebc85594630a62a0b5972063f87625a44f1d6.1440806501.git.alistair.francis@xilinx.com> From: John Snow Message-ID: <55E4D75F.5070406@redhat.com> Date: Mon, 31 Aug 2015 18:38:23 -0400 MIME-Version: 1.0 In-Reply-To: <3b6ebc85594630a62a0b5972063f87625a44f1d6.1440806501.git.alistair.francis@xilinx.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis , qemu-devel@nongnu.org, peter.maydell@linaro.org, crosthwaitepeter@gmail.com Cc: saipava@xilinx.com, edgar.iglesias@xilinx.com, afaerber@suse.de On 08/28/2015 08:04 PM, Alistair Francis wrote: > The AHCIState struct can either have AHCIPCIState or SysbusAHCIState > 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 > --- > V5: > - Remove the return checks when setting PCIDevice > V4: > - Remove unnesicary casts > - Use object_dynamic_cast() instead of object_class_dynamic_cast() > > hw/ide/ahci.c | 13 +++++++------ > hw/ide/ahci.h | 2 ++ > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 02d85fa..d83efa4 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -121,9 +121,9 @@ 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); > + DeviceState *dev_state = s->container; > + PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), > + TYPE_PCI_DEVICE); > > DPRINTF(0, "raise irq\n"); > > @@ -136,9 +136,9 @@ 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 = s->container; > + PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), > + TYPE_PCI_DEVICE); > > DPRINTF(0, "lower irq\n"); > > @@ -1436,6 +1436,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; > This is kind of ugly ... but it works, and it doesn't impact migratability. If someone abstracts MSI away from AHCI in the future, this can be un-done and the state cleaned up again. Doesn't break anything, so: Acked-by: John Snow