Re: [Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Eduardo Habkost]

On Wed, Jan 20, 2016 at 10:10:11AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > +i440fx_realize = k->realize;
> > > > >  k->realize = igd_pt_i440fx_realize;
> > > 
> > > ... because we are overriding it right here.
> > 
> > Many device classes have a parent_realize field so they can keep
> > a pointer to the original realize function. It's better than a
> > static variable.
> 
> How does the attached patch (incremental fix, not tested yet) look like?

Looks good.

Reviewed-by: Eduardo Habkost 

But, I have a similar question to the one I had about patch
04/11: how did this ever work before?

Does that mean i440fx_realize() was never called when
creating/initializing a TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE
before?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Gerd Hoffmann]

  Hi,

> > > > +i440fx_realize = k->realize;
> > > >  k->realize = igd_pt_i440fx_realize;
> > 
> > ... because we are overriding it right here.
> 
> Many device classes have a parent_realize field so they can keep
> a pointer to the original realize function. It's better than a
> static variable.

How does the attached patch (incremental fix, not tested yet) look like?

cheers,
  Gerd

From 3d110e297b5182107e055db3ab69092affdef5bb Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann 
Date: Wed, 20 Jan 2016 10:08:19 +0100
Subject: [PATCH] [fixup] TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE realize_parent

---
 hw/pci-host/igd.c | 24 +++-
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 160828f..2d51745 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -49,12 +49,24 @@ out_free:
 g_free(path);
 }
 
-static void (*i440fx_realize)(PCIDevice *pci_dev, Error **errp);
+#define IGD_PT_I440FX_CLASS(class)  \
+OBJECT_CLASS_CHECK(IGDPtI440fxClass, (class),   \
+   TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+#define IGD_PT_I440FX_GET_CLASS(obj)\
+OBJECT_GET_CLASS(IGDPtI440fxClass, (obj),   \
+ TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
+typedef struct IGDPtI440fxClass {
+PCIDeviceClass parent_class;
+void (*parent_realize)(PCIDevice *dev, Error **errp);
+} IGDPtI440fxClass;
+
 static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
+IGDPtI440fxClass *k = IGD_PT_I440FX_GET_CLASS(pci_dev);
 Error *err = NULL;
 
-i440fx_realize(pci_dev, );
+k->parent_realize(pci_dev, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
@@ -72,11 +84,12 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
 {
+IGDPtI440fxClass *k = IGD_PT_I440FX_CLASS(klass);
 DeviceClass *dc = DEVICE_CLASS(klass);
-PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
 
-i440fx_realize = k->realize;
-k->realize = igd_pt_i440fx_realize;
+k->parent_realize = pc->realize;
+pc->realize = igd_pt_i440fx_realize;
 dc->desc = "IGD Passthrough Host bridge (i440fx)";
 }
 
@@ -84,6 +97,7 @@ static const TypeInfo igd_passthrough_i440fx_info = {
 .name  = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
 .parent= TYPE_I440FX_PCI_DEVICE,
 .class_init= igd_passthrough_i440fx_class_init,
+.class_size= sizeof(IGDPtI440fxClass),
 };
 
 static void (*q35_realize)(PCIDevice *pci_dev, Error **errp);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Eduardo Habkost]

On Wed, Jan 06, 2016 at 04:45:01PM +0100, Gerd Hoffmann wrote:
> > >  
> > > +static void (*i440fx_realize)(PCIDevice *pci_dev, Error **errp);
> > >  static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
> > >  {
> > > +Error *err = NULL;
> > >  uint32_t val = 0;
> > >  int rc, i, num;
> > >  int pos, len;
> > 
> > Can't we get the parent PCIDeviceClass realize function from pci_dev? So
> > that we don't have to introduce i440fx_realize?
> 
> I don't think so ...
> 
> > >  
> > > +i440fx_realize = k->realize;
> > >  k->realize = igd_pt_i440fx_realize;
> 
> ... because we are overriding it right here.

Many device classes have a parent_realize field so they can keep
a pointer to the original realize function. It's better than a
static variable.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Stefano Stabellini]

On Tue, 5 Jan 2016, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/pci-host/igd.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
> index d1eeafb..6f52ab1 100644
> --- a/hw/pci-host/igd.c
> +++ b/hw/pci-host/igd.c
> @@ -53,12 +53,20 @@ out:
>  return ret;
>  }
>  
> +static void (*i440fx_realize)(PCIDevice *pci_dev, Error **errp);
>  static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
>  {
> +Error *err = NULL;
>  uint32_t val = 0;
>  int rc, i, num;
>  int pos, len;

Can't we get the parent PCIDeviceClass realize function from pci_dev? So
that we don't have to introduce i440fx_realize?


> +i440fx_realize(pci_dev, );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
> +
>  num = ARRAY_SIZE(igd_host_bridge_infos);
>  for (i = 0; i < num; i++) {
>  pos = igd_host_bridge_infos[i].offset;
> @@ -77,6 +85,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> +i440fx_realize = k->realize;
>  k->realize = igd_pt_i440fx_realize;
>  dc->desc = "IGD Passthrough Host bridge";
>  }
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Gerd Hoffmann]

> >  
> > +static void (*i440fx_realize)(PCIDevice *pci_dev, Error **errp);
> >  static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
> >  {
> > +Error *err = NULL;
> >  uint32_t val = 0;
> >  int rc, i, num;
> >  int pos, len;
> 
> Can't we get the parent PCIDeviceClass realize function from pci_dev? So
> that we don't have to introduce i440fx_realize?

I don't think so ...

> >  
> > +i440fx_realize = k->realize;
> >  k->realize = igd_pt_i440fx_realize;

... because we are overriding it right here.

cheers,
  Gerd

[Qemu-devel] [PATCH v3 05/11] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 hw/pci-host/igd.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index d1eeafb..6f52ab1 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -53,12 +53,20 @@ out:
 return ret;
 }
 
+static void (*i440fx_realize)(PCIDevice *pci_dev, Error **errp);
 static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
+Error *err = NULL;
 uint32_t val = 0;
 int rc, i, num;
 int pos, len;
 
+i440fx_realize(pci_dev, );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+
 num = ARRAY_SIZE(igd_host_bridge_infos);
 for (i = 0; i < num; i++) {
 pos = igd_host_bridge_infos[i].offset;
@@ -77,6 +85,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass 
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+i440fx_realize = k->realize;
 k->realize = igd_pt_i440fx_realize;
 dc->desc = "IGD Passthrough Host bridge";
 }
-- 
1.8.3.1