diff mbox

[v2,4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Message ID CCCF29FA0F52DC4B8E2D1EF38AE07FF7B37C7C@SHSMSX101.ccr.corp.intel.com
State New
Headers show

Commit Message

Tiejun Chen May 21, 2014, 7:07 a.m. UTC 
> -----Original Message-----

> From: Gerd Hoffmann [mailto:kraxel@redhat.com]

> Sent: Monday, May 19, 2014 9:51 PM

> To: Chen, Tiejun

> Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com;

> mst@redhat.com; Kelly.Zytaruk@amd.com; peter.maydell@linaro.org;

> xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;

> anthony@codemonkey.ws; Zhang, Yang Z

> Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve

> 00:02.0 for INTEL IGD

> 

>   Hi,

> 

> > > Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally

> > > create pci devices with auto slot assignment, which will occupy slot 2

> indeed.

> > > Use -device instead to create the devices.

> > >

> >

> > Are you saying we have to create the devices explicitly when we want

> > to work IGD vga with passthrough? But how to make sure all user know

> > this workable way? Maybe you suggest we should document somewhere.

> 

> libvirt does this unconditionally, because it is a good idea anyway for a number

> of reasons.  Example: create a machine with three pci devices, hot-unplug the

> second, then live-migrate to another machine.  The only way to create the

> correct config on the target machine is to explicitly assign slots, otherwise the

> third pci device ends up in the wrong slot.

> 

> Don't know how the libxl (and xl tool) work.  Maybe it does the same anyway.

> Maybe it can handle the address assignment transparently for the user.

> 

> > > Ah, the xen platform device.  /me looks.  Ah, pc_xen_hvm_init

> > > creates this automatically.  Two options here IMHO:

> > >

> > >   (1) Just move it somewhere else explicitly.  For example slot 3, or

> > >       make it a southbridge function (say 00:01.7).

> > >   (2) Don't create it automatically, instead expect management add it

> > >       if needed, using -device xen-plaform,addr=...

> > >

> > > I personally would suggest to go for #2.  As far I know the platform

> > > device is only needed if you want attach xenbus devices to the guest

> > > (correct?), so creating virtual machines without the xen platform

> > > device is a valid use case and you should allow it.

> 

> > Looks you recommend we should change current xen platform design, I'm

> > not sure if something in libxl also need to be modified. Especially,

> > this may not be compatible with those old xen version.

> 

> Going for (1) certainly is easier as (2) indeed will need changes in the libxl, and

> might be tricky to get work with both old+new versions.

> 

> > And especially, how to guarantee no one occupy 00:02.0 in the future

> > with auto assign?

> 

> Creating devices automatically turned out to have a number of problems.

> So it is very unlikely we'll ever do this again.

> 


According to our discussions, I realize we may have some plans or policies dedicated to how to assign devfn, but to support GFX passthrough for XEN, I think currently it may be a better solution to adopt #1 simply like this:


Then we can go out to plan how to assign devfn in common, is this fine?

Thanks
Tiejun

Comments

Tiejun Chen May 22, 2014, 12:31 a.m. UTC | #1 
Just ping, any concern about this?

Thanks
Tiejun

> -----Original Message-----

> From: qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org

> [mailto:qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org] On Behalf Of

> Chen, Tiejun

> Sent: Wednesday, May 21, 2014 3:08 PM

> To: Gerd Hoffmann; Anthony PERARD; Daniel P. Berrange

> Cc: peter.maydell@linaro.org; xen-devel@lists.xensource.com;

> mst@redhat.com; stefano.stabellini@eu.citrix.com; Kay, Allen M;

> Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; Zhang, Yang Z;

> anthony@codemonkey.ws; anthony.perard@citrix.com

> Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve

> 00:02.0 for INTEL IGD

> 

> > -----Original Message-----

> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]

> > Sent: Monday, May 19, 2014 9:51 PM

> > To: Chen, Tiejun

> > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com;

> > mst@redhat.com; Kelly.Zytaruk@amd.com; peter.maydell@linaro.org;

> > xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;

> > anthony@codemonkey.ws; Zhang, Yang Z

> > Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:

