xlnx-zynqmp: Connect the AHCI SATA device
v3 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.
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 | 42 +++++++++++++++++++++++------------------- hw/ide/ahci.h | 16 ++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ 4 files changed, 63 insertions(+), 31 deletions(-)
PATCH v3 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.
---
hw/ide/ahci.c | 13 ------------- hw/ide/ahci.h | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 13 deletions(-)
hw/ide/ahci.c +0 -13
@@ -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
@@ -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
PATCH v3 2/4 ahci.c: Don't assume AHCIState's parent is AHCIPCIState
In reply to Alistair FrancisThe AHCIState strucut 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.
---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)
hw/ide/ahci.c +23 -6
@@ -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 ); +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 ,"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,
hw/ide/ahci.h +2 -0
@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *container; + AHCIDevice *dev; AHCIControlRegs control_regs; MemoryRegion mem;
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState
Quoted 1 line · Alistair Francis
The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
"struct"
Quoted 17 lines
as a parent. The
ahci_irq_lower()andahci_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]>---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)hw/ide/ahci.c +0
-0
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 126
Quoted 6 lines
hw/ide/ahci.c +0
-3@@ -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? ...
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 124
Quoted 1 line
hw/ide/ahci.c +1
-0+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.
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 130
Quoted 6 lines
hw/ide/ahci.c +6
-0+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?
Regards,
Peter
Quoted 52 lines
hw/ide/ahci.c +16
-3+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,hw/ide/ahci.h +2
-1@@ -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 v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState
Quoted 5 lines · Peter Crosthwaite
Quoted 1 line · Alistair Francis
The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
"struct"
Good catch, will fix
Quoted 27 lines
Quoted 18 lines
as a parent. The
ahci_irq_lower()andahci_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]>---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)hw/ide/ahci.c +0
-0hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 126
Quoted 6 lines
hw/ide/ahci.c +0
-3@@ -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? ...
Quoted 4 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 124
Quoted 1 line
hw/ide/ahci.c +1
-0+DeviceState *dev_state =DEVICE (s->container);Unneeded cast.
s->containeris 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).
Quoted 10 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 130
Quoted 7 lines
hw/ide/ahci.c +6
-0+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.
Thanks,
Alistair
Quoted 57 lines
Regards,
PeterQuoted 52 lines
hw/ide/ahci.c +16
-3+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,hw/ide/ahci.h +2
-1@@ -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 v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState
Quoted 53 lines · Alistair Francis
Quoted 5 lines · Peter Crosthwaite
Quoted 1 line · Alistair Francis
The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
"struct"
Good catch, will fix
Quoted 27 lines
Quoted 18 lines
as a parent. The
ahci_irq_lower()andahci_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]>---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)hw/ide/ahci.c +0
-0hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 126
Quoted 6 lines
hw/ide/ahci.c +0
-3@@ -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? ...Quoted 4 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 124
Quoted 1 line
hw/ide/ahci.c +1
-0+DeviceState *dev_state =DEVICE (s->container);Unneeded cast.
s->containeris 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).
Quoted 8 lines
Quoted 7 lines
hw/ide/ahci.c +6
-0+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 );
Quoted 6 lines
Quoted 2 lines
... 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?
Regards,
Peter
Quoted 61 lines
Thanks,
Alistair
Quoted 57 lines
Regards,
PeterQuoted 52 lines
hw/ide/ahci.c +16
-3+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,hw/ide/ahci.h +2
-1@@ -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 v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState
()
Quoted 58 lines · Peter Crosthwaite
Quoted 56 lines · Alistair Francis
Quoted 5 lines · Peter Crosthwaite
Quoted 1 line · Alistair Francis
The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
"struct"
Good catch, will fix
Quoted 27 lines
Quoted 18 lines
as a parent. The
ahci_irq_lower()andahci_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]>---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)hw/ide/ahci.c +0
-0hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 126
Quoted 6 lines
hw/ide/ahci.c +0
-3@@ -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? ...Quoted 4 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 124
Quoted 1 line
hw/ide/ahci.c +1
-0+DeviceState *dev_state =DEVICE (s->container);Unneeded cast.
s->containeris 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).
Quoted 10 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 130
Quoted 7 lines
hw/ide/ahci.c +6
-0+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?
Quoted 6 lines
Quoted 3 lines
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_castwhich 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
Quoted 66 lines
Regards,
PeterQuoted 61 lines
Thanks,
Alistair
Quoted 57 lines
Regards,
PeterQuoted 52 lines
hw/ide/ahci.c +16
-3+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,hw/ide/ahci.h +2
-1@@ -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 v3 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState
Quoted 66 lines · Alistair Francis
()
Quoted 62 lines · Peter Crosthwaite
Quoted 59 lines · Alistair Francis
Quoted 5 lines · Peter Crosthwaite
Quoted 1 line · Alistair Francis
The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
"struct"
Good catch, will fix
Quoted 27 lines
Quoted 18 lines
as a parent. The
ahci_irq_lower()andahci_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]>---
hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)hw/ide/ahci.c +0
-0hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 126
Quoted 6 lines
hw/ide/ahci.c +0
-3@@ -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? ...Quoted 4 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 124
Quoted 1 line
hw/ide/ahci.c +1
-0+DeviceState *dev_state =DEVICE (s->container);Unneeded cast.
s->containeris 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).
Quoted 10 lines
hw/ide/ahci.c static uint32_t ahci_port_read(AHCIState *s, int port, int offset) · line 130
Quoted 7 lines
hw/ide/ahci.c +6
-0+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.
Quoted 8 lines
Quoted 2 lines
But the original code uses
object_dynamic_castwhich 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.
Nothing more from me :)
Regards,
Peter
Quoted 70 lines
Thanks,
Alistair
Quoted 66 lines
Regards,
PeterQuoted 61 lines
Thanks,
Alistair
Quoted 57 lines
Regards,
PeterQuoted 52 lines
hw/ide/ahci.c +16
-3+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,hw/ide/ahci.h +2
-1@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *container; + AHCIDevice *dev; AHCIControlRegs control_regs; MemoryRegion mem; --1.7.1
PATCH v3 3/4 xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort
In reply to Alistair FrancisConvert all of the non-realize error_propagate() calls into error_abort calls as they shouldn't be user visible failure cases.
---
hw/arm/xlnx-zynqmp.c | 14 ++------------ 1 files changed, 2 insertions(+), 12 deletions(-)
hw/arm/xlnx-zynqmp.c +2 -12
@@ -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
Re: [Qemu-devel] [PATCH v3 3/4] xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort
Quoted 4 lines · Alistair Francis
Convert all of the non-realize
error_propagate()calls intoerror_abortcalls as they shouldn't be user visible failure cases.Signed-off-by Alistair Francis <[email protected]>
So normally these are needed to catch bad usages of -cpu to CPUs that don't have these props, but since ZynqMP doesn't allow -cpu these are promotable to fatal error.
Quoted 40 lines
---
hw/arm/xlnx-zynqmp.c | 14 ++------------ 1 files changed, 2 insertions(+), 12 deletions(-)hw/arm/xlnx-zynqmp.c +2
-13@@ -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 v3 3/4] xlnx-zynqmp.c: Convert some of the error_propagate() calls to error_abort
Quoted 5 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
Convert all of the non-realize
error_propagate()calls intoerror_abortcalls as they shouldn't be user visible failure cases.
Quoted 7 lines
Quoted 1 line
Signed-off-by Alistair Francis <[email protected]>So normally these are needed to catch bad usages of -cpu to CPUs that don't have these props, but since ZynqMP doesn't allow -cpu these are promotable to fatal error.
Reviewed-by Peter Crosthwaite <[email protected]>
Thanks Peter
Alistair
Quoted 42 lines
Quoted 41 lines
---
hw/arm/xlnx-zynqmp.c | 14 ++------------ 1 files changed, 2 insertions(+), 12 deletions(-)hw/arm/xlnx-zynqmp.c +2
-13@@ -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