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

[Qemu-devel] [PATCH v6 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.

V6:
 - Fix up Macros
V5:
 - Remove the return checks when setting PCIDevice
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         |   32 ++++++++++++++++++++------------
 hw/ide/ahci.c                |   26 +++++++-------------------
 hw/ide/ahci.h                |   16 ++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |    3 +++
 4 files changed, 46 insertions(+), 31 deletions(-)

[Qemu-devel] [PATCH v6 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 v6 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>
---
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;
-- 
1.7.1

[Qemu-devel] [PATCH v6 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 v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[John Snow]

On 08/28/2015 08:04 PM, Alistair Francis wrote:
> Connect the Sysbus AHCI device to ZynqMP.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>
> ---
> V6:
>  - Fix up Macros
> V2:
>  - Marcoify the number of SATA prts
>  - Change the error for setting num-ports to error_abort
> 
>  hw/arm/xlnx-zynqmp.c         |   18 ++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h |    3 +++
>  2 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 6756c74..90b2aca 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -28,6 +28,10 @@
>  #define GIC_DIST_ADDR       0xf9010000
>  #define GIC_CPU_ADDR        0xf9020000
>  
> +#define SATA_INTR           133
> +#define SATA_ADDR           0xFD0C0000
> +#define SATA_NUM_PORTS      2
> +
>  static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
>      0xFF0B0000, 0xFF0C0000, 0xFF0D0000, 0xFF0E0000,
>  };
> @@ -90,6 +94,9 @@ static void xlnx_zynqmp_init(Object *obj)
>          object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
>          qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
>      }
> +
> +    object_initialize(&s->sata, sizeof(s->sata), TYPE_SYSBUS_AHCI);
> +    qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default());
>  }
>  
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> @@ -240,6 +247,17 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0,
>                             gic_spi[uart_intr[i]]);
>      }
> +
> +    object_property_set_int(OBJECT(&s->sata), SATA_NUM_PORTS, "num-ports",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->sata), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));
> +        return;
> +    }
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sata), 0, SATA_ADDR);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
>  }
>  
>  static Property xlnx_zynqmp_props[] = {
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 6ccb57b..97622ec 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -22,6 +22,8 @@
>  #include "hw/intc/arm_gic.h"
>  #include "hw/net/cadence_gem.h"
>  #include "hw/char/cadence_uart.h"
> +#include "hw/ide/pci.h"
> +#include "hw/ide/ahci.h"
>  
>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
> @@ -60,6 +62,7 @@ typedef struct XlnxZynqMPState {
>  
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
> +    SysbusAHCIState sata;
>  
>      char *boot_cpu;
>      ARMCPU *boot_cpu_ptr;
> 

For some reason, this patch never made it to the qemu-devel archives for
either v5 or v6. I only received a direct copy to my inbox, but the
mailer daemon seems to have dropped this patch for ... some reason or
another.

--js

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Alistair Francis]

On Mon, Aug 31, 2015 at 1:54 PM, John Snow <jsnow@redhat.com> wrote:
>
>
> On 08/28/2015 08:04 PM, Alistair Francis wrote:
>> Connect the Sysbus AHCI device to ZynqMP.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>
>> ---
>> V6:
>>  - Fix up Macros
>> V2:
>>  - Marcoify the number of SATA prts
>>  - Change the error for setting num-ports to error_abort
>>
>>  hw/arm/xlnx-zynqmp.c         |   18 ++++++++++++++++++
>>  include/hw/arm/xlnx-zynqmp.h |    3 +++
>>  2 files changed, 21 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 6756c74..90b2aca 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -28,6 +28,10 @@
>>  #define GIC_DIST_ADDR       0xf9010000
>>  #define GIC_CPU_ADDR        0xf9020000
>>
>> +#define SATA_INTR           133
>> +#define SATA_ADDR           0xFD0C0000
>> +#define SATA_NUM_PORTS      2
>> +
>>  static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
>>      0xFF0B0000, 0xFF0C0000, 0xFF0D0000, 0xFF0E0000,
>>  };
>> @@ -90,6 +94,9 @@ static void xlnx_zynqmp_init(Object *obj)
>>          object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
>>          qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
>>      }
>> +
>> +    object_initialize(&s->sata, sizeof(s->sata), TYPE_SYSBUS_AHCI);
>> +    qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default());
>>  }
>>
>>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>> @@ -240,6 +247,17 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0,
>>                             gic_spi[uart_intr[i]]);
>>      }
>> +
>> +    object_property_set_int(OBJECT(&s->sata), SATA_NUM_PORTS, "num-ports",
>> +                            &error_abort);
>> +    object_property_set_bool(OBJECT(&s->sata), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate((errp), (err));
>> +        return;
>> +    }
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sata), 0, SATA_ADDR);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
>>  }
>>
>>  static Property xlnx_zynqmp_props[] = {
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index 6ccb57b..97622ec 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -22,6 +22,8 @@
>>  #include "hw/intc/arm_gic.h"
>>  #include "hw/net/cadence_gem.h"
>>  #include "hw/char/cadence_uart.h"
>> +#include "hw/ide/pci.h"
>> +#include "hw/ide/ahci.h"
>>
>>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>> @@ -60,6 +62,7 @@ typedef struct XlnxZynqMPState {
>>
>>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
>> +    SysbusAHCIState sata;
>>
>>      char *boot_cpu;
>>      ARMCPU *boot_cpu_ptr;
>>
>
> For some reason, this patch never made it to the qemu-devel archives for
> either v5 or v6. I only received a direct copy to my inbox, but the
> mailer daemon seems to have dropped this patch for ... some reason or
> another.

I just noticed the same thing. I talked to Xilinx IT and they say that the email
was delivered from our servers. Is there a way to check to see what happened
on the QEMU mailing list side?

Thanks,

Alistair

>
> --js
>

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

[John Snow]

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 <alistair.francis@xilinx.com>
> ---
> 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 <jsnow@redhat.com>

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

[Alistair Francis]

On Mon, Aug 31, 2015 at 3:38 PM, John Snow <jsnow@redhat.com> wrote:
>
>
> 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 <alistair.francis@xilinx.com>
>> ---
>> 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.

I agree, not ideal but I think it's the best option at the moment, without
re-working the code.

>
> Doesn't break anything, so:
> Acked-by: John Snow <jsnow@redhat.com>

Thanks

Alistair
>

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Peter Maydell]

On 31 August 2015 at 22:28, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Mon, Aug 31, 2015 at 1:54 PM, John Snow <jsnow@redhat.com> wrote:
>> For some reason, this patch never made it to the qemu-devel archives for
>> either v5 or v6. I only received a direct copy to my inbox, but the
>> mailer daemon seems to have dropped this patch for ... some reason or
>> another.
>
> I just noticed the same thing. I talked to Xilinx IT and they say that the email
> was delivered from our servers. Is there a way to check to see what happened
> on the QEMU mailing list side?

Not sure -- we use the savannah mail servers, not ones we run ourselves.
Can you just resend the series, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Alistair Francis]

