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

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

Isaku/Stafano,

Just want to report that this patch worked without any problems.  As long as 
everyone is OK with hooking igd_pci_read/write functions in piix_pci.c, I'm 
fine with it too.  I wasn't sure about it earlier ...

Allen

-----Original Message-----
From: Isaku Yamahata [mailto:yamahata@xxxxxxxxxxxxx] 
Sent: Tuesday, June 15, 2010 7:08 PM
To: Kay, Allen M
Cc: Stefano Stabellini; xen-devel@xxxxxxxxxxxxxxxxxxx; Han, Weidong; Jean 
Guyader; Ian Pratt; Ross Philipson
Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough 
for Calpella and Sandybridge

Attached.

On Tue, Jun 15, 2010 at 05:58:57PM -0700, Kay, Allen M wrote:
> 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@vali
> nux.co.jp>
> 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
> 

--
yamahata

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