Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Hi Anthony, Am 22.07.2013 22:29, schrieb Anthony Liguori: for consistency, I think having everything be relatively to *one* type for a Property list is pretty helpful. Expecting someone to know the type hierarchy by heart such that this doesn't look like a bug is too much IMHO. I have changed v2 not to mix different-but-compatible struct types in one VMStateDescription. Could you clarify if that was what you meant with the above? Or would you also be opposed to - post-1.6 - changing VMSTATE_PCIE_DEVICE(parent_obj[.parent_obj], MyStruct) to VMSTATE_PCIE_DEVICE() as suggested elsewhere in this thread? I'm thinking that writing VMSTATE_PCIE_DEVICE() already clearly indicates the developer knows the device inherits from TYPE_PCI_DEVICE. All PCIe devices using VMSTATE_PCIE_DEVICE() today use it at an offset of 0 and so do all PCI devices using VMSTATE_PCI_DEVICE() apparently. VMSTATE_PCI_DEVICE_POINTER() would be unaffected, but is unused anyway. My survey also concluded that luckily all VMSTATE_PCIE_DEVICE() and VMSTATE_PCI_DEVICE() are placed as first VMStateField, so moving parent state to its class might be possible, similar to qdev props todays with class_base_init clearing it for derived types. However this would require to either refactor core VMState code to operate on a list, aggregated from one or more zero-terminated arrays, which I would consider invasive and error-prone, or simply have Device code allocate a new VMStateDescription before registering it in QOM realize (so it can be free'd on unrealize). Thoughts? Either way, it would work for CPU but not for PCI, since there are two different macros, VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() both for PCIDeviceClass. Not sure how to solve that without multi-inheritence. SHPC_VMSTATE() seems to be another macro beyond VMSTATE_MSIX() operating on PCIDevice but placed in an individual device (pci-bridge-dev). Can it be turned into a subsection, Michael? Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Sun, Jul 28, 2013 at 02:36:33PM +0200, Andreas Färber wrote: Hi Anthony, Am 22.07.2013 22:29, schrieb Anthony Liguori: for consistency, I think having everything be relatively to *one* type for a Property list is pretty helpful. Expecting someone to know the type hierarchy by heart such that this doesn't look like a bug is too much IMHO. I have changed v2 not to mix different-but-compatible struct types in one VMStateDescription. Could you clarify if that was what you meant with the above? Or would you also be opposed to - post-1.6 - changing VMSTATE_PCIE_DEVICE(parent_obj[.parent_obj], MyStruct) to VMSTATE_PCIE_DEVICE() as suggested elsewhere in this thread? I'm thinking that writing VMSTATE_PCIE_DEVICE() already clearly indicates the developer knows the device inherits from TYPE_PCI_DEVICE. All PCIe devices using VMSTATE_PCIE_DEVICE() today use it at an offset of 0 and so do all PCI devices using VMSTATE_PCI_DEVICE() apparently. VMSTATE_PCI_DEVICE_POINTER() would be unaffected, but is unused anyway. My survey also concluded that luckily all VMSTATE_PCIE_DEVICE() and VMSTATE_PCI_DEVICE() are placed as first VMStateField, so moving parent state to its class might be possible, similar to qdev props todays with class_base_init clearing it for derived types. However this would require to either refactor core VMState code to operate on a list, aggregated from one or more zero-terminated arrays, which I would consider invasive and error-prone, or simply have Device code allocate a new VMStateDescription before registering it in QOM realize (so it can be free'd on unrealize). Thoughts? Either way, it would work for CPU but not for PCI, since there are two different macros, VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() both for PCIDeviceClass. Not sure how to solve that without multi-inheritence. Maybe combine them and use is_express to select the correct format. SHPC_VMSTATE() seems to be another macro beyond VMSTATE_MSIX() operating on PCIDevice but placed in an individual device (pci-bridge-dev). Can it be turned into a subsection, Michael? Regards, Andreas Not without breaking cross-version migration I think? -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Mon, Jul 22, 2013 at 03:29:46PM -0500, Anthony Liguori wrote:
Michael S. Tsirkin m...@redhat.com writes:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
What cast?
VMstate takes a void *.
One an object is cast to a void *, it's just as much as PCIESlot as it
is a PCIEPort.
That said, for consistency, I think having everything be relatively to
*one* type for a Property list is pretty helpful.
Expecting someone to know the type hierarchy by heart such that this
doesn't look like a bug is too much IMHO.
Regards,
Anthony Liguori
Exactly.
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote:
Am 22.07.2013 22:29, schrieb Anthony Liguori:
Michael S. Tsirkin m...@redhat.com writes:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
What cast?
VMstate takes a void *.
One an object is cast to a void *, it's just as much as PCIESlot as it
is a PCIEPort.
That said, for consistency, I think having everything be relatively to
*one* type for a Property list is pretty helpful.
Expecting someone to know the type hierarchy by heart such that this
doesn't look like a bug is too much IMHO.
I'm updating the patch to that effect for VMState. But I notice the real
fix for qdev properties would be to move the PCIEPort property to the
new PCIEPort type, so that all derived types inherit it. :)
For VMState I believe the real follow-up fix would be mst defining a
central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice.
Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE()
in the first place?
Regards,
Andreas
The real fix is savevm/loadvm taking into account
the class hierarchy.
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: For VMState I believe the real follow-up fix would be mst defining a central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice. Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE() in the first place? The real fix is savevm/loadvm taking into account the class hierarchy. That's not helping, unless you write a patch to show what you mean and how that is going to be migration-compatible. Does your not answering the question mean you don't know? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Tue, Jul 23, 2013 at 11:10:45AM +0200, Andreas Färber wrote: Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: For VMState I believe the real follow-up fix would be mst defining a central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice. Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE() in the first place? The real fix is savevm/loadvm taking into account the class hierarchy. That's not helping, unless you write a patch to show what you mean and I merely mean that if I inherit a class I should inherit it's vmstate. So explicitly adding VMSTATE_PCI_DEVICE should not be necessary. how that is going to be migration-compatible. Most devices put VMSTATE_PCI_DEVICE at the beninning, so just calling that before vmstate for the device should be compatible. Does your not answering the question mean you don't know? Andreas I think the answer is that most pcie devices don't implement AER. AFAIK PCI devices can't support AER at all. -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Am 23.07.2013 11:59, schrieb Michael S. Tsirkin: On Tue, Jul 23, 2013 at 11:10:45AM +0200, Andreas Färber wrote: Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: For VMState I believe the real follow-up fix would be mst defining a central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice. Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE() in the first place? I think the answer is that most pcie devices don't implement AER. AFAIK PCI devices can't support AER at all. Okay, so if it's just PCIe, then XHCI is the oddball preventing moving it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, also operating on PCIDevice. Is there a way to detect use of AER or MSIX to place those into subsections of VMSTATE_PCIE_DEVICE()? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Hi, Okay, so if it's just PCIe, then XHCI is the oddball preventing moving it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, also operating on PCIDevice. Given that live migration support for xhci was added post-1.5 so we don't have a release with it yet this shouldn't be a blocker in case we get this sorted in time for 1.6. Is there a way to detect use of AER or MSIX to place those into subsections of VMSTATE_PCIE_DEVICE()? There is msix_enabled() ... Dunno about AER. cheers, Gerd
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Tue, Jul 23, 2013 at 01:21:13PM +0200, Gerd Hoffmann wrote: Hi, Okay, so if it's just PCIe, then XHCI is the oddball preventing moving it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, also operating on PCIDevice. Given that live migration support for xhci was added post-1.5 so we don't have a release with it yet this shouldn't be a blocker in case we get this sorted in time for 1.6. Is there a way to detect use of AER or MSIX to place those into subsections of VMSTATE_PCIE_DEVICE()? There is msix_enabled() ... msix_present() Dunno about AER. cheers, Gerd Not at the moment but it's not hard to add. -- MST
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Mon, Jul 22, 2013 at 07:42:43PM +0200, Andreas Färber wrote:
Am 21.07.2013 22:26, schrieb Michael S. Tsirkin:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
It doesn't. AFAIU both VMStateDescription and qdev properties store a
numerical offset from the object pointer. CC'ing Juan and Paolo/Anthony.
I.e., if I don't find a counter-example we can drop the arguments from
VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() completely since the
offset would always be 0 and the field name can be hardcoded, cf.
VMSTATE_CPU() in qom/cpu.h.
Currently VMSTATE macros are safe since they all use
the same structure.
You are making them use different types with no
compile or runtime checking that the correct
type is used.
What are the advantages of this hack?
The concrete problems I encountered in this series were a) 80 chars
limit and b) parent_obj.parent_obj.foo.
Since offsetof(MyState, parent_obj.foo) == offsetof(ParentState, foo),
this was a neat way to shorten the line.
I don't think it's a good reason.
The main point of the series is introducing abstract QOM types matching
a specific struct. Renaming the parent field helps catch users that
should no longer be accessing it, both in compile-testing and in future
patch review.
What we would do about VMState when someone at some future point wants
to move PCI state into a QOM interface I have no clue, I'll let the
person who throws the first stone worry about that. ;)
Regards,
Andreas
NACK
Please find a way to have code that won't silently
break when we refactor code.
Normal QOM type use has runtime checks to address that.
@@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass,
void *data)
static const TypeInfo ioh3420_info = {
.name = ioh3420,
-.parent= TYPE_PCI_BRIDGE,
+.parent= TYPE_PCIE_SLOT,
.instance_size = sizeof(PCIESlot),
.class_init= ioh3420_class_init,
};
diff --git a/hw/pci-bridge/xio3130_downstream.c
b/hw/pci-bridge/xio3130_downstream.c
index 9acce3f..549ef26 100644
--- a/hw/pci-bridge/xio3130_downstream.c
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Michael S. Tsirkin m...@redhat.com writes:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
What cast?
VMstate takes a void *.
One an object is cast to a void *, it's just as much as PCIESlot as it
is a PCIEPort.
That said, for consistency, I think having everything be relatively to
*one* type for a Property list is pretty helpful.
Expecting someone to know the type hierarchy by heart such that this
doesn't look like a bug is too much IMHO.
Regards,
Anthony Liguori
What are the advantages of this hack?
@@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass, void
*data)
static const TypeInfo ioh3420_info = {
.name = ioh3420,
-.parent= TYPE_PCI_BRIDGE,
+.parent= TYPE_PCIE_SLOT,
.instance_size = sizeof(PCIESlot),
.class_init= ioh3420_class_init,
};
diff --git a/hw/pci-bridge/xio3130_downstream.c
b/hw/pci-bridge/xio3130_downstream.c
index 9acce3f..549ef26 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,9 +56,8 @@ static void xio3130_downstream_reset(DeviceState *qdev)
static int xio3130_downstream_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -113,9 +112,7 @@ err_bridge:
static void xio3130_downstream_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -147,7 +144,7 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int
devfn, bool multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_xio3130_downstream = {
@@ -157,19 +154,19 @@ static const VMStateDescription
vmstate_xio3130_downstream
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Am 22.07.2013 22:29, schrieb Anthony Liguori:
Michael S. Tsirkin m...@redhat.com writes:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
What cast?
VMstate takes a void *.
One an object is cast to a void *, it's just as much as PCIESlot as it
is a PCIEPort.
That said, for consistency, I think having everything be relatively to
*one* type for a Property list is pretty helpful.
Expecting someone to know the type hierarchy by heart such that this
doesn't look like a bug is too much IMHO.
I'm updating the patch to that effect for VMState. But I notice the real
fix for qdev properties would be to move the PCIEPort property to the
new PCIEPort type, so that all derived types inherit it. :)
For VMState I believe the real follow-up fix would be mst defining a
central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice.
Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE()
in the first place?
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Am 21.07.2013 22:26, schrieb Michael S. Tsirkin:
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
It doesn't. AFAIU both VMStateDescription and qdev properties store a
numerical offset from the object pointer. CC'ing Juan and Paolo/Anthony.
I.e., if I don't find a counter-example we can drop the arguments from
VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() completely since the
offset would always be 0 and the field name can be hardcoded, cf.
VMSTATE_CPU() in qom/cpu.h.
What are the advantages of this hack?
The concrete problems I encountered in this series were a) 80 chars
limit and b) parent_obj.parent_obj.foo.
Since offsetof(MyState, parent_obj.foo) == offsetof(ParentState, foo),
this was a neat way to shorten the line.
The main point of the series is introducing abstract QOM types matching
a specific struct. Renaming the parent field helps catch users that
should no longer be accessing it, both in compile-testing and in future
patch review.
What we would do about VMState when someone at some future point wants
to move PCI state into a QOM interface I have no clue, I'll let the
person who throws the first stone worry about that. ;)
Regards,
Andreas
@@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass, void
*data)
static const TypeInfo ioh3420_info = {
.name = ioh3420,
-.parent= TYPE_PCI_BRIDGE,
+.parent= TYPE_PCIE_SLOT,
.instance_size = sizeof(PCIESlot),
.class_init= ioh3420_class_init,
};
diff --git a/hw/pci-bridge/xio3130_downstream.c
b/hw/pci-bridge/xio3130_downstream.c
index 9acce3f..549ef26 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,9 +56,8 @@ static void xio3130_downstream_reset(DeviceState *qdev)
static int xio3130_downstream_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -113,9 +112,7 @@ err_bridge:
static void xio3130_downstream_exitfn(PCIDevice *d)
{
-
Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/pci-bridge/ioh3420.c| 23 ++-
hw/pci-bridge/xio3130_downstream.c | 23 ++-
hw/pci-bridge/xio3130_upstream.c | 15 +++
hw/pci/pcie_port.c | 22 ++
include/hw/pci/pcie_port.h | 14 --
5 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 728f658..83db054 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
static int ioh3420_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -148,9 +147,7 @@ err_bridge:
static void ioh3420_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_ioh3420 = {
@@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
VMSTATE_END_OF_LIST()
}
};
static Property ioh3420_properties[] = {
-DEFINE_PROP_UINT8(port, PCIESlot, port.port, 0),
+DEFINE_PROP_UINT8(port, PCIEPort, port, 0),
DEFINE_PROP_UINT8(chassis, PCIESlot, chassis, 0),
DEFINE_PROP_UINT16(slot, PCIESlot, slot, 0),
-DEFINE_PROP_UINT16(aer_log_max, PCIESlot,
- port.br.parent_obj.exp.aer_log.log_max,
+DEFINE_PROP_UINT16(aer_log_max, PCIDevice,
+ exp.aer_log.log_max,
PCIE_AER_LOG_MAX_DEFAULT),
DEFINE_PROP_END_OF_LIST(),
};
This looks scary. This does a cast to different types
without any checks at all.
What are the advantages of this hack?
@@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass, void
*data)
static const TypeInfo ioh3420_info = {
.name = ioh3420,
-.parent= TYPE_PCI_BRIDGE,
+.parent= TYPE_PCIE_SLOT,
.instance_size = sizeof(PCIESlot),
.class_init= ioh3420_class_init,
};
diff --git a/hw/pci-bridge/xio3130_downstream.c
b/hw/pci-bridge/xio3130_downstream.c
index 9acce3f..549ef26 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,9 +56,8 @@ static void xio3130_downstream_reset(DeviceState *qdev)
static int xio3130_downstream_initfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIEPort *p = PCIE_PORT(d);
+PCIESlot *s = PCIE_SLOT(d);
int rc;
rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
@@ -113,9 +112,7 @@ err_bridge:
static void xio3130_downstream_exitfn(PCIDevice *d)
{
-PCIBridge *br = PCI_BRIDGE(d);
-PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
-PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
+PCIESlot *s = PCIE_SLOT(d);
pcie_aer_exit(d);
pcie_chassis_del_slot(s);
@@ -147,7 +144,7 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int devfn,
bool multifunction,
qdev_prop_set_uint16(qdev, slot, slot);
qdev_init_nofail(qdev);
-return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
+return PCIE_SLOT(d);
}
static const VMStateDescription vmstate_xio3130_downstream = {
@@ -157,19 +154,19 @@ static const VMStateDescription
vmstate_xio3130_downstream = {
.minimum_version_id_old = 1,
.post_load = pcie_cap_slot_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
-VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
+VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
+VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
vmstate_pcie_aer_log, PCIEAERLog),