> > reserve

> > 00:02.0 for INTEL IGD

> >

> >   Hi,

> >

> > > > Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally

> > > > create pci devices with auto slot assignment, which will occupy

> > > > slot 2

> > indeed.

> > > > Use -device instead to create the devices.

> > > >

> > >

> > > Are you saying we have to create the devices explicitly when we want

> > > to work IGD vga with passthrough? But how to make sure all user know

> > > this workable way? Maybe you suggest we should document somewhere.

> >

> > libvirt does this unconditionally, because it is a good idea anyway

> > for a number of reasons.  Example: create a machine with three pci

> > devices, hot-unplug the second, then live-migrate to another machine.

> > The only way to create the correct config on the target machine is to

> > explicitly assign slots, otherwise the third pci device ends up in the wrong

> slot.

> >

> > Don't know how the libxl (and xl tool) work.  Maybe it does the same

> anyway.

> > Maybe it can handle the address assignment transparently for the user.

> >

> > > > Ah, the xen platform device.  /me looks.  Ah, pc_xen_hvm_init

> > > > creates this automatically.  Two options here IMHO:

> > > >

> > > >   (1) Just move it somewhere else explicitly.  For example slot 3, or

> > > >       make it a southbridge function (say 00:01.7).

> > > >   (2) Don't create it automatically, instead expect management add it

> > > >       if needed, using -device xen-plaform,addr=...

> > > >

> > > > I personally would suggest to go for #2.  As far I know the

> > > > platform device is only needed if you want attach xenbus devices

> > > > to the guest (correct?), so creating virtual machines without the

> > > > xen platform device is a valid use case and you should allow it.

> >

> > > Looks you recommend we should change current xen platform design,

> > > I'm not sure if something in libxl also need to be modified.

> > > Especially, this may not be compatible with those old xen version.

> >

> > Going for (1) certainly is easier as (2) indeed will need changes in

> > the libxl, and might be tricky to get work with both old+new versions.

> >

> > > And especially, how to guarantee no one occupy 00:02.0 in the future

> > > with auto assign?

> >

> > Creating devices automatically turned out to have a number of problems.

> > So it is very unlikely we'll ever do this again.

> >

> 

> According to our discussions, I realize we may have some plans or policies

> dedicated to how to assign devfn, but to support GFX passthrough for XEN, I

> think currently it may be a better solution to adopt #1 simply like this:

> 

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index eaf3e61..500b3c2

> 100644

> --- a/hw/i386/pc_piix.c

> +++ b/hw/i386/pc_piix.c

> @@ -386,7 +386,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs

> *args)

> 

>      bus = pci_find_primary_bus();

>      if (bus != NULL) {

> -        pci_create_simple(bus, -1, "xen-platform");

> +        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");

>      }

>  }

>  #endif

> 

> Then we can go out to plan how to assign devfn in common, is this fine?

> 

> Thanks

> Tiejun
Gerd Hoffmann May 22, 2014, 5:39 a.m. UTC | #2 
Hi,

> > According to our discussions, I realize we may have some plans or policies
> > dedicated to how to assign devfn, but to support GFX passthrough for XEN, I
> > think currently it may be a better solution to adopt #1 simply like this:
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index eaf3e61..500b3c2
> > 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -386,7 +386,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs
> > *args)
> > 
> >      bus = pci_find_primary_bus();
> >      if (bus != NULL) {
> > -        pci_create_simple(bus, -1, "xen-platform");
> > +        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
> >      }
> >  }
> >  #endif
> > 
> > Then we can go out to plan how to assign devfn in common, is this fine?

Somewhere else in the thread someone listed a libxl config file snippet
which is supposed to disable the xen platform device.  Which doesn't
work with upstream qemu for some reason.

Searching ...   ah, here:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03996.html

So, how about making "xen_platform_pci=0" working correctly ?

Another useful thing would be to not create the xen platform device in
case "-nodefaults" was specified on the command line (that switch turns
off a bunch of other devices present by default: vga, nic, cdrom, ...).

cheers,
  Gerd
Tiejun Chen May 22, 2014, 6:18 a.m. UTC | #3 
> -----Original Message-----

