xlnx-zynqmp: Connect the AHCI SATA device

PATCH Superseded Alistair Francis 2 participants 6 messages Aug 28, 2015 Arm/Zynq architecture

Share & tools

Alistair Francis

author

2015-08-28 00:16 UTC #0 lore raw

v4 cover xlnx-zynqmp: Connect the AHCI SATA device

This series connects the AHCI SATA device to the ZynqMP machine. It requires a restructure of the AHCI file to make the AHCI state struct visible. It also requires a small change to the ahci_irq_lower() and ahci_irq_raise() functions to avoid assuming that the AHCIState is a child of AHCIPCIState.

V4:
- Remove unnesicary casts
- Use object_dynamic_cast() instead of object_class_dynamic_cast()
V3:
- Perform checks inside the ahci_irq_lower() and ahci_irq_raise() functions to ensure the correct parent object is used.
V2:
- Macroify the number of SATA ports
- Update the non-realise error_propagate() calls to
use error_abort instead.

Alistair Francis (4):
ahci: Seperate the AHCI state structure into the header ahci.c: Don't assume AHCIState's parent is AHCIPCIState xlnx-zynqmp.c: Convert some of the error_propagate() calls to
error_abort
xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

 hw/arm/xlnx-zynqmp.c         |   33 +++++++++++++++++++++------------
 hw/ide/ahci.c                |   40 +++++++++++++++++++++-------------------
 hw/ide/ahci.h                |   16 ++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |    3 +++
 4 files changed, 61 insertions(+), 31 deletions(-)

Alistair Francis

author

2015-08-28 00:16 UTC #1 lore raw

PATCH v4 1/4 ahci: Separate the AHCI state structure into the header

Pull the AHCI state structure out into the header. This allows other containers to access the struct. This is required to add the device to modern SoC containers.

Signed-off-by Alistair Francis <[email protected]>

Reviewed-by Sai Pavan Boddu <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---

 hw/ide/ahci.c |   13 -------------
 hw/ide/ahci.h |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 13 deletions(-)

hw/ide/ahci.c +0 ~~-13~~

index 48749c1..02d85fa 100644
@@ -25,7 +25,6 @@
 #include <hw/pci/msi.h>
 #include <hw/i386/pc.h>
 #include <hw/pci/pci.h>
-#include <hw/sysbus.h>
 
 #include "qemu/error-report.h"
 #include "sysemu/block-backend.h"

@@ -1625,18 +1624,6 @@ const VMStateDescription vmstate_ahci = {
     },
 };
 
-#define TYPE_SYSBUS_AHCI "sysbus-ahci"
-#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
-
-typedef struct SysbusAHCIState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    AHCIState ahci;
-    uint32_t num_ports;
-} SysbusAHCIState;
-
 static const VMStateDescription vmstate_sysbus_ahci = {
     .name = "sysbus-ahci",
     .fields = (VMStateField[]) {

hw/ide/ahci.h +14 -0

index 79a463d..c055d6b 100644
@@ -24,6 +24,8 @@
 #ifndef HW_IDE_AHCI_H
 #define HW_IDE_AHCI_H
 
+#include <hw/sysbus.h>
+
 #define AHCI_MEM_BAR_SIZE         0x1000
 #define AHCI_MAX_PORTS            32
 #define AHCI_MAX_SG               168 /* hardware max is 64K */

@@ -369,4 +371,16 @@ void ahci_reset(AHCIState *s);
 
 void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
 
+#define TYPE_SYSBUS_AHCI "sysbus-ahci"
+#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
+
+typedef struct SysbusAHCIState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    AHCIState ahci;
+    uint32_t num_ports;
+} SysbusAHCIState;
+
 #endif /* HW_IDE_AHCI_H */

Signature · 1 line

1.7.1

Alistair Francis

author

2015-08-28 00:17 UTC #2 lore raw

PATCH v4 2/4 ahci.c: Don't assume AHCIState's parent is AHCIPCIState

In reply to Alistair Francis

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 <[email protected]>

---
V4:
- Remove unnesicary casts
- Use object_dynamic_cast() instead of object_class_dynamic_cast()

 hw/ide/ahci.c |   27 +++++++++++++++++++++------
 hw/ide/ahci.h |    2 ++
 2 files changed, 23 insertions(+), 6 deletions(-)

hw/ide/ahci.c +21 -6

index 02d85fa..bab6f5c 100644
@@ -121,9 +121,16 @@ 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 = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);
+    if (ret) {
+        /* AHCIState parent is AHCIPCIState */
+        pci_dev = PCI_DEVICE(dev_state);
+    }
 
     DPRINTF(0, "raise irq\n");

@@ -136,9 +143,16 @@ 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 = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(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 +1450,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,

hw/ide/ahci.h +2 -0

index c055d6b..c9b3805 100644
@@ -287,6 +287,8 @@ struct AHCIDevice {
 };
 
 typedef struct AHCIState {
+    DeviceState *container;
+
     AHCIDevice *dev;
     AHCIControlRegs control_regs;
     MemoryRegion mem;

Signature · 1 line

1.7.1

Peter Crosthwaite

2015-08-28 00:27 UTC #3 lore raw

Re: [Qemu-devel] [PATCH v4 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState

Quoted 45 lines · Alistair Francis

Signed-off-by Alistair Francis <[email protected]>

---
V4:
- Remove unnesicary casts
- Use object_dynamic_cast() instead of object_class_dynamic_cast()

 hw/ide/ahci.c |   27 +++++++++++++++++++++------
 hw/ide/ahci.h |    2 ++
 2 files changed, 23 insertions(+), 6 deletions(-)

hw/ide/ahci.c +10 -4

index 02d85fa..bab6f5c 100644
@@ -121,9 +121,16 @@ 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 = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);
+    if (ret) {
+        /* AHCIState parent is AHCIPCIState */
+        pci_dev = PCI_DEVICE(dev_state);
+    }
 
     DPRINTF(0, "raise irq\n");

@@ -136,9 +143,16 @@ 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);

hw/ide/ahci.c static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev) · line 153

Quoted 10 lines

hw/ide/ahci.c +6 -2

-    PCIDevice *pci_dev =
-        (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
 
 
+    DeviceState *dev_state = s->container;
+    PCIDevice *pci_dev = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);

Is ret a correct type? object_dynamic_cast returns an object and you are pointer assigning to a class. I don't think it should need the extra variable at all. Does it work if all you do is this:

- AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
- PCIDevice *pci_dev =
- (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
+ PCIDevice *pci_dev =
+ (PCIDevice *)object_dynamic_cast(OBJECT(s->container),
TYPE_PCI_DEVICE);

Regards,
Peter

Quoted 31 lines

hw/ide/ahci.c +4 -0

+    if (ret) {
+        /* AHCIState parent is AHCIPCIState */
+        pci_dev = PCI_DEVICE(dev_state);
+    }

     DPRINTF(0, "lower irq\n");

hw/ide/ahci.c +1 -0

@@ -1436,6 +1450,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,

hw/ide/ahci.h +2 -1

index c055d6b..c9b3805 100644
@@ -287,6 +287,8 @@ struct AHCIDevice {
 };
 
 typedef struct AHCIState {
+    DeviceState *container;
+
     AHCIDevice *dev;
     AHCIControlRegs control_regs;
     MemoryRegion mem;
--

1.7.1

Alistair Francis

author

2015-08-28 16:29 UTC #4 lore raw

Re: [Qemu-devel] [PATCH v4 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState

Quoted 67 lines · Peter Crosthwaite

Quoted 45 lines · Alistair Francis

Signed-off-by Alistair Francis <[email protected]>

---
V4:
- Remove unnesicary casts
- Use object_dynamic_cast() instead of object_class_dynamic_cast()

 hw/ide/ahci.c |   27 +++++++++++++++++++++------
 hw/ide/ahci.h |    2 ++
 2 files changed, 23 insertions(+), 6 deletions(-)

hw/ide/ahci.c +10 -4

index 02d85fa..bab6f5c 100644
@@ -121,9 +121,16 @@ 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 = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);
+    if (ret) {
+        /* AHCIState parent is AHCIPCIState */
+        pci_dev = PCI_DEVICE(dev_state);
+    }
 
     DPRINTF(0, "raise irq\n");

@@ -136,9 +143,16 @@ 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);

hw/ide/ahci.c static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev) · line 153

Quoted 10 lines

hw/ide/ahci.c +6 -2

-    PCIDevice *pci_dev =
-        (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
 
 
+    DeviceState *dev_state = s->container;
+    PCIDevice *pci_dev = NULL;
+    ObjectClass *ret;
+
+    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
+    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);

Is ret a correct type? object_dynamic_cast returns an object and you are pointer assigning to a class. I don't think it should need the extra variable at all. Does it work if all you do is this:

hw/ide/ahci.c +2 -3

-    AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
-    PCIDevice *pci_dev =
-        (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
+    PCIDevice *pci_dev =
+        (PCIDevice *)object_dynamic_cast(OBJECT(s->container),

Quoted 1 line

TYPE_PCI_DEVICE);

Good catch, that was left over from what I was doing before.

Thanks,

Alistair

Quoted 36 lines

Regards,
Peter

Quoted 31 lines

hw/ide/ahci.c +4 -0

+    if (ret) {
+        /* AHCIState parent is AHCIPCIState */
+        pci_dev = PCI_DEVICE(dev_state);
+    }

     DPRINTF(0, "lower irq\n");

hw/ide/ahci.c +1 -0

@@ -1436,6 +1450,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,

hw/ide/ahci.h +2 -1

index c055d6b..c9b3805 100644
@@ -287,6 +287,8 @@ struct AHCIDevice {
 };
 
 typedef struct AHCIState {
+    DeviceState *container;
+
     AHCIDevice *dev;
     AHCIControlRegs control_regs;
     MemoryRegion mem;
--

1.7.1

Alistair Francis

author

2015-08-28 00:17 UTC #5 lore raw

PATCH v4 3/4 xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort

In reply to Alistair Francis

Convert all of the non-realize error_propagate() calls into error_abort calls as they shouldn't be user visible failure cases.

Signed-off-by Alistair Francis <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---

 hw/arm/xlnx-zynqmp.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

hw/arm/xlnx-zynqmp.c +2 ~~-12~~

index 388baef..6756c74 100644
@@ -162,12 +162,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         g_free(name);
 
         object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
-                                "reset-cbar", &err);
-        if (err) {
-            error_propagate((errp), (err));
-            return;
-        }
-
+                                "reset-cbar", &error_abort);
         object_property_set_bool(OBJECT(&s->apu_cpu[i]), true, "realized",
                                  &err);
         if (err) {

@@ -200,12 +195,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         g_free(name);
 
         object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
-                                 &err);
-        if (err != NULL) {
-            error_propagate(errp, err);
-            return;
-        }
-
+                                 &error_abort);
         object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "realized",
                                  &err);
         if (err) {

Signature · 1 line

1.7.1