From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZV68g-00031U-53 for qemu-devel@nongnu.org; Thu, 27 Aug 2015 18:59:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZV68e-0007hs-RI for qemu-devel@nongnu.org; Thu, 27 Aug 2015 18:59:22 -0400 Received: from mail-oi0-x233.google.com ([2607:f8b0:4003:c06::233]:33794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZV68e-0007hP-Hh for qemu-devel@nongnu.org; Thu, 27 Aug 2015 18:59:20 -0400 Received: by oiex83 with SMTP id x83so20764149oie.1 for ; Thu, 27 Aug 2015 15:59:19 -0700 (PDT) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: References: From: Alistair Francis Date: Thu, 27 Aug 2015 15:58:50 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v3 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: Peter Crosthwaite Cc: Edgar Iglesias , Peter Maydell , "qemu-devel@nongnu.org Developers" , Alistair Francis , Sai Pavan Boddu , John Snow , =?UTF-8?Q?Andreas_F=C3=A4rber?= () On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite wrote: > On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis > wrote: >> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite >> wrote: >>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis >>> 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 >>>> --- >>>> >>>> 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. 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 >>>> >>> >