xlnx-zynqmp: Connect the AHCI SATA device
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 object_class_dynamic_cast() to return NULL if the class doesn't have a type.
Alistair Francis (3):
ahci: Seperate the AHCI state structure into the header object.c: object_class_dynamic_cast return NULL if the class has no
type
xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP
hw/arm/xlnx-zynqmp.c | 20 ++++++++++++++++++++ hw/ide/ahci.c | 13 ------------- hw/ide/ahci.h | 14 ++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ qom/object.c | 2 +- 5 files changed, 38 insertions(+), 14 deletions(-)
PATCH 1/3 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
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 4 lines · 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.
Is there a reason this structure needs to be directly inlined into XlnxZynqMPState, instead of using e.g. qdev_create/object_property_add_child and letting SysbusAHCIState be the business of ahci.c?
Asking as a bit of a qdev outsider.
--js
Quoted 69 lines
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 */
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 8 lines · John Snow
Quoted 4 lines · 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.
Quoted 4 lines
Is there a reason this structure needs to be directly inlined into XlnxZynqMPState, instead of using e.g. qdev_create/object_property_add_child and letting SysbusAHCIState be the business of
ahci.c?
AFAIK there isn't a huge advantage, it ends up achieving basically the same thing. It just means that we don't need to use
the old qdev_* functions (as much). It also follows with QOMificatiom and the device creation can be split up over the init and realise functions.
This is also the way I did machine creation for the Netduino 2 and how it is done for the rest of the ZynqMP device.
Thanks,
Alistair
Quoted 75 lines
Asking as a bit of a qdev outsider.
--js
Quoted 69 lines
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 */
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 9 lines · Alistair Francis
Quoted 8 lines · John Snow
Quoted 4 lines · 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.
Quoted 5 lines
Quoted 4 lines
Is there a reason this structure needs to be directly inlined into XlnxZynqMPState, instead of using e.g. qdev_create/object_property_add_child and letting SysbusAHCIState be the business of
ahci.c?
This has come up a few times, and the conclusion was to do the inlining by preference. I vaguely remember PMM started an effort to build infrastructure to hide the internals from use by outsiders, but the struct def is desirable for the inlining even if the internals are private a different module. It has been the policy on ARM SoC for quite some time. We have made a quite a few of these conversions for an example of a big one, see:
[PATCH v4 00/24] arm: ARM11MPCore+A9MPCore+A15MPCore QOM'ification
Quoted 5 lines
AFAIK there isn't a huge advantage, it ends up achieving basically the same thing. It just means that we don't need to use
the old qdev_* functions (as much). It also follows with QOMificatiom and the device creation can be split up over the init and realise functions.
You can use object_new in place of object_initialize to achieve the split without inlining as well.
Regards,
Peter
Quoted 83 lines
This is also the way I did machine creation for the Netduino 2 and how it is done for the rest of the ZynqMP device.
Thanks,
Alistair
Quoted 75 lines
Asking as a bit of a qdev outsider.
--js
Quoted 69 lines
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 */
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 5 lines · Peter Crosthwaite
This has come up a few times, and the conclusion was to do the inlining by preference. I vaguely remember PMM started an effort to build infrastructure to hide the internals from use by outsiders, but the struct def is desirable for the inlining even if the internals are private a different module.
The idea was just to mark up the fields which are supposed to be private to the implementation, so everybody can use the same header, but outsiders get a warning/error if they try to touch things they shouldn't.
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01846.html
I should probably get back to that at some point -- Paolo had an interesting suggestion involving some cpp macros which I never got round to investigating.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
In reply to Alistair FrancisQuoted 47 lines · 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 <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 +0
-0
hw/ide/ahci.h line 28
Quoted 5 lines
hw/ide/ahci.h +2
-0@@ -24,6 +24,8 @@#ifndef HW_IDE_AHCI_H #define HW_IDE_AHCI_H +#include <hw/sysbus.h> +
Odd that this is the only header. Out-of scope, but should this header be including the same for PCI? It uses PCIDevice * defs, so I am guessing it is relying on clients to pre-include the deps.
Regards,
Peter
Quoted 24 lines
hw/ide/ahci.h +12
-1#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
Peter Crosthwaite
Reviewed-byRe: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 52 lines · Peter Crosthwaite
Quoted 50 lines · 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 <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 +0
-0@@ -24,6 +24,8 @@#ifndef HW_IDE_AHCI_H #define HW_IDE_AHCI_H
Quoted 9 lines
hw/ide/ahci.h line 28
Quoted 2 lines
hw/ide/ahci.h +2
-0+#include <hw/sysbus.h> +Odd that this is the only header. Out-of scope, but should this header be including the same for PCI? It uses PCIDevice * defs, so I am guessing it is relying on clients to pre-include the deps.
Reviewed-by Peter Crosthwaite <[email protected]>
Sorry old habit, try:
Quoted 27 lines
Regards,
PeterQuoted 24 lines
hw/ide/ahci.h +12
-1#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
Re: [Qemu-devel] [PATCH v1 1/3] ahci: Separate the AHCI state structure into the header
Quoted 61 lines · Peter Crosthwaite
Quoted 59 lines · Peter Crosthwaite
Quoted 47 lines · 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 <[email protected]>Reviewed-by Sai Pavan Boddu <[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@@ -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 +0
-0hw/ide/ahci.h line 28
Quoted 5 lines
hw/ide/ahci.h +2
-0@@ -24,6 +24,8 @@#ifndef HW_IDE_AHCI_H #define HW_IDE_AHCI_H +#include <hw/sysbus.h> +Odd that this is the only header. Out-of scope, but should this header be including the same for PCI? It uses PCIDevice * defs, so I am guessing it is relying on clients to pre-include the deps.
Quoted 6 lines
Quoted 2 lines
Reviewed-by Peter Crosthwaite <[email protected]>Sorry old habit, try:
Reviewed-by Peter Crosthwaite <[email protected]>
Thanks Peter,
Alistair
Quoted 29 lines
Quoted 28 lines
Regards,
PeterQuoted 24 lines
hw/ide/ahci.h +12
-1#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
PATCH 2/3 object.c: object_class_dynamic_cast return NULL if the class has no type
In reply to Alistair FrancisIf the ObjectClass has no type return NULL instead of trying to compare the type name.
--- qom/object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
qom/object.c +1 -1
@@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,TypeImpl *target_type;TypeImpl *type; -if (!class) { +if (!class || !class->type) {return NULL ; }
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
What was the issue?
Regards,
Peter
Quoted 23 lines
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[email protected]>--- qom/object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)qom/object.c +1
-2@@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,TypeImpl *target_type;TypeImpl *type; -if (!class) { +if (!class || !class->type) {return NULL ; } --1.7.1
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 7 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
What was the issue?
There is a seg fault in object_class_dynamic_cast() because there is no type in the ObjectClass struct.
It happens when it is trying to cast the "pci-device", which is called from the ahci_irq_lower() function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.
Thanks,
Alistair
Quoted 28 lines
Regards,
PeterQuoted 23 lines
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[email protected]>--- qom/object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)qom/object.c +1
-2@@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,TypeImpl *target_type;TypeImpl *type; -if (!class) { +if (!class || !class->type) {return NULL ; } --1.7.1
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 4 lines
Quoted 2 lines · Peter Crosthwaite
On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
If the ObjectClass has no type return NULL instead of trying to compare the type name.
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.
That should never happen, ever since TYPE_OBJECT is no longer NULL.
Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Thanks,
Andreas
Signature · 2 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.
Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
If Andreas' idea don't work this is still a core QOM bug though. I think object_dynamic_cast should not have this segfault when passed a non implementing object.
Regards,
Peter
Quoted 14 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMU
The image I'm using is available at: http://1drv.ms/1NxDXLo
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 58 lines · Alistair Francis
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
Quoted 9 lines
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMUThe image I'm using is available at: http://1drv.ms/1NxDXLo
So it's not a core bug. That container_of in ahci_lower_irq is incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address.
The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The container_of is replaced with a nav of this pointer and then the conditional PCI cast can work.
Regards,
Peter
Quoted 12 lines
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 76 lines · Peter Crosthwaite
Quoted 58 lines · Alistair Francis
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
Quoted 9 lines
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMUThe image I'm using is available at: http://1drv.ms/1NxDXLo
So it's not a core bug. That
container_ofinahci_lower_irqis incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address.
Quoted 4 lines
The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The
container_ofis replaced with a nav of this pointer and then the conditional PCI cast can work.
This seems to fix the problem. It seems hacky though, I can't find a better way to check the validity of the PCIDevice. Any ideas?
hw/ide/ahci.c +5 -2
@@ -137,8 +137,11 @@ 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 ); +PCIDevice *pci_dev =NULL ; + +if (s->parent_obj) { + pci_dev =PCI_DEVICE (d); + }DPRINTF (0 ,"lower irq\n" );
hw/ide/ahci.h +2 -0
@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *parent_obj; + AHCIDevice *dev; AHCIControlRegs control_regs; MemoryRegion mem;
Thanks,
Alistair
Quoted 17 lines
Regards,
PeterQuoted 12 lines
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 78 lines · Alistair Francis
Quoted 76 lines · Peter Crosthwaite
Quoted 58 lines · Alistair Francis
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
Quoted 9 lines
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMUThe image I'm using is available at: http://1drv.ms/1NxDXLo
So it's not a core bug. That
container_ofinahci_lower_irqis incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address.
Quoted 6 lines
Quoted 4 lines
The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The
container_ofis replaced with a nav of this pointer and then the conditional PCI cast can work.This seems to fix the problem.
I assume you have the appropriate setters for the new variable elsewhere in the code as well?
Quoted 3 lines
It seems hacky though, I can't find a
better way to check the validity of the PCIDevice. Any ideas?
So there a few problems in the way of a correct solution. The caller for ahci_lower_irq does not have access to the QOM object pointer, it's been abstracted away by AHCIState (which is not a QOM object). So you would need to replumb the call path to ahci_lower_irq to pass the QOM object. This would let you drop the container_of completely.
The next step would be to virtualise ahci_lower_irq, as this is implementation dependent (assume specific devices really do need to control the use of PCI MSI?), one implementation for sysbus, one for PCI. This is blocked by the re-plumbing described above as the virtualised called itself will need a ptr to the QOM object.
But I think the back ptr is an acceptable solution for the meantime, this is a clear bug in Sysbus AHCI and should probably even go to qemu-stable.
hw/ide/ahci.c static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev) · line 142
Quoted 13 lines
hw/ide/ahci.c +3
-2@@ -137,8 +137,11 @@ 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 ); +PCIDevice *pci_dev =NULL ; + +if (s->parent_obj) {
I would make the parent obj compulsory for all AHCIState implementations and drop the NULL guard.
Quoted 10 lines
hw/ide/ahci.c +2
-0+ pci_dev =PCI_DEVICE (d); + }DPRINTF (0 ,"lower irq\n" );hw/ide/ahci.h +0
-0
hw/ide/ahci.h struct AHCIDevice { · line 290
Quoted 4 lines
hw/ide/ahci.h +1
-0@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *parent_obj;
This name is really for QOM inline parents. We decided a while back to use "parent" for the QOM parents and "container" for non-parental containers. Memory regions use the .container field for a similar purpose.
Regards,
Peter
Quoted 26 lines
hw/ide/ahci.h +1
-0+AHCIDevice *dev;AHCIControlRegs control_regs;MemoryRegion mem;Thanks,
Alistair
Quoted 17 lines
Regards,
PeterQuoted 12 lines
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 106 lines · Peter Crosthwaite
Quoted 78 lines · Alistair Francis
Quoted 76 lines · Peter Crosthwaite
Quoted 58 lines · Alistair Francis
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
Quoted 9 lines
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMUThe image I'm using is available at: http://1drv.ms/1NxDXLo
So it's not a core bug. That
container_ofinahci_lower_irqis incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address.Quoted 6 lines
Quoted 4 lines
The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The
container_ofis replaced with a nav of this pointer and then the conditional PCI cast can work.This seems to fix the problem.
I assume you have the appropriate setters for the new variable elsewhere in the code as well?
Quoted 3 lines
It seems hacky though, I can't find a
better way to check the validity of the PCIDevice. Any ideas?So there a few problems in the way of a correct solution. The caller for
ahci_lower_irqdoes not have access to the QOM object pointer, it's been abstracted away by AHCIState (which is not a QOM object). So you would need to replumb the call path toahci_lower_irqto pass the QOM object. This would let you drop thecontainer_ofcompletely.The next step would be to virtualise
ahci_lower_irq, as this is implementation dependent (assume specific devices really do need to control the use of PCI MSI?), one implementation for sysbus, one for PCI. This is blocked by the re-plumbing described above as the virtualised called itself will need a ptr to the QOM object.
Quoted 4 lines
But I think the back ptr is an acceptable solution for the meantime, this is a clear bug in Sysbus AHCI and should probably even go to qemu-stable.
I'm not intricately familiar with how the QOM plumbing works, but I can definitely see how assuming all AHCIState pointers come from AHCIPCIState is a problem...
For the uninitiated, how does MSI work with Sysbus? What does a Sysbus AHCI device look like to a guest, and what happens if it tries to utilize the functionality?
(Well, segfault, I guess.)
If someone wants to clue in the device model newbie and send a patch my way, I'll take it.
--js
Quoted 66 lines
hw/ide/ahci.c static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev) · line 142
Quoted 13 lines
hw/ide/ahci.c +3
-2@@ -137,8 +137,11 @@ 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 ); +PCIDevice *pci_dev =NULL ; + +if (s->parent_obj) {I would make the parent obj compulsory for all AHCIState implementations and drop the NULL guard.
Quoted 10 lines
hw/ide/ahci.c +2
-0+ pci_dev =PCI_DEVICE (d); + }DPRINTF (0 ,"lower irq\n" );hw/ide/ahci.h +0
-0hw/ide/ahci.h struct AHCIDevice { · line 290
Quoted 4 lines
hw/ide/ahci.h +1
-0@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *parent_obj;This name is really for QOM inline parents. We decided a while back to use "parent" for the QOM parents and "container" for non-parental containers. Memory regions use the .container field for a similar purpose.
Regards,
PeterQuoted 26 lines
hw/ide/ahci.h +1
-0+AHCIDevice *dev;AHCIControlRegs control_regs;MemoryRegion mem;Thanks,
Alistair
Quoted 17 lines
Regards,
PeterQuoted 12 lines
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 114 lines · John Snow
Quoted 106 lines · Peter Crosthwaite
Quoted 78 lines · Alistair Francis
Quoted 76 lines · Peter Crosthwaite
Quoted 58 lines · Alistair Francis
Quoted 32 lines · Peter Crosthwaite
Quoted 9 lines · Andreas Färber
Am 18.08.2015 um 00:24 schrieb Alistair Francis:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
Quoted 3 lines · Alistair Francis
If the ObjectClass has no type return NULL instead of trying to compare the type name.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
What was the issue?
There is a seg fault in
object_class_dynamic_cast()because there is no type in the ObjectClass struct.That should never happen, ever since
TYPE_OBJECTis no longer NULL.Quoted 3 lines
It happens when it is trying to cast the "pci-device", which is called from the
ahci_irq_lower()function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid.Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook?
On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers?
Quoted 14 lines
If Andreas' idea don't work this is still a core QOM bug though. I think
object_dynamic_castshould not have this segfault when passed a non implementing object.Regards,
PeterQuoted 7 lines
It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this.
Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug.
Hey,
Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault.
I will try to look into it more this week, but if anyone is interested here are the steps to reproduce:
On the latest mainline QEMU, with my 2nd and 3rd patches applied
Quoted 9 lines
$ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMUThe image I'm using is available at: http://1drv.ms/1NxDXLo
So it's not a core bug. That
container_ofinahci_lower_irqis incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address.Quoted 6 lines
Quoted 4 lines
The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The
container_ofis replaced with a nav of this pointer and then the conditional PCI cast can work.This seems to fix the problem.
I assume you have the appropriate setters for the new variable elsewhere in the code as well?
Quoted 3 lines
It seems hacky though, I can't find a
better way to check the validity of the PCIDevice. Any ideas?So there a few problems in the way of a correct solution. The caller for
ahci_lower_irqdoes not have access to the QOM object pointer, it's been abstracted away by AHCIState (which is not a QOM object). So you would need to replumb the call path toahci_lower_irqto pass the QOM object. This would let you drop thecontainer_ofcompletely.The next step would be to virtualise
ahci_lower_irq, as this is implementation dependent (assume specific devices really do need to control the use of PCI MSI?), one implementation for sysbus, one for PCI. This is blocked by the re-plumbing described above as the virtualised called itself will need a ptr to the QOM object.Quoted 4 lines
But I think the back ptr is an acceptable solution for the meantime, this is a clear bug in Sysbus AHCI and should probably even go to qemu-stable.
Quoted 5 lines
I'm not intricately familiar with how the QOM plumbing works, but I can definitely see how assuming all AHCIState pointers come from AHCIPCIState is a problem...
For the uninitiated, how does MSI work with Sysbus?
No such thing :) Interrupts in Sysbus shouldn't really exist and those that do are just a thin wrapper around raw pins. The short answer is MSI is a PCI specific concept and sysbus is an alternative to PCI. Long answer, is any form of MSI requires some sort of transport layer capable of messaging an interrupt controller from a device with the device as the initiator. This is not possible in most real busses which we use the sysbus abstraction for.
Quoted 2 lines
What does a Sysbus
AHCI device look like to a guest, and what happens if it tries to
Sysbus AHCI is basically the raw MMIO regions mapped onto the "system bus" by the machine model. Usually this is done as static mapping. The most common user of this is ARM (and other embedded) SoCs. The downside is there is no standard for remapping the device or self identification of the device (as provided by PCI standard). The existance of the device is made known to a kernel usually by a device tree blob.
In the Xilinx MPSoC case, the AHCI controller is on the SoC (with the ARM processor and many other IO peripherals). There are no cards or even inter-chip connectivites in Alistairs AHCI connection.
Quoted 2 lines
utilize the functionality?
If you rework an exsiting PCI driver to use raw MMIO regions and interrupts rather than bars and MSIs it usually works with minor edits only. SDHCI, AHCI, EHCI, xHCI at least all have both PCI implementations and real world sysbus implementations.
What I would be interested in, is it possible to push all MSI code up to the PCI core to completely remove the need for all PCIisms in AHCI? This seems to be the only thing that is PCI specific.
Regards,
Peter
Quoted 74 lines
(Well, segfault, I guess.)
If someone wants to clue in the device model newbie and send a patch my way, I'll take it.
--js
Quoted 66 lines
hw/ide/ahci.c static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev) · line 142
Quoted 13 lines
hw/ide/ahci.c +3
-2@@ -137,8 +137,11 @@ 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 ); +PCIDevice *pci_dev =NULL ; + +if (s->parent_obj) {I would make the parent obj compulsory for all AHCIState implementations and drop the NULL guard.
Quoted 10 lines
hw/ide/ahci.c +2
-0+ pci_dev =PCI_DEVICE (d); + }DPRINTF (0 ,"lower irq\n" );hw/ide/ahci.h +0
-0hw/ide/ahci.h struct AHCIDevice { · line 290
Quoted 4 lines
hw/ide/ahci.h +1
-0@@ -287,6 +287,8 @@ struct AHCIDevice { }; typedef struct AHCIState { + DeviceState *parent_obj;This name is really for QOM inline parents. We decided a while back to use "parent" for the QOM parents and "container" for non-parental containers. Memory regions use the .container field for a similar purpose.
Regards,
PeterQuoted 26 lines
hw/ide/ahci.h +1
-0+AHCIDevice *dev;AHCIControlRegs control_regs;MemoryRegion mem;Thanks,
Alistair
Quoted 17 lines
Regards,
PeterQuoted 12 lines
Thanks,
Alistair
Quoted 8 lines
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 6 lines · Peter Crosthwaite
Quoted 1 line · John Snow
For the uninitiated, how does MSI work with Sysbus?
No such thing :) Interrupts in Sysbus shouldn't really exist and those that do are just a thin wrapper around raw pins. The short answer is MSI is a PCI specific concept and sysbus is an alternative to PCI.
My preferred way to think about this is that the 'sysbus' device is the raw "whatever-it-is chip" (uart, usb controller, whatever), and you can wire that chip up directly on a board (sysbus), or wire it up to a pci bus. (Yeah, in practice the PCI functionality is probably on the same bit of silicon, but as a model it works more or less.)
You can see something similar going on in our uart models:
we have a pci-serial, an isa-serial and some embedded UARTs which all use the same core serial code but present a different interface to the rest of the system.
Quoted 3 lines
What I would be interested in, is it possible to push all MSI code up to the PCI core to completely remove the need for all PCIisms in AHCI? This seems to be the only thing that is PCI specific.
Possibly it might have to live in a pci-ahci wrapper class, but it ought to be possible to keep it out of the common ahci code I hope.
-- PMM
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
In reply to Peter CrosthwaiteQuoted 4 lines · Peter Crosthwaite
Quoted 3 lines · John Snow
On 08/26/2015 05:02 PM, Peter Crosthwaite wrote:
[snip]
Quoted 24 lines
Quoted 18 lines
Quoted 13 lines
So there a few problems in the way of a correct solution. The caller for
ahci_lower_irqdoes not have access to the QOM object pointer, it's been abstracted away by AHCIState (which is not a QOM object). So you would need to replumb the call path toahci_lower_irqto pass the QOM object. This would let you drop thecontainer_ofcompletely.The next step would be to virtualise
ahci_lower_irq, as this is implementation dependent (assume specific devices really do need to control the use of PCI MSI?), one implementation for sysbus, one for PCI. This is blocked by the re-plumbing described above as the virtualised called itself will need a ptr to the QOM object.Quoted 4 lines
But I think the back ptr is an acceptable solution for the meantime, this is a clear bug in Sysbus AHCI and should probably even go to qemu-stable.
Quoted 5 lines
I'm not intricately familiar with how the QOM plumbing works, but I can definitely see how assuming all AHCIState pointers come from AHCIPCIState is a problem...
For the uninitiated, how does MSI work with Sysbus?
Quoted 8 lines
No such thing :) Interrupts in Sysbus shouldn't really exist and those that do are just a thin wrapper around raw pins. The short answer is MSI is a PCI specific concept and sysbus is an alternative to PCI. Long answer, is any form of MSI requires some sort of transport layer capable of messaging an interrupt controller from a device with the device as the initiator. This is not possible in most real busses which we use the sysbus abstraction for.
Ah, OK ... I seemed to recall there being some MSI bits in the HBA registers, so I was confused. However, I think it's just a read-only status bit, which allows it to just always be off for your device.
Quoted 18 lines
Quoted 2 lines
What does a Sysbus
AHCI device look like to a guest, and what happens if it tries toSysbus AHCI is basically the raw MMIO regions mapped onto the "system bus" by the machine model. Usually this is done as static mapping. The most common user of this is ARM (and other embedded) SoCs. The downside is there is no standard for remapping the device or self identification of the device (as provided by PCI standard). The existance of the device is made known to a kernel usually by a device tree blob.
In the Xilinx MPSoC case, the AHCI controller is on the SoC (with the ARM processor and many other IO peripherals). There are no cards or even inter-chip connectivites in Alistairs AHCI connection.
Quoted 2 lines
utilize the functionality?
Quoted 8 lines
If you rework an exsiting PCI driver to use raw MMIO regions and interrupts rather than bars and MSIs it usually works with minor edits only. SDHCI, AHCI, EHCI, xHCI at least all have both PCI implementations and real world sysbus implementations.
What I would be interested in, is it possible to push all MSI code up to the PCI core to completely remove the need for all PCIisms in AHCI? This seems to be the only thing that is PCI specific.
Fields that reference MSI in the AHCI spec are:
GHC.MRSM (MSI Revert To Single Message, read-only, is 0 when MSI is off) CCC_CTL.INT (Advertises MSI vector)
You're save leaving both of these to zero, so I think there's nothing else PCI-specific in the HBA region.
Anyway, apologies for not noticing this while I was reworking the AHCI device!
Quoted 11 lines
Regards,
PeterQuoted 7 lines
(Well, segfault, I guess.)
If someone wants to clue in the device model newbie and send a patch my way, I'll take it.
--js
[snip]
Re: [Qemu-devel] [PATCH v1 2/3] object.c: object_class_dynamic_cast return NULL if the class has no type
Quoted 8 lines · John Snow
Quoted 4 lines · Peter Crosthwaite
Quoted 3 lines · John Snow
On 08/26/2015 05:02 PM, Peter Crosthwaite wrote:
Quoted 8 lines
[snip]
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
So there a few problems in the way of a correct solution. The caller for
ahci_lower_irqdoes not have access to the QOM object pointer, it's been abstracted away by AHCIState (which is not a QOM object). So you would need to replumb the call path toahci_lower_irqto pass the QOM object. This would let you drop thecontainer_ofcompletely.
Ok, I think I have figured it out. It took me a while to get my head around what is going on here. I'm just testing then I'll send a new version of the series. Thanks for your help Peter.
Thanks,
Alistair
Quoted 83 lines
Quoted 18 lines
Quoted 12 lines
Quoted 11 lines
The next step would be to virtualise
ahci_lower_irq, as this is implementation dependent (assume specific devices really do need to control the use of PCI MSI?), one implementation for sysbus, one for PCI. This is blocked by the re-plumbing described above as the virtualised called itself will need a ptr to the QOM object.But I think the back ptr is an acceptable solution for the meantime, this is a clear bug in Sysbus AHCI and should probably even go to qemu-stable.
Quoted 5 lines
I'm not intricately familiar with how the QOM plumbing works, but I can definitely see how assuming all AHCIState pointers come from AHCIPCIState is a problem...
For the uninitiated, how does MSI work with Sysbus?
Quoted 8 lines
No such thing :) Interrupts in Sysbus shouldn't really exist and those that do are just a thin wrapper around raw pins. The short answer is MSI is a PCI specific concept and sysbus is an alternative to PCI. Long answer, is any form of MSI requires some sort of transport layer capable of messaging an interrupt controller from a device with the device as the initiator. This is not possible in most real busses which we use the sysbus abstraction for.
Ah, OK ... I seemed to recall there being some MSI bits in the HBA registers, so I was confused. However, I think it's just a read-only status bit, which allows it to just always be off for your device.
Quoted 18 lines
Quoted 2 lines
What does a Sysbus
AHCI device look like to a guest, and what happens if it tries toSysbus AHCI is basically the raw MMIO regions mapped onto the "system bus" by the machine model. Usually this is done as static mapping. The most common user of this is ARM (and other embedded) SoCs. The downside is there is no standard for remapping the device or self identification of the device (as provided by PCI standard). The existance of the device is made known to a kernel usually by a device tree blob.
In the Xilinx MPSoC case, the AHCI controller is on the SoC (with the ARM processor and many other IO peripherals). There are no cards or even inter-chip connectivites in Alistairs AHCI connection.
Quoted 2 lines
utilize the functionality?
Quoted 8 lines
If you rework an exsiting PCI driver to use raw MMIO regions and interrupts rather than bars and MSIs it usually works with minor edits only. SDHCI, AHCI, EHCI, xHCI at least all have both PCI implementations and real world sysbus implementations.
What I would be interested in, is it possible to push all MSI code up to the PCI core to completely remove the need for all PCIisms in AHCI? This seems to be the only thing that is PCI specific.
Fields that reference MSI in the AHCI spec are:
GHC.MRSM (MSI Revert To Single Message, read-only, is 0 when MSI is off) CCC_CTL.INT (Advertises MSI vector)
You're save leaving both of these to zero, so I think there's nothing else PCI-specific in the HBA region.
Anyway, apologies for not noticing this while I was reworking the AHCI device!
Quoted 11 lines
Regards,
PeterQuoted 7 lines
(Well, segfault, I guess.)
If someone wants to clue in the device model newbie and send a patch my way, I'll take it.
--js
[snip]
PATCH 3/3 xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP
In reply to Alistair FrancisConnect the Sysbus AHCI device to ZynqMP.
--- hw/arm/xlnx-zynqmp.c | 20 ++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ 2 files changed, 23 insertions(+), 0 deletions(-)
hw/arm/xlnx-zynqmp.c +20 -0
@@ -28,6 +28,9 @@#define GIC_DIST_ADDR 0xf9010000#define GIC_CPU_ADDR 0xf9020000 +#define SYSBUS_AHCI_ADDR 0xFD0C0000 +#define SYSBUS_AHCI_INTR 133 +static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS ] = {0xFF0B0000 ,0xFF0C0000 ,0xFF0D0000 ,0xFF0E0000 , };
@@ -90,6 +93,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)
@@ -235,6 +241,20 @@ 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),2 ,"num-ports" , &err); +if (err) { +error_propagate ((errp), (err)); +return ; + } +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 ,SYSBUS_AHCI_ADDR ); +sysbus_connect_irq (SYS_BUS_DEVICE (&s->sata),0 , gic_spi[SYSBUS_AHCI_INTR ]); }static Property xlnx_zynqmp_props[] = {
include/hw/arm/xlnx-zynqmp.h +3 -0
@@ -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), \
@@ -54,6 +56,7 @@ typedef struct XlnxZynqMPState {MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS ][XLNX_ZYNQMP_GIC_ALIASES ];CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS ];CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS ]; +SysbusAHCIState sata;char *boot_cpu;ARMCPU *boot_cpu_ptr;
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v1 3/3] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP
Quoted 34 lines · Alistair Francis
Connect the Sysbus AHCI device to ZynqMP.
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[email protected]>--- hw/arm/xlnx-zynqmp.c | 20 ++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ 2 files changed, 23 insertions(+), 0 deletions(-)hw/arm/xlnx-zynqmp.c +6
-0@@ -28,6 +28,9 @@#define GIC_DIST_ADDR 0xf9010000#define GIC_CPU_ADDR 0xf9020000 +#define SYSBUS_AHCI_ADDR 0xFD0C0000 +#define SYSBUS_AHCI_INTR 133 +static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS ] = {0xFF0B0000 ,0xFF0C0000 ,0xFF0D0000 ,0xFF0E0000 , };@@ -90,6 +93,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)
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 245
Quoted 5 lines
hw/arm/xlnx-zynqmp.c +2
-0@@ -235,6 +241,20 @@ 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),2 ,"num-ports" , &err);
2 Should get a macro up top.
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 249
Quoted 4 lines
hw/arm/xlnx-zynqmp.c +4
-0+if (err) { +error_propagate ((errp), (err)); +return ; + }
What is the user visible failure case here? I think this can be error_abort'ed. I think some of the others in the file (mainly the reset-cbar one) could be error_aborted as well.
Regards,
Peter
Quoted 36 lines
hw/arm/xlnx-zynqmp.c +8
-0+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 ,SYSBUS_AHCI_ADDR ); +sysbus_connect_irq (SYS_BUS_DEVICE (&s->sata),0 , gic_spi[SYSBUS_AHCI_INTR ]); }static Property xlnx_zynqmp_props[] = {include/hw/arm/xlnx-zynqmp.h +3
-1@@ -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), \@@ -54,6 +56,7 @@ typedef struct XlnxZynqMPState {MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS ][XLNX_ZYNQMP_GIC_ALIASES ];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 v1 3/3] xlnx-zynqmp: Connect the sysbus AHCI to ZynqMP
Quoted 34 lines · Peter Crosthwaite
Quoted 32 lines · Alistair Francis
Connect the Sysbus AHCI device to ZynqMP.
Signed-off-by Alistair Francis <[email protected]>Reviewed-by Sai Pavan Boddu <[email protected]>--- hw/arm/xlnx-zynqmp.c | 20 ++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ 2 files changed, 23 insertions(+), 0 deletions(-)hw/arm/xlnx-zynqmp.c +6
-0@@ -28,6 +28,9 @@#define GIC_DIST_ADDR 0xf9010000#define GIC_CPU_ADDR 0xf9020000 +#define SYSBUS_AHCI_ADDR 0xFD0C0000 +#define SYSBUS_AHCI_INTR 133 +static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS ] = {0xFF0B0000 ,0xFF0C0000 ,0xFF0D0000 ,0xFF0E0000 , };@@ -90,6 +93,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 ()); }
Quoted 9 lines
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 245
Quoted 7 lines
hw/arm/xlnx-zynqmp.c +2
-0static void xlnx_zynqmp_realize (DeviceState *dev,Error **errp)@@ -235,6 +241,20 @@ 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),2 ,"num-ports" , &err);2 Should get a macro up top.
Ok, will fix
Quoted 9 lines
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 249
Quoted 5 lines
hw/arm/xlnx-zynqmp.c +4
-0+if (err) { +error_propagate ((errp), (err)); +return ; + }What is the user visible failure case here? I think this can be error_abort'ed. I think some of the others in the file (mainly the reset-cbar one) could be
error_abortedas well.
Fair point. I was just following what most of the others do. Thinking about it I agree that pretty much all of the non-realise calls should just abort. I'll fix this and the others in the next version.
Thanks,
Alistair
Quoted 41 lines
Regards,
PeterQuoted 36 lines
hw/arm/xlnx-zynqmp.c +8
-0+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 ,SYSBUS_AHCI_ADDR ); +sysbus_connect_irq (SYS_BUS_DEVICE (&s->sata),0 , gic_spi[SYSBUS_AHCI_INTR ]); }static Property xlnx_zynqmp_props[] = {include/hw/arm/xlnx-zynqmp.h +3
-1@@ -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), \@@ -54,6 +56,7 @@ typedef struct XlnxZynqMPState {MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS ][XLNX_ZYNQMP_GIC_ALIASES ];CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS ];CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS ]; +SysbusAHCIState sata;char *boot_cpu;ARMCPU *boot_cpu_ptr; --1.7.1
Quoted 5 lines · 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
object_class_dynamic_cast()to return NULL if the class doesn't have a type.
Ping!
Not a huge rush, this was probably lost as I sent it during the freeze.
Thanks,
Alistair
Quoted 15 lines
Alistair Francis (3):
ahci: Seperate the AHCI state structure into the headerobject.c:object_class_dynamic_castreturn NULL if the class has no
type
xlnx-zynqmp: Connect the sysbus AHCI to ZynqMPhw/arm/xlnx-zynqmp.c | 20 ++++++++++++++++++++ hw/ide/ahci.c | 13 ------------- hw/ide/ahci.h | 14 ++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ qom/object.c | 2 +- 5 files changed, 38 insertions(+), 14 deletions(-)