RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge

["Kay, Allen M" <allen.m.kay@xxxxxxxxx>]

Isaku,

I cannot apply the patch below because format problems.  Can you resend it as a 
attachment?

Allen

-----Original Message-----
From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx 
[mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Isaku Yamahata
Sent: Sunday, June 13, 2010 8:30 PM
To: Stefano Stabellini
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; Kay, Allen M; Han, Weidong; Jean Guyader; 
Ian Pratt; Ross Philipson
Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough 
for Calpella and Sandybridge

On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote:
> On Tue, 8 Jun 2010, Kay, Allen M wrote:
> > Isaku,
> > 
> > Thanks for the feedback.
> > 
> > > pci_{read, write}_block() would be better than
> > > switch(len) case 1: case 2: case4:...
> > 
> > Done!
> > 
> > > Is it really necessary to move PCIBus and PCIBridge to the header file?
> > > Doesn't pci_bridge_init() work?
> > 
> > I changed to code to utilize pci_bridge_init().  However, I still need to 
> > move
> > PCIBus and PCIBridge defines to the header file.  The alternative is to 
> > pollute
> > pc.c with graphics passthrough specific code.
> > 
> > > Overriding pci config read/write methods of i440fx would be much cleaner
> > > than hooking pci_data_read/write. (pass igd_pci_read/write to 
> > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL)
> > 
> > Doing this resulted in a lot of duplicated code and also force code path to 
> > change
> > even when IGD passthrough is not used.  To do it correctly, I also need to
> > put in IGD or PASSTHROGH awareness in piix_pci.c.  For now, I'm going to 
> > keep
> > the original patch.
> > 
> 
> I see.
> 
> Even though the patch is an improvement over what we currently have in
> qemu-xen, it is still not acceptable in upstream qemu as it is.
> Considering that we are pretty close to have the merge complete, I think
> it worth trying to come up with something cleaner.
> 
> Isaku, you are the latest one to touch the pci_host code in qemu, do you
> have any other suggestion?

How about the following patch which is on top of the v2 patch?
I did only compile test, but I think it's enough to show my idea.

For PCI bridge, I have a patches to clean up/enhance the implementation and
I'm planning to push to the qemu upstream soon.


>From 9a22525e523e834ab3822d14e715388c19a7d02e Mon Sep 17 00:00:00 2001
Message-Id: 
<9a22525e523e834ab3822d14e715388c19a7d02e.1276485897.git.yamahata@xxxxxxxxxxxxx>
In-Reply-To: <cover.1276485897.git.yamahata@xxxxxxxxxxxxx>
References: <cover.1276485897.git.yamahata@xxxxxxxxxxxxx>
From: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Date: Mon, 14 Jun 2010 12:24:31 +0900
Subject: [PATCH] pci passthrough: some clean up and unexport PCIBus and 
PCIBridge.

some clean up and unexport PCIBus and PCIBridge.

Signed-off-by: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
---
 hw/pass-through.h |   10 ++++++++--
 hw/pci.c          |   29 +++++++++++++++++++++++------
 hw/pci.h          |   23 -----------------------
 hw/piix_pci.c     |    4 ++--
 hw/pt-graphics.c  |   32 +++++++++++++++++++-------------
 5 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/hw/pass-through.h b/hw/pass-through.h
index 242d079..adc9f58 100644
--- a/hw/pass-through.h
+++ b/hw/pass-through.h
@@ -409,13 +409,19 @@ uint8_t pci_intx(struct pt_dev *ptdev);
 u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len);
 int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len);
 void intel_pch_init(PCIBus *bus);
-int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len);
-int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len);
 int register_vga_regions(struct pt_dev *real_device);
 int unregister_vga_regions(struct pt_dev *real_device);
 int setup_vga_pt(struct pt_dev *real_device);
 PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid,
            uint16_t did, const char *name, uint16_t revision);
 
+#ifdef CONFIG_PASSTHROUGH
+void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len);
+uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len);
+#else
+#define igd_pci_write(pci_dev, config_addr, val, len)   NULL
+#define igd_pci_read(pci_dev, config_addr, val, len)    NULL
+#endif
+
 #endif /* __PASSTHROUGH_H__ */
 
diff --git a/hw/pci.c b/hw/pci.c
index a5cb378..b6400f3 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -39,6 +39,24 @@ extern int igd_passthru;
 
 //#define DEBUG_PCI
 
+struct PCIBus {
+    int bus_num;
+    int devfn_min;
+    pci_set_irq_fn set_irq;
+    pci_map_irq_fn map_irq;
+    uint32_t config_reg; /* XXX: suppress */
+    /* low level pic */
+    SetIRQFunc *low_set_irq;
+    qemu_irq *irq_opaque;
+    PCIDevice *devices[256];
+    PCIDevice *parent_dev;
+    PCIBus *next;
+    /* The bus IRQ state is the logical OR of the connected devices.
+       Keep a count of the number of devices with raised IRQs.  */
+    int nirq;
+    int irq_count[];
+};
+
 static void pci_update_mappings(PCIDevice *d);
 static void pci_set_irq(void *opaque, int irq_num, int level);
 
