WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for

To: "Kay\, Allen M" <allen.m.kay@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
From: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Date: Wed, 16 Jun 2010 11:08:29 +0900
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>, "Han, Weidong" <weidong.han@xxxxxxxxx>, Jean Guyader <Jean.Guyader@xxxxxxxxxx>, Ian Pratt <Ian.Pratt@xxxxxxxxxxxxx>, Ross Philipson <Ross.Philipson@xxxxxxxxxx>
Delivery-date: Tue, 15 Jun 2010 19:13:11 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <987664A83D2D224EAE907B061CE93D53014438E3BE@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <987664A83D2D224EAE907B061CE93D530114C3D62D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20100607074551.GB19463@xxxxxxxxxxxxx> <987664A83D2D224EAE907B061CE93D5301150AC175@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <alpine.DEB.2.00.1006111145470.3401@kaball-desktop> <20100614033028.GO23473@xxxxxxxxxxxxx> <987664A83D2D224EAE907B061CE93D53014438E3BE@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.19 (2009-01-05)
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@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
> 

-- 
yamahata

Attachment: 0002-pci-passthrough-some-clean-up-and-unexport-PCIBus-an.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>