On Fri, Sep 4, 2015 at 6:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 August 2015 at 22:28, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Mon, Aug 31, 2015 at 1:54 PM, John Snow <jsnow@redhat.com> wrote:
>>> For some reason, this patch never made it to the qemu-devel archives for
>>> either v5 or v6. I only received a direct copy to my inbox, but the
>>> mailer daemon seems to have dropped this patch for ... some reason or
>>> another.
>>
>> I just noticed the same thing. I talked to Xilinx IT and they say that the email
>> was delivered from our servers. Is there a way to check to see what happened
>> on the QEMU mailing list side?
>
> Not sure -- we use the savannah mail servers, not ones we run ourselves.
> Can you just resend the series, please?

I just re-sent the set. Hopefully it works this time, otherwise I'll
look at sending
it through my Gmail to see if that helps

Thanks,

Alistair

>
> thanks
> -- PMM
>

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

[Peter Crosthwaite]

On Fri, Sep 4, 2015 at 8:17 AM, 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>

John Snow's ACK is missing. If he does it again on this V, it wont
need a respin as patches can pick it from list, otherwise will need a
V6 or committer has to pick it up.

Patch is good:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> ---
> 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;
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH RESEND v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Peter Crosthwaite]

On Fri, Sep 4, 2015 at 8:17 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Connect the Sysbus AHCI device to ZynqMP.
>
> 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>

> ---
> V6:
>  - Fix up Macros
> V2:
>  - Marcoify the number of SATA prts
>  - Change the error for setting num-ports to error_abort
>
>  hw/arm/xlnx-zynqmp.c         |   18 ++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h |    3 +++
>  2 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 6756c74..90b2aca 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -28,6 +28,10 @@
>  #define GIC_DIST_ADDR       0xf9010000
>  #define GIC_CPU_ADDR        0xf9020000
>
> +#define SATA_INTR           133
> +#define SATA_ADDR           0xFD0C0000
> +#define SATA_NUM_PORTS      2
> +
>  static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
>      0xFF0B0000, 0xFF0C0000, 0xFF0D0000, 0xFF0E0000,
>  };
> @@ -90,6 +94,9 @@ static void xlnx_zynqmp_init(Object *obj)
>          object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
>          qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
>      }
> +
> +    object_initialize(&s->sata, sizeof(s->sata), TYPE_SYSBUS_AHCI);
> +    qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default());
>  }
>
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> @@ -240,6 +247,17 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0,
>                             gic_spi[uart_intr[i]]);
>      }
> +
> +    object_property_set_int(OBJECT(&s->sata), SATA_NUM_PORTS, "num-ports",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->sata), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));
> +        return;
> +    }
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sata), 0, SATA_ADDR);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
>  }
>
>  static Property xlnx_zynqmp_props[] = {
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 6ccb57b..97622ec 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -22,6 +22,8 @@
>  #include "hw/intc/arm_gic.h"
>  #include "hw/net/cadence_gem.h"
>  #include "hw/char/cadence_uart.h"
> +#include "hw/ide/pci.h"

That's a bug in ahci.h that we should fix.

Regards,
Peter

> +#include "hw/ide/ahci.h"
>
>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
> @@ -60,6 +62,7 @@ typedef struct XlnxZynqMPState {
>
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
> +    SysbusAHCIState sata;
>
>      char *boot_cpu;
>      ARMCPU *boot_cpu_ptr;
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[John Snow]

On 09/04/2015 09:42 AM, Peter Maydell wrote:
> On 31 August 2015 at 22:28, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Mon, Aug 31, 2015 at 1:54 PM, John Snow <jsnow@redhat.com> wrote:
>>> For some reason, this patch never made it to the qemu-devel archives for
>>> either v5 or v6. I only received a direct copy to my inbox, but the
>>> mailer daemon seems to have dropped this patch for ... some reason or
>>> another.
>>
>> I just noticed the same thing. I talked to Xilinx IT and they say that the email
>> was delivered from our servers. Is there a way to check to see what happened
>> on the QEMU mailing list side?
> 
> Not sure -- we use the savannah mail servers, not ones we run ourselves.
> Can you just resend the series, please?
> 
> thanks
> -- PMM
> 

For some reason this still looks all goofed up:

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01180.html

I am not sure what to make of this anymore, since the reply to the
original is showing up under the resend here. We're bending the laws of
space and time.

We might need to ask the Savannah people what's going on...

--js


P.S.: Alistair, if you do resend from a different address, please attach
my Acked-by to patches 1 & 2. Sorry for the huge hassle here, I have no
idea why the list is exploding.

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Alistair Francis]

On Fri, Sep 4, 2015 at 12:59 PM, John Snow <jsnow@redhat.com> wrote:
>
>
> On 09/04/2015 09:42 AM, Peter Maydell wrote:
>> On 31 August 2015 at 22:28, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Mon, Aug 31, 2015 at 1:54 PM, John Snow <jsnow@redhat.com> wrote:
>>>> For some reason, this patch never made it to the qemu-devel archives for
>>>> either v5 or v6. I only received a direct copy to my inbox, but the
>>>> mailer daemon seems to have dropped this patch for ... some reason or
>>>> another.
>>>
>>> I just noticed the same thing. I talked to Xilinx IT and they say that the email
>>> was delivered from our servers. Is there a way to check to see what happened
>>> on the QEMU mailing list side?
>>
>> Not sure -- we use the savannah mail servers, not ones we run ourselves.
>> Can you just resend the series, please?
>>
>> thanks
>> -- PMM
>>
>
> For some reason this still looks all goofed up:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01180.html
>
> I am not sure what to make of this anymore, since the reply to the
> original is showing up under the resend here. We're bending the laws of
> space and time.
>
> We might need to ask the Savannah people what's going on...

This is weird. Is there any chance we can ask them to see what is going on?

I don't think it is on my side as everyone individually received them.

>
> --js
>
>
> P.S.: Alistair, if you do resend from a different address, please attach
> my Acked-by to patches 1 & 2. Sorry for the huge hassle here, I have no
> idea why the list is exploding.

Sorry about dropping the ACKs, I just re-sent the patches as they
originally were.

I had a go at sending through my Gmail and couldn't get it to work. If
someone wants a patch resend next week (it's a long weekend here in
the US) I will try again and hopefully remember your ACKs this time.

Thanks,

Alistair

>
>

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Peter Maydell]

On 29 August 2015 at 01:04, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Connect the Sysbus AHCI device to ZynqMP.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>

> +    object_property_set_int(OBJECT(&s->sata), SATA_NUM_PORTS, "num-ports",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->sata), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));

Weird unnecessary extra brackets. I'll just remove them...

-- PMM

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

[Peter Maydell]

On 4 September 2015 at 16:17, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> 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.

No idea why the list doesn't like patch 4, but anyway I've
applied the series to target-arm.next.

PS: You should send a patch to fix all the occurrences
of "error_propagate((errp), (err));"  in xlnx-zynqmp.c...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 4/4] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP

[Alistair Francis]

On Tue, Sep 8, 2015 at 7:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 August 2015 at 01:04, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Connect the Sysbus AHCI device to ZynqMP.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>
>
>> +    object_property_set_int(OBJECT(&s->sata), SATA_NUM_PORTS, "num-ports",
>> +                            &error_abort);
>> +    object_property_set_bool(OBJECT(&s->sata), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate((errp), (err));
>
> Weird unnecessary extra brackets. I'll just remove them...

Thanks Peter

Alistair

>
> -- PMM
>

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

[Alistair Francis]

On Tue, Sep 8, 2015 at 8:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 September 2015 at 16:17, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> 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.
>
> No idea why the list doesn't like patch 4, but anyway I've
> applied the series to target-arm.next.

Thanks for that Peter.

>
> PS: You should send a patch to fix all the occurrences
> of "error_propagate((errp), (err));"  in xlnx-zynqmp.c...

Thanks for pointing that out. I'm working on a patch now.

Thanks,

Alistair

>
> thanks
> -- PMM
>