> From: Gerd Hoffmann [mailto:kraxel@redhat.com]

> Sent: Thursday, May 22, 2014 1:40 PM

> To: Chen, Tiejun

> Cc: Anthony PERARD; Daniel P. Berrange; peter.maydell@linaro.org;

> xen-devel@lists.xensource.com; mst@redhat.com;

> stefano.stabellini@eu.citrix.com; Kay, Allen M; Kelly.Zytaruk@amd.com;

> qemu-devel@nongnu.org; Zhang, Yang Z; anthony@codemonkey.ws

> Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve

> 00:02.0 for INTEL IGD

> 

>   Hi,

> 

> > > According to our discussions, I realize we may have some plans or

> > > policies dedicated to how to assign devfn, but to support GFX

> > > passthrough for XEN, I think currently it may be a better solution to adopt

> #1 simply like this:

> > >

> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index

> > > eaf3e61..500b3c2

> > > 100644

> > > --- a/hw/i386/pc_piix.c

> > > +++ b/hw/i386/pc_piix.c

> > > @@ -386,7 +386,7 @@ static void

> pc_xen_hvm_init(QEMUMachineInitArgs

> > > *args)

> > >

> > >      bus = pci_find_primary_bus();

> > >      if (bus != NULL) {

> > > -        pci_create_simple(bus, -1, "xen-platform");

> > > +        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");

> > >      }

> > >  }

> > >  #endif

> > >

> > > Then we can go out to plan how to assign devfn in common, is this fine?

> 

> Somewhere else in the thread someone listed a libxl config file snippet which is

> supposed to disable the xen platform device.  Which doesn't work with

> upstream qemu for some reason.

> 

> Searching ...   ah, here:

> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03996.html

> 

> So, how about making "xen_platform_pci=0" working correctly ?


Yes, this is exactly what I did as Anthony told me to add these two lines:

> xen_platform_pci=0

> device_model_args_hvm = [ '-device', 'xen-platform,addr=0x3' ]


But also as I already reply to Anthony yesterday, this doesn't work

http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04101.html

I think this only work with some legacy xen/qemu which don't introduce to support ACPI pcihp.

> 

> Another useful thing would be to not create the xen platform device in case

> "-nodefaults" was specified on the command line (that switch turns off a bunch

> of other devices present by default: vga, nic, cdrom, ...).

> 


Currently looks 'xen-platform' itself can't be created, not those devices existed on that.

Thanks
Tiejun
Konrad Rzeszutek Wilk May 22, 2014, 9:58 a.m. UTC | #4 
On Thu, May 22, 2014 at 07:39:36AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > According to our discussions, I realize we may have some plans or policies
> > > dedicated to how to assign devfn, but to support GFX passthrough for XEN, I
> > > think currently it may be a better solution to adopt #1 simply like this:
> > > 
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index eaf3e61..500b3c2
> > > 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -386,7 +386,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs
> > > *args)
> > > 
> > >      bus = pci_find_primary_bus();
> > >      if (bus != NULL) {
> > > -        pci_create_simple(bus, -1, "xen-platform");
> > > +        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
> > >      }
> > >  }
> > >  #endif
> > > 
> > > Then we can go out to plan how to assign devfn in common, is this fine?
> 
> Somewhere else in the thread someone listed a libxl config file snippet
> which is supposed to disable the xen platform device.  Which doesn't
> work with upstream qemu for some reason.
> 
> Searching ...   ah, here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03996.html
> 
> So, how about making "xen_platform_pci=0" working correctly ?
> 

You don't want that enabled by default.
Otherwise the Linux (and Windows) drivers won't enable
the PV functionality and we are using the emulated drivers (slow).

> Another useful thing would be to not create the xen platform device in
> case "-nodefaults" was specified on the command line (that switch turns
> off a bunch of other devices present by default: vga, nic, cdrom, ...).
> 
> cheers,
>   Gerd
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
diff mbox

Patch

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index eaf3e61..500b3c2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -386,7 +386,7 @@  static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
-        pci_create_simple(bus, -1, "xen-platform");
+        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
     }
 }
 #endif