[Qemu-devel] [PATCH v4 0/4] xlnx-zynqmp: Connect the AHCI SATA device

[Qemu-devel] [PATCH v4 0/4] xlnx-zynqmp: Connect the AHCI SATA device

[Alistair Francis]

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(-)

[Qemu-devel] [PATCH v4 1/4] ahci: Separate the AHCI state structure into the header

[Alistair Francis]

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 <alistair.francis@xilinx.com>
Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48749c1..02d85fa 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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[]) {
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 79a463d..c055d6b 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -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 */
-- 
1.7.1

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

[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 <alistair.francis@xilinx.com>
---
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(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 02d85fa..bab6f5c 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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,
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 v4 3/4] xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort

[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 <alistair.francis@xilinx.com>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

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

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 388baef..6756c74 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -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) {
-- 
1.7.1

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

[Peter Crosthwaite]

On Thu, Aug 27, 2015 at 5:16 PM, Alistair Francis
<alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
> ---
> 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(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 02d85fa..bab6f5c 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -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);

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

> +    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,
> 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
>

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

[Alistair Francis]

On Thu, Aug 27, 2015 at 5:27 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Thu, Aug 27, 2015 at 5:16 PM, Alistair Francis
> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>> ---
>> 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(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 02d85fa..bab6f5c 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -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);
>
> 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);

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

Thanks,

Alistair

>
> Regards,
> Peter
>
>> +    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,
>> 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
>>
>