@@ -573,9 +591,6 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t 
val, int len)
            pci_dev->name, config_addr, val, len);
 #endif
 
-#ifdef CONFIG_PASSTHROUGH
-    if (igd_pci_write(pci_dev, config_addr, val, len) == 0)
-#endif
     pci_dev->config_write(pci_dev, config_addr, val, len);
 }
 
@@ -610,9 +625,6 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
     }
     config_addr = addr & 0xff;
 
-#ifdef CONFIG_PASSTHROUGH
-    if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0)
-#endif
     val = pci_dev->config_read(pci_dev, config_addr, len);
 
  done_config_read:
@@ -860,6 +872,11 @@ void pci_unplug_netifs(void)
     }
 }
 
+typedef struct {
+    PCIDevice dev;
+    PCIBus *bus;
+} PCIBridge;
+
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
diff --git a/hw/pci.h b/hw/pci.h
index 7a44035..8abbad7 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -210,29 +210,6 @@ struct PCIDevice {
 typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 
-struct PCIBus {
-    int bus_num;
-    int devfn_min;
-    pci_set_irq_fn set_irq;
-    pci_map_irq_fn map_irq;
-    uint32_t config_reg; /* XXX: suppress */
-    /* low level pic */
-    SetIRQFunc *low_set_irq;
-    qemu_irq *irq_opaque;
-    PCIDevice *devices[256];
-    PCIDevice *parent_dev;
-    PCIBus *next;
-    /* The bus IRQ state is the logical OR of the connected devices.
-       Keep a count of the number of devices with raised IRQs.  */
-    int nirq;
-    int irq_count[];
-};
-
-typedef struct {
-    PCIDevice dev;
-    PCIBus *bus;
-} PCIBridge;
-
 extern char direct_pci_str[];
 extern int direct_pci_msitranslate;
 extern int direct_pci_power_mgmt;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 49d72b2..1bfe0fb 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -25,7 +25,7 @@
 #include "hw.h"
 #include "pc.h"
 #include "pci.h"
-
+#include "pass-through.h"
 
 static void i440fx_set_irq(qemu_irq *pic, int irq_num, int level);
 static void piix3_write_config(PCIDevice *d, 
@@ -207,7 +207,7 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq 
*pic)
     register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
 
     d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0,
-                            NULL, NULL);
+                            igd_pci_read, igd_pci_write);
 
     pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL);
     pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441);
diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index 4923715..32c174d 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -8,6 +8,7 @@
 
 #include <unistd.h>
 #include <sys/ioctl.h>
+#include <assert.h>
 
 extern int gfx_passthru;
 extern int igd_passthru;
@@ -34,10 +35,13 @@ void intel_pch_init(PCIBus *bus)
                     pch_map_irq, "intel_bridge_1f");
 }
 
-int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len)
+void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len)
 {
-    if ( !igd_passthru || (pci_dev->devfn != 0x00 ) )
-        return 0;
+    assert(pci_dev->devfn == 0x00);
+    if ( !igd_passthru ) {
+        pci_default_write_config(pci_dev, config_addr, val, len);
+        return;
+    }
 
     switch (config_addr)
     {
@@ -48,15 +52,18 @@ int igd_pci_write(PCIDevice *pci_dev, int config_addr, 
uint32_t val, int len)
                    PCI_FUNC(pci_dev->devfn), config_addr, len, val);
             break;
         default:
-            pci_dev->config_write(pci_dev, config_addr, val, len);
+            pci_default_write_config(pci_dev, config_addr, val, len);
     }
-    return 1;
 }
 
-int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len)
+uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len)
 {
-    if ( !igd_passthru || (pci_dev->devfn != 0) )
-        return 0;
+    uint32_t val;
+
+    assert(pci_dev->devfn == 0x00);
+    if ( !igd_passthru ) {
+        return pci_default_read_config(pci_dev, config_addr, len);
+    }
 
     switch (config_addr)
     {
@@ -68,17 +75,16 @@ int igd_pci_read(PCIDevice *pci_dev, int config_addr, 
uint32_t *val, int len)
         case 0x58:        /* SNB: PAVPC Offset */
         case 0xa4:        /* SNB: graphics base of stolen memory */
         case 0xa8:        /* SNB: base of GTT stolen memory */
-            *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn),
+            val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn),
                                    0, config_addr, len);
             PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n",
                    pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn),
-                   PCI_FUNC(pci_dev->devfn), config_addr, len, *val);
-
+                   PCI_FUNC(pci_dev->devfn), config_addr, len, val);
             break;
         default:
-            *val = pci_dev->config_read(pci_dev, config_addr, len);
+            val = pci_default_read_config(pci_dev, config_addr, len);
     }
-    return 1;
+    return val;
 }
 
 /*
-- 
1.6.6.1

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel