diff mbox

[v3,04/11] igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize

Message ID 1451994098-6972-5-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Jan. 5, 2016, 11:41 a.m. UTC 
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/igd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Stefano Stabellini Jan. 6, 2016, 2:32 p.m. UTC | #1 
On Tue, 5 Jan 2016, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/pci-host/igd.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
> index ef0273b..d1eeafb 100644
> --- a/hw/pci-host/igd.c
> +++ b/hw/pci-host/igd.c
> @@ -53,7 +53,7 @@ out:
>      return ret;
>  }
>  
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      uint32_t val = 0;
>      int rc, i, num;
> @@ -65,12 +65,11 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>          len = igd_host_bridge_infos[i].len;
>          rc = host_pci_config_read(pos, len, val);
>          if (rc) {
> -            return -ENODEV;
> +            error_setg(errp, "failed to read host config");
> +            return;
>          }
>          pci_default_write_config(pci_dev, pos, val, len);
>      }
> -
> -    return 0;
>  }
>  
>  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -78,7 +77,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = igd_pt_i440fx_initfn;
> +    k->realize = igd_pt_i440fx_realize;
>      dc->desc = "IGD Passthrough Host bridge";
>  }
>  
> -- 
> 1.8.3.1
>
Eduardo Habkost Jan. 23, 2016, 2:51 p.m. UTC | #2 
On Tue, Jan 05, 2016 at 12:41:31PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/igd.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
> index ef0273b..d1eeafb 100644
> --- a/hw/pci-host/igd.c
> +++ b/hw/pci-host/igd.c
> @@ -53,7 +53,7 @@ out:
>      return ret;
>  }
>  
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      uint32_t val = 0;
>      int rc, i, num;
> @@ -65,12 +65,11 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>          len = igd_host_bridge_infos[i].len;
>          rc = host_pci_config_read(pos, len, val);
>          if (rc) {
> -            return -ENODEV;
> +            error_setg(errp, "failed to read host config");
> +            return;
>          }
>          pci_default_write_config(pci_dev, pos, val, len);
>      }
> -
> -    return 0;
>  }
>  
>  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -78,7 +77,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = igd_pt_i440fx_initfn;
> +    k->realize = igd_pt_i440fx_realize;

I am trying to understand how this have ever worked before:

* PCIDeviceClass::init is called by pci_default_realize()
  (default value for PCIDeviceClass::realize)
* i440fx_class_init() overrides PCIDeviceClass::realize
  to i440fx_realize()

So, when exactly was igd_pt_i440fx_realize() being called, before
this series? I don't have a Xen host to be able to test it using
xenfv, and if I test "-machine pc,igd-passthrough=on" after
applying patch 01/11, I don't see igd_pt_i440fx_initfn() being
called at all.
Gerd Hoffmann Jan. 25, 2016, 8:59 a.m. UTC | #3 
Hi,

> >  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> > @@ -78,7 +77,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >  
> > -    k->init = igd_pt_i440fx_initfn;
> > +    k->realize = igd_pt_i440fx_realize;
> 
> I am trying to understand how this have ever worked before:
> 
> * PCIDeviceClass::init is called by pci_default_realize()
>   (default value for PCIDeviceClass::realize)
> * i440fx_class_init() overrides PCIDeviceClass::realize
>   to i440fx_realize()
> 
> So, when exactly was igd_pt_i440fx_realize() being called, before
> this series?

It simply didn't?

I suspect this got ported over from the qemu-xen tree, but wasn't really
tested and also not adapted to commit "9af21db pci: Trivial device model
conversions to realize".  So this patch actually is yet another
bugfix ...

Current test status of this series (by Hao) is this:

  * newer linux intel drivers work just fine without chipset tweaks.
  * older linux intel drivers and windows guests don't work even with
    all the fixes in this series.

So applying the series doesn't improve things at all (code cleanups
aside).  My current plan to go forward is:

  (a) get test hardware (wip atm).
  (b) go figure what is really needed, lots of testing.
  (c) rework and repost series.

cheers,
  Gerd
Stefano Stabellini Jan. 25, 2016, 11:53 a.m. UTC | #4 
On Mon, 25 Jan 2016, Gerd Hoffmann wrote:
>   Hi,
> 
> > >  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> > > @@ -78,7 +77,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > >  
> > > -    k->init = igd_pt_i440fx_initfn;
> > > +    k->realize = igd_pt_i440fx_realize;
> > 
> > I am trying to understand how this have ever worked before:
> > 
> > * PCIDeviceClass::init is called by pci_default_realize()
> >   (default value for PCIDeviceClass::realize)
> > * i440fx_class_init() overrides PCIDeviceClass::realize
> >   to i440fx_realize()
> > 
> > So, when exactly was igd_pt_i440fx_realize() being called, before
> > this series?
> 
> It simply didn't?
> 
> I suspect this got ported over from the qemu-xen tree, but wasn't really
> tested and also not adapted to commit "9af21db pci: Trivial device model
> conversions to realize".  So this patch actually is yet another
> bugfix ...

You are probably right. For your reference, the original code is here:

http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=blob_plain;f=hw/pt-graphics.c;hb=HEAD
diff mbox

Patch

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index ef0273b..d1eeafb 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -53,7 +53,7 @@  out:
     return ret;
 }
 
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
     uint32_t val = 0;
     int rc, i, num;
@@ -65,12 +65,11 @@  static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
         len = igd_host_bridge_infos[i].len;
         rc = host_pci_config_read(pos, len, val);
         if (rc) {
-            return -ENODEV;
+            error_setg(errp, "failed to read host config");
+            return;
         }
         pci_default_write_config(pci_dev, pos, val, len);
     }
-
-    return 0;
 }
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -78,7 +77,7 @@  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = igd_pt_i440fx_initfn;
+    k->realize = igd_pt_i440fx_realize;
     dc->desc = "IGD Passthrough Host bridge";
 }