Re: [Qemu-devel] [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

On 9/22/2015 12:03 AM, Stefano Stabellini wrote:

It is going to be in QEMU 2.5 and qemu-xen 4.7.


Thanks for your reply.

Do we have any possibility of just merging this series into qemu-xen 
4.6? We really want to support IGD passthrough on xen 4.6 if possible :)


Thanks
Tiejun



On Mon, 21 Sep 2015, Chen, Tiejun wrote:

Stefano,

I have two questions,

#1. Which qemu version is this igd stuff going into? 2.6?
#2. Is this igd stuff going into qemu-xen inside xen? Any plan to go into xen
4.6?

Thanks
Tiejun

On 9/9/2015 1:21 AM, Stefano Stabellini wrote:
> The following changes since commit 8611280505119e296757a60711a881341603fa5a:
>
>target-microblaze: Use setcond for pcmp* (2015-09-08 08:49:33 +0200)
>
> are available in the git repository at:
>
>git://xenbits.xen.org/people/sstabellini/qemu-dm.git
> tags/xen-2015-09-08-tag
>
> for you to fetch changes up to ba2250ad148997b1352aba976aac66b55410e7e4:
>
>xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.
> (2015-09-08 15:21:56 +)
>
> 
> Xen branch xen-2015-09-08
>
> 
> Don Slutz (1):
>xen-hvm: Add trace to ioreq
>
> Jan Beulich (1):
>xen/HVM: atomically access pointers in bufioreq handling
>
> Konrad Rzeszutek Wilk (7):
>xen-hvm: When using xc_domain_add_to_physmap also include errno when
> reporting
>xen/pt: Update comments with proper function name.
>xen/pt: Make xen_pt_msi_set_enable static
>xen/pt: xen_host_pci_config_read returns -errno, not -1 on failure
>xen: use errno instead of rc for xc_domain_add_to_physmap
>xen/pt/msi: Add the register value when printing logging and error
> messages
>xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.
>
> Michael S. Tsirkin (1):
>i440fx: make types configurable at run-time
>
> Tiejun Chen (9):
>pc_init1: pass parameters just with types
>piix: create host bridge to passthrough
>hw/pci-assign: split pci-assign.c
>xen, gfx passthrough: basic graphics passthrough support
>xen, gfx passthrough: retrieve VGA BIOS to work
>igd gfx passthrough: create a isa bridge
>xen, gfx passthrough: register a isa bridge
>xen, gfx passthrough: register host bridge specific to passthrough
>xen, gfx passthrough: add opregion mapping
>
>   configure |   28 +
>   hw/core/machine.c |   20 +++
>   hw/i386/Makefile.objs |1 +
>   hw/i386/kvm/pci-assign.c  |   82 ++---
>   hw/i386/pc_piix.c |  139 -
>   hw/i386/pci-assign-load-rom.c |   93 ++
>   hw/pci-host/piix.c|   91 +-
>   hw/xen/Makefile.objs  |1 +
>   hw/xen/xen-host-pci-device.c  |5 +
>   hw/xen/xen-host-pci-device.h  |1 +
>   hw/xen/xen_pt.c   |   42 ++-
>   hw/xen/xen_pt.h   |   22 +++-
>   hw/xen/xen_pt_config_init.c   |   59 -
>   hw/xen/xen_pt_graphics.c  |  272
> +
>   hw/xen/xen_pt_msi.c   |2 +-
>   include/hw/boards.h   |1 +
>   include/hw/i386/pc.h  |9 +-
>   include/hw/pci/pci-assign.h   |   27 
>   include/hw/xen/xen_common.h   |   34 +-
>   qemu-options.hx   |3 +
>   trace-events  |7 ++
>   vl.c  |   10 ++
>   xen-hvm.c |   55 +++--
>   23 files changed, 891 insertions(+), 113 deletions(-)
>   create mode 100644 hw/i386/pci-assign-load-rom.c
>   create mode 100644 hw/xen/xen_pt_graphics.c
>   create mode 100644 include/hw/pci/pci-assign.h
>
> ___
> Xen-devel mailing list
> xen-de...@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [Qemu-devel] [v4][PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Chen, Tiejun]

Ping...

Thanks
Tiejun

On 9/18/2015 4:30 PM, Tiejun Chen wrote:

Ian,

As we discussed previously,

http://patchwork.ozlabs.org/patch/457055/

now it's time to push this into on xen/tools side since all qemu stuffs
have been merged.

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02094.html

v4:

Ian,

Actually we had v3.5 online previously, which was reviewed by you.

http://permalink.gmane.org/gmane.comp.emulators.qemu/329100

So here I just bring a little bit to refine code just for patch #2
according to out last conversation.

v3:

* Refine some codes based on Campbell's feedback so thanks for Campbell's
   kind guideline to patch #2
* Update the manpages in patch #2

v2:

* Refine patch #2's head description
* Improve codes quality inside patch #1 based on Wei's comments
* Refill the summary inside patch #0 based on Konrad and Wei's suggestion

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,igd-passthru=on".

https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02050.html

This need to bring a change on tool side.

After a discussion with Campbell, we'd like to construct a table to record
all IGD devices we can support. If we hit that table, we should pass that
option. And so we also introduce a new field of type, 'gfx_passthru_kind',
to cooperate with 'gfx_passthru' to cover all scenarios like this,

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to true and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD

And note actually that option "-gfx_passthru" is just introduced to
work for qemu-xen-traditional so we should get this away from
libxl__build_device_model_args_new() in the case of qemu upstream.


Tiejun Chen (2):
   libxl: introduce libxl__is_igd_vga_passthru
   libxl: introduce gfx_passthru_kind

  docs/man/xl.cfg.pod.5|  35 --
  tools/libxl/libxl.h  |   6 ++
  tools/libxl/libxl_dm.c   |  46 +++--
  tools/libxl/libxl_internal.h |   2 +
  tools/libxl/libxl_pci.c  | 124 +++
  tools/libxl/libxl_types.idl  |   6 ++
  tools/libxl/xl_cmdimpl.c |  14 +++-
  7 files changed, 223 insertions(+), 10 deletions(-)

Thanks
Tiejun

Re: [Qemu-devel] [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

Stefano,

I have two questions,

#1. Which qemu version is this igd stuff going into? 2.6?
#2. Is this igd stuff going into qemu-xen inside xen? Any plan to go 
into xen 4.6?


Thanks
Tiejun

On 9/9/2015 1:21 AM, Stefano Stabellini wrote:

The following changes since commit 8611280505119e296757a60711a881341603fa5a:

   target-microblaze: Use setcond for pcmp* (2015-09-08 08:49:33 +0200)

are available in the git repository at:

   git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-2015-09-08-tag

for you to fetch changes up to ba2250ad148997b1352aba976aac66b55410e7e4:

   xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings. 
(2015-09-08 15:21:56 +)


Xen branch xen-2015-09-08


Don Slutz (1):
   xen-hvm: Add trace to ioreq

Jan Beulich (1):
   xen/HVM: atomically access pointers in bufioreq handling

Konrad Rzeszutek Wilk (7):
   xen-hvm: When using xc_domain_add_to_physmap also include errno when 
reporting
   xen/pt: Update comments with proper function name.
   xen/pt: Make xen_pt_msi_set_enable static
   xen/pt: xen_host_pci_config_read returns -errno, not -1 on failure
   xen: use errno instead of rc for xc_domain_add_to_physmap
   xen/pt/msi: Add the register value when printing logging and error 
messages
   xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.

Michael S. Tsirkin (1):
   i440fx: make types configurable at run-time

Tiejun Chen (9):
   pc_init1: pass parameters just with types
   piix: create host bridge to passthrough
   hw/pci-assign: split pci-assign.c
   xen, gfx passthrough: basic graphics passthrough support
   xen, gfx passthrough: retrieve VGA BIOS to work
   igd gfx passthrough: create a isa bridge
   xen, gfx passthrough: register a isa bridge
   xen, gfx passthrough: register host bridge specific to passthrough
   xen, gfx passthrough: add opregion mapping

  configure |   28 +
  hw/core/machine.c |   20 +++
  hw/i386/Makefile.objs |1 +
  hw/i386/kvm/pci-assign.c  |   82 ++---
  hw/i386/pc_piix.c |  139 -
  hw/i386/pci-assign-load-rom.c |   93 ++
  hw/pci-host/piix.c|   91 +-
  hw/xen/Makefile.objs  |1 +
  hw/xen/xen-host-pci-device.c  |5 +
  hw/xen/xen-host-pci-device.h  |1 +
  hw/xen/xen_pt.c   |   42 ++-
  hw/xen/xen_pt.h   |   22 +++-
  hw/xen/xen_pt_config_init.c   |   59 -
  hw/xen/xen_pt_graphics.c  |  272 +
  hw/xen/xen_pt_msi.c   |2 +-
  include/hw/boards.h   |1 +
  include/hw/i386/pc.h  |9 +-
  include/hw/pci/pci-assign.h   |   27 
  include/hw/xen/xen_common.h   |   34 +-
  qemu-options.hx   |3 +
  trace-events  |7 ++
  vl.c  |   10 ++
  xen-hvm.c |   55 +++--
  23 files changed, 891 insertions(+), 113 deletions(-)
  create mode 100644 hw/i386/pci-assign-load-rom.c
  create mode 100644 hw/xen/xen_pt_graphics.c
  create mode 100644 include/hw/pci/pci-assign.h

___
Xen-devel mailing list
xen-de...@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

On 9/15/2015 7:00 PM, Paolo Bonzini wrote:



On 15/09/2015 11:55, Stefano Stabellini wrote:

On Mon, 14 Sep 2015, Paolo Bonzini wrote:

> On 10/09/2015 12:29, Stefano Stabellini wrote:

> > +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > +return -errno;
> > +}
> >  do {
> > -rc = pread(config_fd, (uint8_t *)&val, len, pos);
> > +rc = read(config_fd, (uint8_t *)&val, len);
> >  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));

>
> This leaks config_fd.

I don't follow, it leaks config_fd where?


Where lseek returns -errno (and IIRC in other places in the same function).


Do you mean we need this change?

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 1fb71c8..7d44228 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -775,15 +775,18 @@ static int host_pci_config_read(int pos, int len, 
uint32_t val)

 }

 if (lseek(config_fd, pos, SEEK_SET) != pos) {
+close(config_fd);
 return -errno;
 }
 do {
 rc = read(config_fd, (uint8_t *)&val, len);
 } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 if (rc != len) {
+close(config_fd);
 return -errno;
 }

+close(config_fd);
 return 0;
 }


Thanks
Tiejun

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

Thanks! I'll fold it the offending patch
(http://marc.info/?l=qemu-devel&m=144174596628052&w=2) and resend.



Reviewed-by: Michael S. Tsirkin 



Michale and Stefano,

Thanks a lot :)

Tiejun

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
was set by configure. That won't be the case on OSX or Windows, where
the Xen headers don't exist.



Okay. This actually shouldn't be enabled on Windows so what about this?

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 58a33fb..9a1fcb9 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -739,6 +739,7 @@ static const TypeInfo i440fx_info = {
 .class_init= i440fx_class_init,
 };

+#ifndef _WIN32
 /* IGD Passthrough Host Bridge. */
 typedef struct {
 uint8_t offset;
@@ -819,6 +820,7 @@ static const TypeInfo igd_passthrough_i440fx_info = {
 .instance_size = sizeof(PCII440FXState),
 .class_init= igd_passthrough_i440fx_class_init,
 };
+#endif

 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
 PCIBus *rootbus)
@@ -861,7 +863,9 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
 type_register_static(&i440fx_info);
+#ifndef _WIN32
 type_register_static(&igd_passthrough_i440fx_info);
+#endif
 type_register_static(&piix3_pci_type_info);
 type_register_static(&piix3_info);
 type_register_static(&piix3_xen_info);

Thanks
Tiejun

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

As you see this short log, "hw/pci-assign: split pci-assign.c", so this
means I just extract something from the original hw/i386/kvm/pci-assign.c,
and here so I just keep those original head files residing
hw/i386/kvm/pci-assign.c, and I didn't introduce anything new.


hw/i386/kvm/pci-assign.c is only built if configure set CONFIG_KVM,
which it won't do on Windows or OSX builds.

It sounds like your patch has incorrectly moved code out of files
which are compiled only if KVM is present, or only if we're doing
Xen PCI passthrough, and into compiled-for-everything files.



Yes, we want to share this chunk of codes between Xen and Kvm. Just to 
this error, could we remove #include ? As I mentioned I still 
can compile this file without this head file.


Thanks
Tiejun

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

On 9/10/2015 12:10 AM, Stefano Stabellini wrote:

On Wed, 9 Sep 2015, Stefano Stabellini wrote:

On Tue, 8 Sep 2015, Peter Maydell wrote:
> On 8 September 2015 at 18:21, Stefano Stabellini
>  wrote:
> > The following changes since commit 8611280505119e296757a60711a881341603fa5a:
> >
> >   target-microblaze: Use setcond for pcmp* (2015-09-08 08:49:33 +0200)
> >
> > are available in the git repository at:
> >
> >   git://xenbits.xen.org/people/sstabellini/qemu-dm.git 
tags/xen-2015-09-08-tag
> >
> > for you to fetch changes up to ba2250ad148997b1352aba976aac66b55410e7e4:
> >
> >   xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings. 
(2015-09-08 15:21:56 +)
> >
> > 
> > Xen branch xen-2015-09-08
> >
> > 
>
> Hi. I'm afraid this fails to build on OSX (and probably Windows too,
> though that build hasn't run yet):
>
>   CCi386-softmmu/hw/i386/pci-assign-load-rom.o
> /Users/pm215/src/qemu/hw/i386/pci-assign-load-rom.c:6:10: fatal error:
>   'sys/io.h' file not found
> #include 
>  ^
>   CCalpha-softmmu/hw/alpha/pci.o
> 1 error generated.

Tiejun,

this is caused by 33d33242b7d802e6c994f3d56ecba96a66465dc3,
"hw/pci-assign: split pci-assign.c". Could you please double-check
non-Linux builds?


I found another issue introduced by the gfx passthrough series on
Windows:

../hw/pci-host/piix.o: In function `host_pci_config_read':
/root/qemu/hw/pci-host/piix.c:778: undefined reference to `_pread'

It is introduced by:

commit fdb70721ba0496a767137e5505dd27627d19c4a8
Author: Tiejun Chen 
Date:   Wed Jul 15 13:37:43 2015 +0800

 piix: create host bridge to passthrough


You might have to replace the pread call with lseek and read.



This is also surprising to me. Just see xen_host_pci_config_() 
inside /hw/xen/xen-host-pci-device.c, there are so many this pread 
usage(). So I really don't understand what's difference between these 
two files.	


Thanks
Tiejun

Re: [Qemu-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

On 9/9/2015 9:06 PM, Stefano Stabellini wrote:

On Tue, 8 Sep 2015, Peter Maydell wrote:

On 8 September 2015 at 18:21, Stefano Stabellini
 wrote:
> The following changes since commit 8611280505119e296757a60711a881341603fa5a:
>
>   target-microblaze: Use setcond for pcmp* (2015-09-08 08:49:33 +0200)
>
> are available in the git repository at:
>
>   git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-2015-09-08-tag
>
> for you to fetch changes up to ba2250ad148997b1352aba976aac66b55410e7e4:
>
>   xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings. 
(2015-09-08 15:21:56 +)
>
> 
> Xen branch xen-2015-09-08
>
> 

Hi. I'm afraid this fails to build on OSX (and probably Windows too,
though that build hasn't run yet):

  CCi386-softmmu/hw/i386/pci-assign-load-rom.o
/Users/pm215/src/qemu/hw/i386/pci-assign-load-rom.c:6:10: fatal error:
  'sys/io.h' file not found
#include 
 ^
  CCalpha-softmmu/hw/alpha/pci.o
1 error generated.


Tiejun,

this is caused by 33d33242b7d802e6c994f3d56ecba96a66465dc3,
"hw/pci-assign: split pci-assign.c". Could you please double-check
non-Linux builds?


Its interesting.

As you see this short log, "hw/pci-assign: split pci-assign.c", so this 
means I just extract something from the original 
hw/i386/kvm/pci-assign.c, and here so I just keep those original head 
files residing hw/i386/kvm/pci-assign.c, and I didn't introduce anything 
new.


So its very probably that you still can't compile successfully even 
without my commit on OSX/Windows, right? I think Peter may be right,


"Will passthrough even work on Windows and OSX hosts?
Consider whether we should be building this code on those
hosts at all..."

I prefer this isn't what we did previously.




I suspect that the fix would be quite small, but I don't have an OSX or
a Windows build environment to try it.


I haven't a this build environment as well. But I think right now you 
can remove "#include " to fix this simply since looks this is 
redundant actually.


hw/i386/pci-assign: remove one head file

This is redundant actually but really break OS/Windows build.

Signed-off-by: Tiejun Chen 

diff --git a/hw/i386/pci-assign-load-rom.c b/hw/i386/pci-assign-load-rom.c
index bad53b7..1f0d4ef 100644
--- a/hw/i386/pci-assign-load-rom.c
+++ b/hw/i386/pci-assign-load-rom.c
@@ -3,7 +3,6 @@
  */
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 


At least I can build this under Linux,

./configure --target-list=x86_64-softmmu && make

Thanks
Tiejun



Speak about build environments, Peter, would you care to share your
scripts and setup so that I can run similar tests in the future on my
own?  I have no OSX machines so I tried to do a Windows
cross-compile, following http://wiki.qemu.org/Hosts/W32 on Debian 7, but
I failed very early with an "ERROR: zlib check failed".

Re: [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough

[Chen, Tiejun]

On 2015/7/15 20:38, Stefano Stabellini wrote:

On Wed, 15 Jul 2015, Michael S. Tsirkin wrote:

On Wed, Jul 15, 2015 at 12:46:29PM +0100, Stefano Stabellini wrote:

Thanks Tiejun, the patch series looks OK to me now.

It looks like it has all the required acks.
Michael, are you OK with it? If so, should I add it to my queue, or do
you want to add it to yours?


I'm OK with the PC bits. Please merge it through your tree.


OK. Added to my queue. I'll send it out after 2.4.


Really thanks both you guys.

Thanks
Tiejun





On Wed, 15 Jul 2015, Tiejun Chen wrote:

v10:

* Don't extern igd_passthrough_isa_bridge_create() in the
   include/hw/xen/xen.h file. Instead, move inside the
   include/hw/i386/pc.h file in patch #7

v9:

* Rebase on the latest
* Inside patch #8, move is_igd_vga_passthrough(dev)) from
   xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
* Inside patch #9, simplify pc_xen_hvm_init_pci()
* Michael acked them on pc side
* Stefano ackes then on xen side

v8:

* Rebase on the latest qemu tree
* Cleanup one xen leftover in patch #3

v7:

* Instead of "-gfx_passthru" we'd like to make that a machine
   option, "-machine xxx,igd-passthru=on""
* try to make something as common shared by others like KvmGT in
   the future
* Just read those real value from host bridge pci
   configuration space when create host bridge then put in dev->config.

v6:

* Drop introducing a new machine specific to IGD passthrough
* Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
* Currently IGD drivers always need to access PCH by 1f.0. But we
   don't want to poke that directly to get ID, and although in real
   world different GPU should have different PCH. But actually the
   different PCH DIDs likely map to different PCH SKUs. We do the
   same thing for the GPU. For PCH, the different SKUs are going to
   be all the same silicon design and implementation, just different
   features turn on and off with fuses. The SW interfaces should be
   consistent across all SKUs in a given family (eg LPT). But just
   same features may not be supported.

   Most of these different PCH features probably don't matter to the
   Gfx driver, but obviously any difference in display port connections
   will so it should be fine with any PCH in case of passthrough.

   So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
   scenarios, 0x9cc3 for BDW(Broadwell).
* Drop igd write ops since its fine to emulate that, and we also shrink
   those igd read ops as necessary.
* Rebase and cleanup all patches.

v5:

* Simplify to make sure its really inherited from the standard one in patch #3
* Then drop the original patch #3

v4:

* Rebase on latest tree
* Drop patch #2
* Regenerate patches after Michael introduce patch #1
* We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
* Test: boot with a preinstalled winxp
   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc

v3:

* Drop patch #4
* Add one patch #1 from Michael
* Rebase
* In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.


Michael S. Tsirkin (1):
   i440fx: make types configurable at run-time

Tiejun Chen (9):
   pc_init1: pass parameters just with types
   piix: create host bridge to passthrough
   hw/pci-assign: split pci-assign.c
   xen, gfx passthrough: basic graphics passthrough support
   xen, gfx passthrough: retrieve VGA BIOS to work
   igd gfx passthrough: create a isa bridge
   xen, gfx passthrough: register a isa bridge
   xen, gfx passthrough: register host bridge specific to passthrough
   xen, gfx passthrough: add opregion mapping

  hw/core/machine.c |  20 +++
  hw/i386/Makefile.objs |   1 +
  hw/i386/kvm/pci-assign.c  |  82 +-
  hw/i386/pc_piix.c | 139 -
  hw/i386/pci-assign-load-rom.c |  93 
  hw/pci-host/piix.c|  91 +++-
  hw/xen/Makefile.objs  |   1 +
  hw/xen/xen-host-pci-device.c  |   5 +
  hw/xen/xen-host-pci-device.h  |   1 +
  hw/xen/xen_pt.c   |  36 +
  hw/xen/xen_pt.h   |  21 ++-
  hw/xen/xen_pt_config_init.c   |  51 ++-
  hw/xen/xen_pt_graphics.c  | 272 ++
  include/hw/boards.h   |   1 +
  include/hw/i386/pc.h  |   9 +-
  include/hw/pci/pci-assign.h   |  27 
  qemu-options.hx   |   3 +
  vl.c

Re: [Qemu-devel] [v9][PATCH 08/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 4356af4..703148e 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -51,4 +51,5 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
  #  define HVM_MAX_VCPUS 32
  #endif

+extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t 
gpu_dev_id);
  #endif /* QEMU_HW_XEN_H */


You are right that I was confused on my previous comment, but it is true
that it shouldn't be part of this patch: it should be part of patch


Right.


7/10. Also the declaration should probably not be in the xen.h header.

Michael, where do you think should go?


Donnu - add a header?



What about the include/hw/i386/pc.h file?

Thanks
Tiejun

Re: [Qemu-devel] [v8][RESEND][PATCH 00/10] xen: add Intel IGD passthrough

[Chen, Tiejun]

On 2015/7/2 0:03, Stefano Stabellini wrote:

Aside from a couple of really minor stylistic issues, I think the patch


Thanks for your review.


series can go in from my point of view. However it looks like you are
still missing a few acked-by/reviewed-by on the non-xen patches.


Just yesterday Michael Acked them :)

Note once you're fine to my replies to your previous comments, I'd like 
to rebase them on the latest and then send out the last revision adding 
Acked-by with you and Michael.


Thanks
Tiejun



On Wed, 1 Jul 2015, Chen, Tiejun wrote:

Ping...

Thanks
Tiejun

On 2015/6/5 16:44, Tiejun Chen wrote:

v8:

* Rebase on the latest qemu tree
* Cleanup one xen leftover in patch #3

v7:

* Instead of "-gfx_passthru" we'd like to make that a machine
option, "-machine xxx,igd-passthru=on""
* try to make something as common shared by others like KvmGT in
the future
* Just read those real value from host bridge pci
configuration space when create host bridge then put in dev->config.

v6:

* Drop introducing a new machine specific to IGD passthrough
* Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
* Currently IGD drivers always need to access PCH by 1f.0. But we
don't want to poke that directly to get ID, and although in real
world different GPU should have different PCH. But actually the
different PCH DIDs likely map to different PCH SKUs. We do the
same thing for the GPU. For PCH, the different SKUs are going to
be all the same silicon design and implementation, just different
features turn on and off with fuses. The SW interfaces should be
consistent across all SKUs in a given family (eg LPT). But just
same features may not be supported.

Most of these different PCH features probably don't matter to the
Gfx driver, but obviously any difference in display port connections
will so it should be fine with any PCH in case of passthrough.

So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
scenarios, 0x9cc3 for BDW(Broadwell).
* Drop igd write ops since its fine to emulate that, and we also shrink
those igd read ops as necessary.
* Rebase and cleanup all patches.

v5:

* Simplify to make sure its really inherited from the standard one in patch
#3
* Then drop the original patch #3

v4:

* Rebase on latest tree
* Drop patch #2
* Regenerate patches after Michael introduce patch #1
* We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
* Test: boot with a preinstalled winxp
./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c
-machine pc

v3:

* Drop patch #4
* Add one patch #1 from Michael
* Rebase
* In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine
pc

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.


Michael S. Tsirkin (1):
i440fx: make types configurable at run-time

Tiejun Chen (9):
pc_init1: pass parameters just with types
piix: create host bridge to passthrough
hw/pci-assign: split pci-assign.c
xen, gfx passthrough: basic graphics passthrough support
xen, gfx passthrough: retrieve VGA BIOS to work
igd gfx passthrough: create a isa bridge
xen, gfx passthrough: register a isa bridge
xen, gfx passthrough: register host bridge specific to passthrough
xen, gfx passthrough: add opregion mapping

   hw/core/machine.c |  20 +++
   hw/i386/Makefile.objs |   1 +
   hw/i386/kvm/pci-assign.c  |  82 +-
   hw/i386/pc_piix.c | 151 ++-
   hw/i386/pci-assign-load-rom.c |  93 
   hw/pci-host/piix.c|  91 +++-
   hw/xen/Makefile.objs  |   1 +
   hw/xen/xen-host-pci-device.c  |   5 +
   hw/xen/xen-host-pci-device.h  |   1 +
   hw/xen/xen_pt.c   |  32 
   hw/xen/xen_pt.h   |  21 ++-
   hw/xen/xen_pt_config_init.c   |  51 ++-
   hw/xen/xen_pt_graphics.c  | 272 ++
   include/hw/boards.h   |   1 +
   include/hw/i386/pc.h  |   8 +-
   include/hw/pci/pci-assign.h   |  27 
   include/hw/xen/xen.h  |   1 +
   qemu-options.hx   |   3 +
   vl.c  |  10 ++
   19 files changed, 780 insertions(+), 91 deletions(-)
   create mode 100644 hw/i386/pci-assign-load-rom.c
   create mode 100644 hw/xen/xen_pt_graphics.c
   create mode 100644 include/hw/pci/pci-assign.h

Thanks
Tiejun

Re: [Qemu-devel] [v8][RESEND][PATCH 09/10] xen, gfx passthrough: register host bridge specific to passthrough

[Chen, Tiejun]

  >>   #ifdef CONFIG_XEN

+static void igd_passthrough_pc_init_pci(MachineState *machine)
+{
+pc_init1(machine,
+ TYPE_I440FX_PCI_HOST_BRIDGE,
+ TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE);
+}
+
+static void pc_init_pci(MachineState *machine)
+{
+pc_init1(machine,
+ TYPE_I440FX_PCI_HOST_BRIDGE,
+ TYPE_I440FX_PCI_DEVICE);
+}
+
+static void pc_xen_hvm_init_pci(MachineState *machine)
+{
+if (has_igd_gfx_passthru)
+igd_passthrough_pc_init_pci(machine);
+else
+pc_init_pci(machine);
+}


I don't see any value in introducing pc_init_pci and
igd_passthrough_pc_init_pci.  I would expand both of them here.



Agree, and what about this?

static void pc_xen_hvm_init_pci(MachineState *machine)
{
const char *pci_type = has_igd_gfx_passthru ?
TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : 
TYPE_I440FX_PCI_DEVICE;


pc_init1(machine,
 TYPE_I440FX_PCI_HOST_BRIDGE,
 pci_type);
}

Thanks
Tiejun

Re: [Qemu-devel] [v8][RESEND][PATCH 08/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

+static void
+xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
+  XenHostPCIDevice *dev)
+{
+uint16_t gpu_dev_id;
+PCIDevice *d = &s->dev;
+
+if (!is_igd_vga_passthrough(dev)) {
+return;
+}


I would rather move the if into xen_pt_initfn, otherwise reading
xen_pt_initfn it looks like we are going to create an isa bridge
regardless.


This makes sense.





+gpu_dev_id = dev->device_id;
+igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
+}
+
  /* init */

  static int xen_pt_initfn(PCIDevice *d)
@@ -725,6 +740,9 @@ static int xen_pt_initfn(PCIDevice *d)


I'd like to add something like,

if (!is_igd_vga_passthrough(&s->real_device)) {
XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
   " to passthrough IGD GFX.\n");
xen_host_pci_device_put(&s->real_device); 

return -1; 


}


  xen_host_pci_device_put(&s->real_device);
  return -1;
  }
+
+/* Register ISA bridge for passthrough GFX. */
+xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
  }

  /* Handle real device's MMIO/PIO BARs */
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 4356af4..703148e 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -51,4 +51,5 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
  #  define HVM_MAX_VCPUS 32
  #endif

+extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t 
gpu_dev_id);
  #endif /* QEMU_HW_XEN_H */


Either static or extern. You probably want to drop this declaration.



I just guess you're confused between igd_passthrough_isa_bridge_create() 
and xen_igd_passthrough_isa_bridge_create(), or am I wrong?


Note igd_passthrough_isa_bridge_create() is defined in the pc_piix.c file.

Thanks
Tiejun

Re: [Qemu-devel] [v8][RESEND][PATCH 00/10] xen: add Intel IGD passthrough

[Chen, Tiejun]

Ping...

Thanks
Tiejun

On 2015/6/5 16:44, Tiejun Chen wrote:

v8:

* Rebase on the latest qemu tree
* Cleanup one xen leftover in patch #3

v7:

* Instead of "-gfx_passthru" we'd like to make that a machine
   option, "-machine xxx,igd-passthru=on""
* try to make something as common shared by others like KvmGT in
   the future
* Just read those real value from host bridge pci
   configuration space when create host bridge then put in dev->config.

v6:

* Drop introducing a new machine specific to IGD passthrough
* Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
* Currently IGD drivers always need to access PCH by 1f.0. But we
   don't want to poke that directly to get ID, and although in real
   world different GPU should have different PCH. But actually the
   different PCH DIDs likely map to different PCH SKUs. We do the
   same thing for the GPU. For PCH, the different SKUs are going to
   be all the same silicon design and implementation, just different
   features turn on and off with fuses. The SW interfaces should be
   consistent across all SKUs in a given family (eg LPT). But just
   same features may not be supported.

   Most of these different PCH features probably don't matter to the
   Gfx driver, but obviously any difference in display port connections
   will so it should be fine with any PCH in case of passthrough.

   So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
   scenarios, 0x9cc3 for BDW(Broadwell).
* Drop igd write ops since its fine to emulate that, and we also shrink
   those igd read ops as necessary.
* Rebase and cleanup all patches.

v5:

* Simplify to make sure its really inherited from the standard one in patch #3
* Then drop the original patch #3

v4:

* Rebase on latest tree
* Drop patch #2
* Regenerate patches after Michael introduce patch #1
* We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
* Test: boot with a preinstalled winxp
   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc

v3:

* Drop patch #4
* Add one patch #1 from Michael
* Rebase
* In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.


Michael S. Tsirkin (1):
   i440fx: make types configurable at run-time

Tiejun Chen (9):
   pc_init1: pass parameters just with types
   piix: create host bridge to passthrough
   hw/pci-assign: split pci-assign.c
   xen, gfx passthrough: basic graphics passthrough support
   xen, gfx passthrough: retrieve VGA BIOS to work
   igd gfx passthrough: create a isa bridge
   xen, gfx passthrough: register a isa bridge
   xen, gfx passthrough: register host bridge specific to passthrough
   xen, gfx passthrough: add opregion mapping

  hw/core/machine.c |  20 +++
  hw/i386/Makefile.objs |   1 +
  hw/i386/kvm/pci-assign.c  |  82 +-
  hw/i386/pc_piix.c | 151 ++-
  hw/i386/pci-assign-load-rom.c |  93 
  hw/pci-host/piix.c|  91 +++-
  hw/xen/Makefile.objs  |   1 +
  hw/xen/xen-host-pci-device.c  |   5 +
  hw/xen/xen-host-pci-device.h  |   1 +
  hw/xen/xen_pt.c   |  32 
  hw/xen/xen_pt.h   |  21 ++-
  hw/xen/xen_pt_config_init.c   |  51 ++-
  hw/xen/xen_pt_graphics.c  | 272 ++
  include/hw/boards.h   |   1 +
  include/hw/i386/pc.h  |   8 +-
  include/hw/pci/pci-assign.h   |  27 
  include/hw/xen/xen.h  |   1 +
  qemu-options.hx   |   3 +
  vl.c  |  10 ++
  19 files changed, 780 insertions(+), 91 deletions(-)
  create mode 100644 hw/i386/pci-assign-load-rom.c
  create mode 100644 hw/xen/xen_pt_graphics.c
  create mode 100644 include/hw/pci/pci-assign.h

Thanks
Tiejun

Re: [Qemu-devel] [v8][PATCH 00/10] xen: add Intel IGD passthrough

[Chen, Tiejun]

Sorry, just please ignore this series because looks some patches are not 
sent out.


Tiejun

On 2015/6/5 16:31, Tiejun Chen wrote:

v8:

* Rebase on the latest qemu tree
* Cleanup one xen leftover in patch #3

v7:

* Instead of "-gfx_passthru" we'd like to make that a machine
   option, "-machine xxx,igd-passthru=on""
* try to make something as common shared by others like KvmGT in
   the future
* Just read those real value from host bridge pci
   configuration space when create host bridge then put in dev->config.

v6:

* Drop introducing a new machine specific to IGD passthrough
* Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
* Currently IGD drivers always need to access PCH by 1f.0. But we
   don't want to poke that directly to get ID, and although in real
   world different GPU should have different PCH. But actually the
   different PCH DIDs likely map to different PCH SKUs. We do the
   same thing for the GPU. For PCH, the different SKUs are going to
   be all the same silicon design and implementation, just different
   features turn on and off with fuses. The SW interfaces should be
   consistent across all SKUs in a given family (eg LPT). But just
   same features may not be supported.

   Most of these different PCH features probably don't matter to the
   Gfx driver, but obviously any difference in display port connections
   will so it should be fine with any PCH in case of passthrough.

   So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
   scenarios, 0x9cc3 for BDW(Broadwell).
* Drop igd write ops since its fine to emulate that, and we also shrink
   those igd read ops as necessary.
* Rebase and cleanup all patches.

v5:

* Simplify to make sure its really inherited from the standard one in patch #3
* Then drop the original patch #3

v4:

* Rebase on latest tree
* Drop patch #2
* Regenerate patches after Michael introduce patch #1
* We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
* Test: boot with a preinstalled winxp
   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc

v3:

* Drop patch #4
* Add one patch #1 from Michael
* Rebase
* In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.


Michael S. Tsirkin (1):
   i440fx: make types configurable at run-time

Tiejun Chen (9):
   pc_init1: pass parameters just with types
   piix: create host bridge to passthrough
   hw/pci-assign: split pci-assign.c
   xen, gfx passthrough: basic graphics passthrough support
   xen, gfx passthrough: retrieve VGA BIOS to work
   igd gfx passthrough: create a isa bridge
   xen, gfx passthrough: register a isa bridge
   xen, gfx passthrough: register host bridge specific to passthrough
   xen, gfx passthrough: add opregion mapping

  hw/core/machine.c |  20 +++
  hw/i386/Makefile.objs |   1 +
  hw/i386/kvm/pci-assign.c  |  82 +-
  hw/i386/pc_piix.c | 151 ++-
  hw/i386/pci-assign-load-rom.c |  93 
  hw/pci-host/piix.c|  91 +++-
  hw/xen/Makefile.objs  |   1 +
  hw/xen/xen-host-pci-device.c  |   5 +
  hw/xen/xen-host-pci-device.h  |   1 +
  hw/xen/xen_pt.c   |  32 
  hw/xen/xen_pt.h   |  21 ++-
  hw/xen/xen_pt_config_init.c   |  51 ++-
  hw/xen/xen_pt_graphics.c  | 272 ++
  include/hw/boards.h   |   1 +
  include/hw/i386/pc.h  |   8 +-
  include/hw/pci/pci-assign.h   |  27 
  include/hw/xen/xen.h  |   1 +
  qemu-options.hx   |   3 +
  vl.c  |  10 ++
  19 files changed, 780 insertions(+), 91 deletions(-)
  create mode 100644 hw/i386/pci-assign-load-rom.c
  create mode 100644 hw/xen/xen_pt_graphics.c
  create mode 100644 include/hw/pci/pci-assign.h

Thanks
Tiejun

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/6/2 17:17, Michael S. Tsirkin wrote:

On Tue, Jun 02, 2015 at 08:50:58AM +0800, Chen, Tiejun wrote:

On 2015/6/1 2:11, Michael S. Tsirkin wrote:

On Thu, Mar 19, 2015 at 09:01:27AM +0800, Chen, Tiejun wrote:

On 2015/3/18 18:21, Gerd Hoffmann wrote:

On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote:

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one. And we also just expose
a minimal real host bridge pci configuration subset.



+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+{0x08, 2},  /* revision id */
+{0x2c, 2},  /* sybsystem vendor id */
+{0x2e, 2},  /* sybsystem id */
+{0x50, 2},  /* SNB: processor graphics control register */
+{0x52, 2},  /* processor graphics control register */
+{0xa4, 4},  /* SNB: graphics base of stolen memory */
+{0xa8, 4},  /* SNB: base of GTT stolen memory */
+};


Hmm, no vendor/device id here?  Will the idg guest drivers happily read


These two emulated values are already fine.

And this is a long story. At the beginning, we were initially trying to
expose more,

+case 0x00:/* vendor id */
+case 0x02:/* device id */
+case 0x08:/* revision id */
+case 0x2c:/* sybsystem vendor id */
+case 0x2e:/* sybsystem id */
+case 0x50:/* SNB: processor graphics control register */
+case 0x52:/* processor graphics control register */
+case 0xa0:/* top of memory */
+case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */
+case 0x58:/* SNB: PAVPC Offset */
+case 0xa4:/* SNB: graphics base of stolen memory */
+case 0xa8:/* SNB: base of GTT stolen memory */

But this brought some discussions with Paolo and Michael, and then plus our
further experiment, now as everyone expect, this is a minimal real host
bridge pci configuration subset which we need to expose.


graphics control registers from i440fx even though this chipset never
existed in a igd variant?

[ just wondering, if it works that way fine, it certainly makes things


Yes, it works fine.


   easier for the firmware which uses host bridge pci id to figure
   whenever it is running @ i440fx or q35 ].


+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+uint32_t val = 0;
+int rc, i, num;
+int pos, len;
+
+num = ARRAY_SIZE(igd_host_bridge_infos);
+for (i = 0; i < num; i++) {
+pos = igd_host_bridge_infos[i].offset;
+len = igd_host_bridge_infos[i].len;
+rc = host_pci_config_read(pos, len, val);
+if (rc) {
+return -ENODEV;
+}
+pci_default_write_config(pci_dev, pos, val, len);
+}
+
+return 0;
+}


Nothing i440fx specific in here, just copying some host bridge pci
config space bits.  I guess we can sub-class the q35 host bridge the
same way and even reuse the init function?


This is our another discussion long time ago :)

Currently Xen don't run with q35. If I remember those discussions correctly,
something is still restricted to Windows.




+#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"


One xen leftover slipped through here.


Fixed.


So should I expect v8 then?



For this revision we just had only this valuable comment, and that is just
about to rename a macro, so I think its not worth resend this, right?

If I'm wrong let me know :)

Thanks
Tiejun


I didn't follow closely so I have no idea how valuable was the last
round of feedback, but when you say "Fixed" don't you mean "will be


It should be, but in this revision, I just received this sole comment 
until now so I mean its not worth resending a new review just with this 
fix :)


Anyway, its not big deal, just let me send this out as you expect.

Thanks
Tiejun


fixed in the next revision of the patchset"?

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/6/1 2:11, Michael S. Tsirkin wrote:

On Thu, Mar 19, 2015 at 09:01:27AM +0800, Chen, Tiejun wrote:

On 2015/3/18 18:21, Gerd Hoffmann wrote:

On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote:

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one. And we also just expose
a minimal real host bridge pci configuration subset.



+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+{0x08, 2},  /* revision id */
+{0x2c, 2},  /* sybsystem vendor id */
+{0x2e, 2},  /* sybsystem id */
+{0x50, 2},  /* SNB: processor graphics control register */
+{0x52, 2},  /* processor graphics control register */
+{0xa4, 4},  /* SNB: graphics base of stolen memory */
+{0xa8, 4},  /* SNB: base of GTT stolen memory */
+};


Hmm, no vendor/device id here?  Will the idg guest drivers happily read


These two emulated values are already fine.

And this is a long story. At the beginning, we were initially trying to
expose more,

+case 0x00:/* vendor id */
+case 0x02:/* device id */
+case 0x08:/* revision id */
+case 0x2c:/* sybsystem vendor id */
+case 0x2e:/* sybsystem id */
+case 0x50:/* SNB: processor graphics control register */
+case 0x52:/* processor graphics control register */
+case 0xa0:/* top of memory */
+case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */
+case 0x58:/* SNB: PAVPC Offset */
+case 0xa4:/* SNB: graphics base of stolen memory */
+case 0xa8:/* SNB: base of GTT stolen memory */

But this brought some discussions with Paolo and Michael, and then plus our
further experiment, now as everyone expect, this is a minimal real host
bridge pci configuration subset which we need to expose.


graphics control registers from i440fx even though this chipset never
existed in a igd variant?

[ just wondering, if it works that way fine, it certainly makes things


Yes, it works fine.


   easier for the firmware which uses host bridge pci id to figure
   whenever it is running @ i440fx or q35 ].


+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+uint32_t val = 0;
+int rc, i, num;
+int pos, len;
+
+num = ARRAY_SIZE(igd_host_bridge_infos);
+for (i = 0; i < num; i++) {
+pos = igd_host_bridge_infos[i].offset;
+len = igd_host_bridge_infos[i].len;
+rc = host_pci_config_read(pos, len, val);
+if (rc) {
+return -ENODEV;
+}
+pci_default_write_config(pci_dev, pos, val, len);
+}
+
+return 0;
+}


Nothing i440fx specific in here, just copying some host bridge pci
config space bits.  I guess we can sub-class the q35 host bridge the
same way and even reuse the init function?


This is our another discussion long time ago :)

Currently Xen don't run with q35. If I remember those discussions correctly,
something is still restricted to Windows.




+#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"


One xen leftover slipped through here.


Fixed.


So should I expect v8 then?



For this revision we just had only this valuable comment, and that is 
just about to rename a macro, so I think its not worth resend this, right?


If I'm wrong let me know :)

Thanks
Tiejun

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/3/26 16:15, Michael S. Tsirkin wrote:

On Thu, Mar 26, 2015 at 03:57:18PM +0800, Chen, Tiejun wrote:

Michael and Gerd,

I don't see any more comments for this revision, so what's next that I
should do?

Thanks
Tiejun

On 2015/3/19 9:01, Chen, Tiejun wrote:


It's only been a week, and we are busy finalizing 2.3.  So please give
people time to review.


Ping

Thanks
Tiejun

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

Perhaps add "With qemu-xen-traditional IGD is always assumed and other
options than autodetect or explicit IGD will result in an error"?


Will do.




diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a8b08f2..4fd6310 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -325,7 +325,15 @@ static char **
libxl__build_device_model_args_old(libxl__gc *gc,
   flexarray_vappend(dm_args, "-net", "none", NULL);
   }
   if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+switch (b_info->u.hvm.gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+flexarray_append(dm_args, "-gfx_passthru");
+break;
+default:
+LOG(ERROR, "unsupported gfx_passthru_kind.\n");


Sorry, LOG should not get a \n like my example had, my fault.


Actually myself really should double check this.



With that if you resend the series with git send-email (so it doesn't
get whitespace mangled) I think we are good to go!



Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
complete to review all associated qemu patch set. Although that don't 
bring any change to our two patches on Xen side, I think we'd better 
merge these patches until qemu patches are really applied into qemu 
tree. So I will send this series again until we can really consume this 
with qemu upstream, right?


BTW, I really appreciate your all comments in this thread.

Thanks
Tiejun

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/30 17:19, Ian Campbell wrote:

On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:

Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do
it now,

@@ -326,6 +326,10 @@ static char **
libxl__build_device_model_args_old(libxl__gc *gc,
   }
   if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
   flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->u.hvm.gfx_passthru_kind >
+LIBXL_GFX_PASSTHRU_KIND_IGD)
+LOG(ERROR, "unsupported device type for
\"gfx_passthru\".\n");
+return NULL;


I'd rather not encode any ordering constraints if we don't have to. I
think this is preferable:

   if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
switch (b_info->u.hvm.gfx_passthru_kind) {
case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
case LIBXL_GFX_PASSTHRU_KIND_IGD:
flexarray_append(dm_args, "-gfx_passthru");
break;
default:
LOG(ERROR, "unsupported gfx_passthru_kind.\n");
return NULL;
}
  }

(notice that the error message above doesn't refer to the xl specific
option naming).



Sorry for this delay response.

This looks reasonable and I regenerate this patch based on this comment:

 libxl: introduce gfx_passthru_kind

Although we already have 'gfx_passthru' in b_info, this doesn't suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
gfx_passthru = 1=> sets build_info.u.gfx_passthru to true and
   build_info.u.gfx_passthru_kind to DEFAULT
gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to true
   and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

And now "gfx_passthru" is supported both with the qemu-xen-traditional
device-model and upstream qemu-xen device-model. But when given as a
string this option describes the type of device to enable. Note this
behavior is only supported with the upstream qemu-xen device-model.

Signed-off-by: Tiejun Chen 
---
 docs/man/xl.cfg.pod.5   | 34 +
 tools/libxl/libxl.h |  6 ++
 tools/libxl/libxl_dm.c  | 46 
+

 tools/libxl/libxl_types.idl |  6 ++
 tools/libxl/xl_cmdimpl.c| 14 --
 5 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..dfde92d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L above.
 devices passed through to this VM. See L
 above.

-=item B
+=item B

 Enable graphics device PCI passthrough. This option makes an assigned
 PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters

 L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
 for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B option describes the type
+of device to enable. Note this behavior is only supported with the upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B
+
+Disables graphics device PCI passthrough.
+
+=item B, B
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but forcing the type
+of device to Intel Graphics Device.
+
+=back

 Note that some graphics adapters (AMD/ATI cards, for example) do not
 necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 100644
--- a/tools/libxl/lib

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/27 17:54, Ian Campbell wrote:

On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:

On 2015/3/26 18:06, Ian Campbell wrote:

On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:

Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
and whoever adds a new type will have to remember to add a check for
qemu-trad then.



When we really have to introduce a new type, this means we probably need
to change something inside qemu codes. So a new type should just go into
that table to support qemu upstream since now we shouldn't refactor
anything in qemu-xen-traditional, right?


We'd want to error out on attempts to use qemu-xen-trad with non-IGD
passthru.



On qemu-xen-traditional side, we always recognize this as BOOLEAN,

  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {

  flexarray_append(dm_args, "-gfx_passthru");

  }

Additionally, this is also clarified explicitly in manpage, and
especially we don't change this behavior now, so I'm just wondering why
we should do this :)


If someone does gfx_passthru = "foobar" and device_model_version =
"qemu-xen-traditional" in their xl config then it would be rather mean
of us to silently enable IGD passthru for them instead. When this occurs
libxl should notice and fail.

IGD is currently the only option, so this code would be needed when
someone adds "foobar" support.



Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do 
it now,


@@ -326,6 +326,10 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,

 }
 if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
 flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->u.hvm.gfx_passthru_kind >
+LIBXL_GFX_PASSTHRU_KIND_IGD)
+LOG(ERROR, "unsupported device type for 
\"gfx_passthru\".\n");

+return NULL;
 }
 } else {
 if (!sdl && !vnc)


Thanks
Tiejun

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/26 18:06, Ian Campbell wrote:

On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:

Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
and whoever adds a new type will have to remember to add a check for
qemu-trad then.



When we really have to introduce a new type, this means we probably need
to change something inside qemu codes. So a new type should just go into
that table to support qemu upstream since now we shouldn't refactor
anything in qemu-xen-traditional, right?


We'd want to error out on attempts to use qemu-xen-trad with non-IGD
passthru.



On qemu-xen-traditional side, we always recognize this as BOOLEAN,

if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 

flexarray_append(dm_args, "-gfx_passthru"); 


}

Additionally, this is also clarified explicitly in manpage, and 
especially we don't change this behavior now, so I'm just wondering why 
we should do this :)


Thanks
Tiejun

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/3/26 16:15, Michael S. Tsirkin wrote:

On Thu, Mar 26, 2015 at 03:57:18PM +0800, Chen, Tiejun wrote:

Michael and Gerd,

I don't see any more comments for this revision, so what's next that I
should do?

Thanks
Tiejun

On 2015/3/19 9:01, Chen, Tiejun wrote:


It's only been a week, and we are busy finalizing 2.3.  So please give


Oh, understood.

Thanks
Tiejun

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

Michael and Gerd,

I don't see any more comments for this revision, so what's next that I 
should do?


Thanks
Tiejun

On 2015/3/19 9:01, Chen, Tiejun wrote:

On 2015/3/18 18:21, Gerd Hoffmann wrote:

On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote:

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one. And we also just expose
a minimal real host bridge pci configuration subset.



+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+{0x08, 2},  /* revision id */
+{0x2c, 2},  /* sybsystem vendor id */
+{0x2e, 2},  /* sybsystem id */
+{0x50, 2},  /* SNB: processor graphics control register */
+{0x52, 2},  /* processor graphics control register */
+{0xa4, 4},  /* SNB: graphics base of stolen memory */
+{0xa8, 4},  /* SNB: base of GTT stolen memory */
+};


Hmm, no vendor/device id here?  Will the idg guest drivers happily read


These two emulated values are already fine.

And this is a long story. At the beginning, we were initially trying to
expose more,

+case 0x00:/* vendor id */
+case 0x02:/* device id */
+case 0x08:/* revision id */
+case 0x2c:/* sybsystem vendor id */
+case 0x2e:/* sybsystem id */
+case 0x50:/* SNB: processor graphics control register */
+case 0x52:/* processor graphics control register */
+case 0xa0:/* top of memory */
+case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */
+case 0x58:/* SNB: PAVPC Offset */
+case 0xa4:/* SNB: graphics base of stolen memory */
+case 0xa8:/* SNB: base of GTT stolen memory */

But this brought some discussions with Paolo and Michael, and then plus
our further experiment, now as everyone expect, this is a minimal real
host bridge pci configuration subset which we need to expose.


graphics control registers from i440fx even though this chipset never
existed in a igd variant?

[ just wondering, if it works that way fine, it certainly makes things


Yes, it works fine.


   easier for the firmware which uses host bridge pci id to figure
   whenever it is running @ i440fx or q35 ].


+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+uint32_t val = 0;
+int rc, i, num;
+int pos, len;
+
+num = ARRAY_SIZE(igd_host_bridge_infos);
+for (i = 0; i < num; i++) {
+pos = igd_host_bridge_infos[i].offset;
+len = igd_host_bridge_infos[i].len;
+rc = host_pci_config_read(pos, len, val);
+if (rc) {
+return -ENODEV;
+}
+pci_default_write_config(pci_dev, pos, val, len);
+}
+
+return 0;
+}


Nothing i440fx specific in here, just copying some host bridge pci
config space bits.  I guess we can sub-class the q35 host bridge the
same way and even reuse the init function?


This is our another discussion long time ago :)

Currently Xen don't run with q35. If I remember those discussions
correctly, something is still restricted to Windows.




+#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE
"xen-igd-passthrough-i440FX"


One xen leftover slipped through here.


Fixed.

Thanks
Tiejun

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/25 18:32, Ian Campbell wrote:

On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote:


+But when given as a string the B option describes the type
+of device to enable. Not this behavior is only supported with upstream


"Note" and "the upstream..."


Fixed.




+=item "igd"
+
+Enables graphics device PCI passthrough but force set the type of device
+with the Intel Graphics Device.


"but force set the type" doesn't scan very well. "... forcing the type
of device to Intel Graphics Device" works I think.


Fine to me as well.




I understand what you mean but that table just includes IGDs existed on
BDW and HSW. Because in the case of qemu upstream we're just covering
these platforms, and with our discussion we don't have any plan to add
those legacy platforms in the future. But qemu-xen-traditional still
covers those platforms. So I'm afraid its not good to check this with
that table as well.


Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
and whoever adds a new type will have to remember to add a check for
qemu-trad then.



When we really have to introduce a new type, this means we probably need 
to change something inside qemu codes. So a new type should just go into 
that table to support qemu upstream since now we shouldn't refactor 
anything in qemu-xen-traditional, right?


Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/25 18:26, Ian Campbell wrote:

On Wed, 2015-03-25 at 09:18 +0800, Chen, Tiejun wrote:

Actually my problem is that, I need to add a new parameter, 'flag', like
this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools
can't be compiled successfully. Or maybe you're suggesting I may isolate
this file while building tools, right?


I answered this in my original reply:
  it is acceptable for the new
 parameter to not be plumbed up to the Python bindings until someone
 comes along with a requirement to use it from Python. IOW you can just
 pass whatever the nop value is for the new argument.



Yes, I knew this but I'm just getting a little confusion again after 
we're starting to talk if xc.c is compiled...


And I will try to do this.


The "nop value" is whatever value should be passed to retain the current
behaviour.


Sure.

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:40, Ian Campbell wrote:

On Tue, 2015-03-24 at 18:31 +0800, Chen, Tiejun wrote:

NB, the libxl ones are broken and not even compiled right now, you can
ignore them.


Looks this is still compiled now.


xc is, xl is not, I am sure of that.


Indeed, you're right :)




I don't know what the semantics of flag is, if it is per SBDF then I


Yes, this should be a flag specific to a SBDF.

You know, I'm working to fix RMRR completely. Based on some discussion
about that design ( I assume you may read that thread previously :) ),
now we probably need to pass a flag to introduce our policy.


Unless you have a concrete requirement to expose RMRR via the Python
bindings to libxc (i.e. you know somebody is using them) then I think
you should not bother.


Actually my problem is that, I need to add a new parameter, 'flag', like 
this, xc_assign_device(xxx,xxx,flag). So if I don't refine xc.c, tools 
can't be compiled successfully. Or maybe you're suggesting I may isolate 
this file while building tools, right?




Making RMRR work via the (C) interface to libxl used by xl and libvirt
is sufficient for a new in tree feature.


Yeah.

Thanks
Tiejun



Ian.

suppose if you really wanted to expose this here then you would need to
invent some syntax for doing so.



Definitely.

When I finish this I will send you to review technically.

Again, really appreciate your clarification to me.

Thanks
Tiejun

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/24 22:50, Ian Campbell wrote:

On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:

Although we already have 'gfx_passthru' in b_info, this doesn' suffice




Fixed.


 ^t


after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to true and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false

true

You had it right in the code.


Fixed.




and build_info.u.gfx_passthru_kind to IGD



[snip]


+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@ through to this VM. See L above.
  devices passed through to this VM. See L
  above.

-=item B
+=item B


I think B is more accurate.


Yeah, it make more sense.




  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,12 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model. Note with the
+qemu-xen-traditional device-model this option is just treated as BOOLEAN
+actually, but with upstream qemu-xen device-model this option is extended
+to pass a specific device name to force work. Currently just 'igd' is
+defined to support Intel graphics device.


How about:

 When given as a boolean the B option either
 disables gfx passthru or enables autodetection.

 When given as a string the B option describes the
 type of passthru to enable.

 Valid options are:

 =over 4

 =item B

 Disables gfx_passthru.

 =item B, B

 Enables gfx_passthru and autodetects the type of device which is
 being used.

 =item "igd"

 Enables gfx_passthru of the Intel Graphics Device.

 =back

 Note: When used with the qemu-xen-traditional device model only
 IGD passthru is supported.

(do check my pod syntax, I'm mostly making it up!)


Please take a look at this,

@@ -671,7 +671,7 @@ through to this VM. See L above.
 devices passed through to this VM. See L
 above.

-=item B
+=item B

 Enable graphics device PCI passthrough. This option makes an assigned
 PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters

 L wiki page
 for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B option describes the type
+of device to enable. Not this behavior is only supported with upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B
+
+Disables graphics device PCI passthrough.
+
+=item B, B
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but force set the type of device
+with the Intel Graphics Device.
+
+=back

 Note that some graphics adapters (AMD/ATI cards, for example) do not
 necessarily require gfx_passthru option, so you can use the normal Xen



The note at the end makes me thing that perhaps something ought to check
this constraint in the qemu-xen-traditional case. It might be easiest to


I understand what you mean but that table just includes IGDs existed on 
BDW and HSW. Because in the case of qemu upstream we're just covering 
these platforms, and with our discussion we don't have any plan to add 
those legacy platforms in the future. But qemu-xen-traditional still 
covers those platforms. So I'm afraid its not good to check this with 
that table as well.



do it in libxl

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 18:20, Ian Campbell wrote:

On Tue, 2015-03-24 at 18:15 +0800, Chen, Tiejun wrote:

On 2015/3/24 17:51, Ian Campbell wrote:

On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:

All guys,



Thanks for your reply.


Sorry to bother you.

I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
pyxc_methods[] and pyxl_methods[]?


They are registered with the Python runtime, so they are called from
Python code. The first member of the struct is the pythonic function


Sorry I don't understanding this. So seems you mean instead of xl, this
is called by the third party user with python?


Yes, tools/python/xen is the python bindings for various C libraries
supported by Xen.


Thanks for your explanation.



NB, the libxl ones are broken and not even compiled right now, you can
ignore them.


Looks this is still compiled now.






name, e.g. from xc.c:
  { "domain_create",


Otherwise, often we always perform `xl create xxx' to create a VM. So I
think this should go into this flow like this,

xl_cmdtable.c:main_create()
|
+ create_domain()
|
+ libxl_domain_create_new()
|
+ do_domain_create()
|
+ 
Right?


Yes, xl is written in C not python so tools/python doesn't enter the
picture.


Yeah.






(PyCFunction)pyxc_domain_create,


So I don't see 'pyxc_domain_create' is called. Or I'm missing something...


Chances are that there are no intree users of this code any more, xend
would have used it at one time with something like:
import xen.lowlevel.xc
 xc = xen.lowlevel.xc.xc()
 xc.domain_create()
etc.




In my specific case, I'm trying to introduce a new flag to each a device
while assigning device. So this means I have to add a parameter, 'flag',
into

int xc_assign_device(
   xc_interface *xch,
   uint32_t domid,
   uint32_t machine_sbdf)

Then this is extended as

int xc_assign_device(
   xc_interface *xch,
   uint32_t domid,
   uint32_t machine_sbdf,
   uint32_t flag)

After this introduction, obviously I should cover all cases using
xc_assign_device(). And also I found this fallout goes into these two
files. For example, here pyxc_assign_device() is involved. Currently it
has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
should represent all pci devices with SBDF format, right?


It appears so, yes.


But I don't know exactly what rule should be complied to construct this
sort of flag into 'pci_str', or any reasonable idea to achieve my goal?


If it is non-trivial to fix them IMHO it is acceptable for the new
parameter to not be plumbed up to the Python bindings until someone
comes along with a requirement to use it from Python. IOW you can just
pass whatever the nop value is for the new argument.



Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But
I'm not sure if I'm breaking the existing usage since like I said, I
don't know what scenarios are using these methods.


Like I said in the paragraph above, if it is complicated then it is fine
to ignore this new parameter from Python.

I don't know what the semantics of flag is, if it is per SBDF then I


Yes, this should be a flag specific to a SBDF.

You know, I'm working to fix RMRR completely. Based on some discussion 
about that design ( I assume you may read that thread previously :) ), 
now we probably need to pass a flag to introduce our policy.



suppose if you really wanted to expose this here then you would need to
invent some syntax for doing so.



Definitely.

When I finish this I will send you to review technically.

Again, really appreciate your clarification to me.

Thanks
Tiejun

Re: [Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

On 2015/3/24 17:51, Ian Campbell wrote:

On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote:

All guys,



Thanks for your reply.


Sorry to bother you.

I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and
tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like
pyxc_methods[] and pyxl_methods[]?


They are registered with the Python runtime, so they are called from
Python code. The first member of the struct is the pythonic function


Sorry I don't understanding this. So seems you mean instead of xl, this 
is called by the third party user with python?



name, e.g. from xc.c:
 { "domain_create",


Otherwise, often we always perform `xl create xxx' to create a VM. So I 
think this should go into this flow like this,


xl_cmdtable.c:main_create()
|
+ create_domain()
|
+ libxl_domain_create_new()
|
+ do_domain_create()
|
+ 
Right?


   (PyCFunction)pyxc_domain_create,


So I don't see 'pyxc_domain_create' is called. Or I'm missing something...


   METH_VARARGS | METH_KEYWORDS, "\n"
   "Create a new domain.\n"
   " dom[int, 0]:Domain identifier to use (allocated if 
zero).\n"
   "Returns: [int] new domain identifier; -1 on error.\n" },
defines a method called domain_create, in the xen.lowlevel.xc namespace.


  And how should we call these approaches?


I'm not sure what you are asking here.


If you can give a real case to call this, things couldn't be better :)




In my specific case, I'm trying to introduce a new flag to each a device
while assigning device. So this means I have to add a parameter, 'flag',
into

int xc_assign_device(
  xc_interface *xch,
  uint32_t domid,
  uint32_t machine_sbdf)

Then this is extended as

int xc_assign_device(
  xc_interface *xch,
  uint32_t domid,
  uint32_t machine_sbdf,
  uint32_t flag)

After this introduction, obviously I should cover all cases using
xc_assign_device(). And also I found this fallout goes into these two
files. For example, here pyxc_assign_device() is involved. Currently it
has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str'
should represent all pci devices with SBDF format, right?


It appears so, yes.


But I don't know exactly what rule should be complied to construct this
sort of flag into 'pci_str', or any reasonable idea to achieve my goal?


If it is non-trivial to fix them IMHO it is acceptable for the new
parameter to not be plumbed up to the Python bindings until someone
comes along with a requirement to use it from Python. IOW you can just
pass whatever the nop value is for the new argument.



Should I extend this 'pci_str' like "Seg,bus,device,function:flag"? But 
I'm not sure if I'm breaking the existing usage since like I said, I 
don't know what scenarios are using these methods.


Thanks
Tiejun

[Qemu-devel] One question to lowlevel/xl/xl.c and lowlevel/xc/xc.c

[Chen, Tiejun]

All guys,

Sorry to bother you.

I have a question to two files, tools/python/xen/lowlevel/xc/xc.c and 
tools/python/xen/lowlevel/xl/xl.c. Who is a caller to those methods like 
pyxc_methods[] and pyxl_methods[]? And how should we call these approaches?


In my specific case, I'm trying to introduce a new flag to each a device 
while assigning device. So this means I have to add a parameter, 'flag', 
into


int xc_assign_device(
xc_interface *xch,
uint32_t domid,
uint32_t machine_sbdf)

Then this is extended as

int xc_assign_device(
xc_interface *xch,
uint32_t domid,
uint32_t machine_sbdf,
uint32_t flag)

After this introduction, obviously I should cover all cases using 
xc_assign_device(). And also I found this fallout goes into these two 
files. For example, here pyxc_assign_device() is involved. Currently it 
has two parameters, 'dom' and 'pci_str', and as I understand 'pci_str' 
should represent all pci devices with SBDF format, right?


But I don't know exactly what rule should be complied to construct this 
sort of flag into 'pci_str', or any reasonable idea to achieve my goal?


Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/20 18:11, Ian Campbell wrote:

On Fri, 2015-03-20 at 18:08 +0800, Chen, Tiejun wrote:

+if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf,
+
&b_info->u.hvm.gfx_passthru_kind)) {
+fprintf(stderr,
+"ERROR: invalid value \"%s\" for
\"gfx_passthru_kind\"\n",
+buf);
+exit (1);
+}
+}


This unnecessary bit seems to have crept back in.


Sorry to hit this again when I address other comments.



Don't forget to update the manpages if you haven't already.



Looks I need to update docs/man/xl.cfg.pod.5.

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

+case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+LOG(ERROR, "unable to detect required gfx_passthru_kind");


In this case you will now have logged twice. I'd suggest logging only
here and not in the helper.


+default:


And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
since it would indicate some sort of corruption. So, I would drop the
logging on failure in libxl__detect_gfx_passthru_kind and here do:
case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
LOG(ERROR, "unable to detect required gfx_passthru_kind");
return NULL;
default:
LOG(ERROR, "invalid value for gfx_passthru_kind");
return NULL;




Okay, I update this patch. If this is fine to you I'd like to send this 
whole series.


---
 tools/libxl/libxl.h  |  6 ++
 tools/libxl/libxl_dm.c   | 36 +---
 tools/libxl/libxl_internal.h |  4 
 tools/libxl/libxl_types.idl  |  6 ++
 tools/libxl/xl_cmdimpl.c | 23 +--
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);

 #define LIBXL_HAVE_PSR_MBM 1
 #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..bf72103 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,22 @@ static char *dm_spice_options(libxl__gc *gc,
 return opt;
 }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+const libxl_domain_config *guest_config)
+{
+const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+return b_info->u.hvm.gfx_passthru_kind;
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+return LIBXL_GFX_PASSTHRU_KIND_IGD;
+}
+
+return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
 const char *dm, int guest_domid,
 const libxl_domain_config 
*guest_config,
@@ -710,9 +726,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -757,6 +770,23 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+enum libxl_gfx_passthru_kind gfx_passthru_kind =
+libxl__detect_gfx_passthru_kind(gc, 
guest_config);

+switch (gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+break;
+case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+LOG(ERROR, "unable to detect required gfx_passthru_kind");
+return NULL;
+default:
+LOG(ERROR, "invalid value for gfx_passthru_kind");
+return NULL;
+}
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c97c62d..26a01cb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,10 @@ static inline void 
libxl__update_config_vtpm(libxl__gc *gc,

  */
 void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
 const libxl_bitmap *sptr);
+
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+const libxl_domain_config *d_config);
+
 #endif

 /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
 (3, "native_parav

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/19 18:44, Ian Campbell wrote:

On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:

This duplicates the code from above. I think this would be best done as:

static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
  if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
  return 0;

  if (libxl__is_igd_vga_passthru(gc, guest_config)) {
  b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
  return 0;
  }

  LOG(ERROR, "Unable to detect graphics passthru kind");
  return 1;
}

Then for the code in libxl__build_device_model_args_new:

   if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
   if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
return NULL
   switch (b_info->u.hvm.gfx_passthru_kind) {
   case LIBXL_GFX_PASSTHRU_KIND_IGD:
   machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
   break;
   default:
   LOG(ERROR, "unknown gfx_passthru_kind\n");
  return NULL;
   }
  }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.


+default:
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");


Please return an error here, as I've shown above.


Looks good and thanks, but here 'guest_config' is a const so we
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,

b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..605b17c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
   return opt;
   }

+static int
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+const libxl_domain_config *guest_config)
+{
+const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+return b_info->u.hvm.gfx_passthru_kind;
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+return LIBXL_GFX_PASSTHRU_KIND_IGD;
+}
+
+LOG(ERROR, "Unable to detect graphics passthru kind");
+return -1;


I think you can make this function return enum
libxl_gfx_passthrough_kind and then return
LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.


+}
+
   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
   const char *dm, int guest_domid,
   const libxl_domain_config
*guest_config,
@@ -427,7 +444,7 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
   const char *keymap = dm_keymap(guest_config);
   char *machinearg;
   flexarray_t *dm_args;
-int i, connection, devid;
+int i, connection, devid, gfx_passthru_kind;


Please declare this in the smallest necessary scope...
[...]

+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,


i.e. here.


+
guest_config);
+switch (gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+break;
+default:


With the suggestion to return KIND_DEFAULT if detection fails then I
think an extra case should be added:
 case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
 LOG(ERROR, "unable to detect required
 gfx_passthru_kind");
 return NULL;


+LOG(ERROR, "unknown gfx_passthru_kind\n");


I think LOG is supposed to not include the final \n.



Refactor again,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..05c8916 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
 return opt;
 }

+static enum libxl_gfx_passthru_kind
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+const libxl_domain_config *guest_config)
+{
+const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+return b_info->u.hvm.gfx_passthru_kind;
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+return LIBXL_GFX_PASSTHRU_KIND_IGD;
+}
+
+LOG(ERROR, "Unable to detect graphics passthru kind");
+return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

This duplicates the code from above. I think this would be best done as:

static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
 if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
 return 0;

 if (libxl__is_igd_vga_passthru(gc, guest_config)) {
 b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
 return 0;
 }

 LOG(ERROR, "Unable to detect graphics passthru kind");
 return 1;
}

Then for the code in libxl__build_device_model_args_new:

  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
  if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
   return NULL
  switch (b_info->u.hvm.gfx_passthru_kind) {
  case LIBXL_GFX_PASSTHRU_KIND_IGD:
  machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
  break;
  default:
  LOG(ERROR, "unknown gfx_passthru_kind\n");
 return NULL;
  }
 }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.


+default:
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");


Please return an error here, as I've shown above.


Looks good and thanks, but here 'guest_config' is a const so we 
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,


b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..605b17c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
 return opt;
 }

+static int
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+const libxl_domain_config *guest_config)
+{
+const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+return b_info->u.hvm.gfx_passthru_kind;
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+return LIBXL_GFX_PASSTHRU_KIND_IGD;
+}
+
+LOG(ERROR, "Unable to detect graphics passthru kind");
+return -1;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
 const char *dm, int guest_domid,
 const libxl_domain_config 
*guest_config,
@@ -427,7 +444,7 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 const char *keymap = dm_keymap(guest_config);
 char *machinearg;
 flexarray_t *dm_args;
-int i, connection, devid;
+int i, connection, devid, gfx_passthru_kind;
 uint64_t ram_size;
 const char *path, *chardev;

@@ -710,9 +727,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -757,6 +771,20 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
+ 
guest_config);

+switch (gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+break;
+default:
+LOG(ERROR, "unknown gfx_passthru_kind\n");
+return NULL;
+}
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);




diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c97c62d..8912421 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,11 @@ static inline void
libxl__update_config_vtpm(libxl__gc *gc,
*/
   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
   const libxl_bitmap *sptr);
+
+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND


No need for this ifdef.


Removed.

Thanks
Tiejun

Re: [Qemu-devel] [v7][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/3/18 18:21, Gerd Hoffmann wrote:

On Mi, 2015-03-18 at 17:06 +0800, Tiejun Chen wrote:

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one. And we also just expose
a minimal real host bridge pci configuration subset.



+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+{0x08, 2},  /* revision id */
+{0x2c, 2},  /* sybsystem vendor id */
+{0x2e, 2},  /* sybsystem id */
+{0x50, 2},  /* SNB: processor graphics control register */
+{0x52, 2},  /* processor graphics control register */
+{0xa4, 4},  /* SNB: graphics base of stolen memory */
+{0xa8, 4},  /* SNB: base of GTT stolen memory */
+};


Hmm, no vendor/device id here?  Will the idg guest drivers happily read


These two emulated values are already fine.

And this is a long story. At the beginning, we were initially trying to 
expose more,


+case 0x00:/* vendor id */
+case 0x02:/* device id */
+case 0x08:/* revision id */
+case 0x2c:/* sybsystem vendor id */
+case 0x2e:/* sybsystem id */
+case 0x50:/* SNB: processor graphics control register */
+case 0x52:/* processor graphics control register */
+case 0xa0:/* top of memory */
+case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */
+case 0x58:/* SNB: PAVPC Offset */
+case 0xa4:/* SNB: graphics base of stolen memory */
+case 0xa8:/* SNB: base of GTT stolen memory */

But this brought some discussions with Paolo and Michael, and then plus 
our further experiment, now as everyone expect, this is a minimal real 
host bridge pci configuration subset which we need to expose.



graphics control registers from i440fx even though this chipset never
existed in a igd variant?

[ just wondering, if it works that way fine, it certainly makes things


Yes, it works fine.


   easier for the firmware which uses host bridge pci id to figure
   whenever it is running @ i440fx or q35 ].


+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+uint32_t val = 0;
+int rc, i, num;
+int pos, len;
+
+num = ARRAY_SIZE(igd_host_bridge_infos);
+for (i = 0; i < num; i++) {
+pos = igd_host_bridge_infos[i].offset;
+len = igd_host_bridge_infos[i].len;
+rc = host_pci_config_read(pos, len, val);
+if (rc) {
+return -ENODEV;
+}
+pci_default_write_config(pci_dev, pos, val, len);
+}
+
+return 0;
+}


Nothing i440fx specific in here, just copying some host bridge pci
config space bits.  I guess we can sub-class the q35 host bridge the
same way and even reuse the init function?


This is our another discussion long time ago :)

Currently Xen don't run with q35. If I remember those discussions 
correctly, something is still restricted to Windows.





+#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "xen-igd-passthrough-i440FX"


One xen leftover slipped through here.


Fixed.

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

I think the _libxl_ message needs to be just "Unable to detect graphics
passthru kind". i.e. it can't/shouldn't reference anything to do with xl
config options etc (which would make no sense if libvirt was being
used).

That's not very user friendly though, so you may want to consider adding
a new specific error code for this case and returning it here, such that
xl or libvirt can then give a more comprehensible error message.


But,
args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
  guest_config, d_state, NULL);
|
	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
guest_config, state, dm_state_fd);

|
+  Currently we always return "NULL" inside.

if (!args) {
ret = ERROR_FAIL;
goto out;
}

So I'm not very sure how to pass this new specific error code to xl/libvirt.




So looks the whole policy should be something like this,

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ skip_vfb:
   xlu_cfg_replace_string (config, "spice_streaming_video",
   &b_info->u.hvm.spice.streaming_video, 0);
   xlu_cfg_get_defbool(config, "nographic",
&b_info->u.hvm.nographic, 0);
-xlu_cfg_get_defbool(config, "gfx_passthru",
-&b_info->u.hvm.gfx_passthru, 0);
+if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf,
+
&b_info->u.hvm.gfx_passthru_kind)) {
+fprintf(stderr,
+"ERROR: invalid value \"%s\" for
\"gfx_passthru\"\n",
+buf);
+exit (1);
+}
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+}


Up to here is fine.


Thanks but I guess I'd better to paste this whole patch to avoid I'm 
still missing something :)


---
 tools/libxl/libxl.h  |  6 ++
 tools/libxl/libxl_dm.c   | 25 ++---
 tools/libxl/libxl_internal.h |  5 +
 tools/libxl/libxl_types.idl  |  6 ++
 tools/libxl/xl_cmdimpl.c | 14 --
 5 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);

 #define LIBXL_HAVE_PSR_MBM 1
 #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..045d48a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -757,6 +754,28 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+switch (b_info->u.hvm.gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);

+} else {
+LOG(ERROR, "Unable to detect graphics passthru kind,"
+" please set gfx_passthru_kind. See xl.cfg(5) 
for more"

+" information.\n");
+return NULL;
+}
+break;
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+break;
+default:
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+break;
+}
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

If I remember the context correctly this is in the autodetect case,
so I think shouldn't mention IGD. Something like "Unable to detect
graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
for more


s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
'gfx_passthru_kind' from 'gfx_passthru'.


I think you have it backwards.

In the case here gfx_passthru=1 has been set by the user, but
gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
failed.

So if the user wants to make progress they should set gfx_passthru_kind
to whatever type of passthrough they were trying to do.


Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
parsed from 'gfx_passthru', right?




Alternatively I suppose you could recommend removing gfx_passthru=1 (or


I'm still keep that currently.


changing to=0), but given they've set =1 that doesn't seem to be the
most productive suggestion.



So looks the whole policy should be something like this,

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..5518759 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,27 @@ skip_vfb:
 xlu_cfg_replace_string (config, "spice_streaming_video",
 &b_info->u.hvm.spice.streaming_video, 0);
 xlu_cfg_get_defbool(config, "nographic", 
&b_info->u.hvm.nographic, 0);

-xlu_cfg_get_defbool(config, "gfx_passthru",
-&b_info->u.hvm.gfx_passthru, 0);
+if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf,
+ 
&b_info->u.hvm.gfx_passthru_kind)) {

+fprintf(stderr,
+"ERROR: invalid value \"%s\" for 
\"gfx_passthru\"\n",

+buf);
+exit (1);
+}
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+}
+if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf,
+ 
&b_info->u.hvm.gfx_passthru_kind)) {

+fprintf(stderr,
+"ERROR: invalid value \"%s\" for 
\"gfx_passthru_kind\"\n",

+buf);
+exit (1);
+}
+}
 switch (xlu_cfg_get_list_as_string_list(config, "serial",

&b_info->u.hvm.serial_list,
 1))

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/13 18:11, Ian Campbell wrote:

On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote:

I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to
return an error.


Then, looks I should do this,

LOG(ERROR, "No supported IGD to passthru," " or please force set
gfx_passthru=\"igd\".\n"); return NULL;


If I remember the context correctly this is in the autodetect case,
so I think shouldn't mention IGD. Something like "Unable to detect
graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
for more


s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
'gfx_passthru_kind' from 'gfx_passthru'.


information."




@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx,
libxl_mac *dst, libxl_mac *src);


[snip]


/* * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field
and * the libxl_gfx_passthru_kind enumeration is defined. */ #define
LIBXL_HAVE_GFX_PASSTHRU_KIND

Users don't care about the internal details, just about the existence
of the support.



Updated.

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
error.


Then, looks I should do this,

LOG(ERROR, "No supported IGD to passthru,"
" or please force set gfx_passthru=\"igd\".\n");
return NULL;




@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
libxl_mac *src);
   #define LIBXL_HAVE_PSR_MBM 1
   #endif

+/*
+ * LIBXL_HAVE_GFX_PASSTHRU_KIND
+ *
+ * If this is defined, the Graphic Device Passthrough Override is
supported.


Almost, please also explicitly name the type field as other similar
comments do for clarity.


Okay, maybe something is like this,

+/*
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU
+ *
+ * If this is defined, the IGD Graphic Device Passthrough is supported.
+ *
+ * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the
+ * libxl_device_pci field in the hvm is present in the pci_info structure
+ * fixup_ids[] which contains all supported IGD devices. So wwe use
+ * "igd-passthru=on" specify on the qemu command-line.
+ */
+#define LIBXL_HAVE_IGD_GFX_PASSTHRU 1
+


+ */


[snip]


and this should be in libxl_internal.h not here...


Okay.

I mistakenly understand we always have to expose this in libxl.h...




But looks libxl__gc{} is defined in the libxl_internal.h file... I guess


[snip]


+if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+if (l) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+} else {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+}


This is exactly the same as:
 libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);



Sure.

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

+
+if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
+if (libxl__is_igd_vga_passthru(gc, guest_config))
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+} else if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_IGD) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+} else {
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+}


I think this whole block should be inside an:
 if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
the semantics should be that kind is ignored unless passthru is enabled.

and then the if/else chain could become a switch perhaps?


Okay.



I'm unsure what we should do if kind==DEFAULT but
libxl__is_igd_vga_passthru fails. At a minimum a warning seems
desirable. I'm not sure if it should warrant a failure or not.


I think a warning message plus abort() should be enough, and then user 
can determine what's next,


Please take a look at this,

+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+switch (b_info->u.hvm.gfx_passthru_kind) {
+case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);

+} else {
+LOG(WARN, "No supported IGD to passthru,"
+" or please force set gfx_passthru=\"igd\".\n");
+abort();
+}
+break;
+case LIBXL_GFX_PASSTHRU_KIND_IGD:
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+break;
+default:
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+break;
+}
+}
+




+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index fc060c6..9a534cc 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
  device = fixup_ids[j].device;

  if (pt_vendor == vendor &&  pt_device == device)
-return 1;
+return true;
  }
  }

-return 0;
+return false;


Please fold this into the previous patch. (You may retain the ack I
gave).


Thanks for your catch.


  }

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..d64ad10 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
  (3, "native_paravirt"),
  ])

+libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
+(0, "default"),
+(1, "igd"),
+])
+
  # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
  libxl_timer_mode = Enumeration("timer_mode", [
  (-1, "unknown"),
@@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 ("spice",libxl_spice_info),

 ("gfx_passthru", libxl_defbool),
+   ("gfx_passthru_kind", 
libxl_gfx_passthru_kind),


Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
functionality is now available. There is a comment near the top
explaining why etc and a bunch of examples.

Note that the #define is for 3rd party code only, there is no need to
actually use it in either libxl or xl code.


Like this,

@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);

 #define LIBXL_HAVE_PSR_MBM 1
 #endif

+/*
+ * LIBXL_HAVE_GFX_PASSTHRU_KIND
+ *
+ * If this is defined, the Graphic Device Passthrough Override is 
supported.

+ */
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
  uint64_t *tsc_r);
 #endif

+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+const libxl_domain_config *d_config);
+#endif
+
 /* misc */

 /* Each of these sets or clears the flag according to whether the

But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
we need a conversion, or other idea?





 ("serial",   string),
 ("boot", string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 440db78..

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/9 18:17, Wei Liu wrote:

On Mon, Mar 09, 2015 at 02:45:36PM +0800, Chen, Tiejun wrote:
[...]



+exit (1);
+}
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (libxl_gfx_passthru_kind_from_string(buf, 
&b_info->u.hvm.gfx_passthru_kind)) {


Ditto.


+fprintf(stderr, "ERROR: invalid value \"%s\" for 
\"gfx_passthru\"\n",


Ditto.


So,

@@ -1959,13 +1959,15 @@ skip_vfb:
  } else if (i == 1) {
  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
  } else {
-fprintf(stderr, "ERROR: invalid value %ld for
\"gfx_passthru\"\n", l);
+fprintf(stderr, "ERROR: invalid value %ld for"
+" \"gfx_passthru\"\n", l);
  exit (1);
  }
  } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
-if (libxl_gfx_passthru_kind_from_string(buf,
&b_info->u.hvm.gfx_passthru_kind)) {
-fprintf(stderr, "ERROR: invalid value \"%s\" for
\"gfx_passthru\"\n",
-buf);
+if (libxl_gfx_passthru_kind_from_string(buf,
+ &b_info->u.hvm.gfx_passthru_kind)) {
+fprintf(stderr, "ERROR: invalid value \"%s\" for"
+" \"gfx_passthru\"\n", buf);
  exit (1);
  }
  libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);



Your changes are mangled by your email client. But we don't normally
split the format string so that it's easier to grep.


Yeah.



What we normally do is

  fprintf(stderr,
  "FORMAT STRING",
 a, b, c);

If format string still treads over 80 column it's fine. We can live
with that.



Understood.

Thanks
Tiejun

Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Chen, Tiejun]

On 2015/3/6 23:53, Konrad Rzeszutek Wilk wrote:

On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that


Could you also include in the cover letter an URL link to the latest
discussion on this?


Thanks for reminding. I really should do this.

https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02050.html

Thanks
Tiejun



Thank you.

a machine option, "-machine xxx,igd-passthru=on". This need
to bring a change on tool side.

After a discussion with Campbell, we'd like to construct a table to record
all IGD devices we can support. If we hit that table, we should pass that
option. And so we also introduce a new field of type, 'gfx_passthru_kind',
to cooperate with 'gfx_passthru' to cover all scenarios like this,

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to false and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD


Tiejun Chen (2):
   libxl: introduce libxl__is_igd_vga_passthru
   libxl: introduce gfx_passthru_kind

  tools/libxl/libxl_dm.c   |  13 +
  tools/libxl/libxl_internal.h |   2 ++
  tools/libxl/libxl_pci.c  | 124 

  tools/libxl/libxl_types.idl  |   6 ++
  tools/libxl/xl_cmdimpl.c |  19 +--
  5 files changed, 162 insertions(+), 2 deletions(-)

Thanks
Tiejun

___
Xen-devel mailing list
xen-de...@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 20:59, Wei Liu wrote:

On Fri, Mar 06, 2015 at 05:18:36PM +0800, Chen, Tiejun wrote:

On 2015/3/6 17:08, Tiejun Chen wrote:

Although we already have 'gfx_passthru' in b_info, this doesn' suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to false and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

Signed-off-by: Tiejun Chen 
---
  tools/libxl/libxl_dm.c  | 13 +
  tools/libxl/libxl_types.idl |  6 ++
  tools/libxl/xl_cmdimpl.c| 19 +--
  3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..780dd2a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c


Sorry, looks I'm missing to remove '-gfx_passthru' since this should be gone
in the case of qemu upstream,

@@ -710,9 +710,6 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
  } else {
  if (!sdl && !vnc) {
  flexarray_append(dm_args, "-nographic");

I will fold this into next revision after this review.



I think this change alone warrants a separate changeset. It should be
patch 0 of your series as a preparatory patch. Please remember to
elaborate in commit message why that hunk is not needed in upstream
QEMU.



What about this?

"-gfx_passthru" is just introduced to work for qemu-xen-traditional so 
we should get this away from libxl__build_device_model_args_new() in the 
case of qemu upstream.


Thanks
Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 20:55, Wei Liu wrote:

On Fri, Mar 06, 2015 at 05:08:23PM +0800, Tiejun Chen wrote:

Although we already have 'gfx_passthru' in b_info, this doesn' suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to false and

^
true?


I just simply pick up this from Campbell's comment but I agree you.




build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

Signed-off-by: Tiejun Chen 
---
  tools/libxl/libxl_dm.c  | 13 +
  tools/libxl/libxl_types.idl |  6 ++
  tools/libxl/xl_cmdimpl.c| 19 +--
  3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..780dd2a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -757,6 +757,19 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}


You can remove that {} around machinearg -- it's only one line after
`if'.


Okay.




+} else if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_IGD) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+} else {
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..e209460 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
  (3, "native_paravirt"),
  ])

+libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
+(0, "default"),
+(1, "igd"),
+], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
+


You don't need to set init_val if the default value is 0.


Got it.




  # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
  libxl_timer_mode = Enumeration("timer_mode", [
  (-1, "unknown"),
@@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 ("spice",libxl_spice_info),

 ("gfx_passthru", libxl_defbool),
+   ("gfx_passthru_kind", 
libxl_gfx_passthru_kind),

 
One space is enough, I think.


Okay.





 ("serial",   string),
 ("boot", string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 440db78..3f7fede 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,23 @@ skip_vfb:
  xlu_cfg_replace_string (config, "spice_streaming_video",
  &b_info->u.hvm.spice.streaming_video, 0);
  xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
-xlu_cfg_get_defbool(config, "gfx_passthru",
-&b_info->u.hvm.gfx_passthru, 0);
+if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+if (!l) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+} else if (i == 1) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+} else {
+fprintf(stderr, "ERROR: invalid value %ld for 
\"gfx_passthru\"\n", l);


Please wrap this line to be < 80 column.


+exit (1);
+}
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (libxl_gfx_pass

Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

[Chen, Tiejun]

On 2015/3/6 20:40, Wei Liu wrote:

On Fri, Mar 06, 2015 at 05:08:22PM +0800, Tiejun Chen wrote:

While working with qemu, IGD is a specific device in the case of pass through
so we need to identify that to handle more later. Here we define a table to
record all IGD types currently we can support. Also we need to introduce two
helper functions to get vendor and device ids to lookup that table.

Signed-off-by: Tiejun Chen 
---
  tools/libxl/libxl_internal.h |   2 +
  tools/libxl/libxl_pci.c  | 124 +++
  2 files changed, 126 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..8b952b8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t 
domid, libxl_device_pc
  _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int num);
  _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);
+_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config *d_config);

  /*- xswait: wait for a xenstore node to be suitable -*/

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..dc5a89e 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, 
libxl_device_pci *pcidev,
  return 0;
  }

+static unsigned long sysfs_dev_get_vendor(libxl__gc *gc,
+  libxl_device_pci *pcidev)


uint16_t?


+{
+char *pci_device_vendor_path =
+libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+   pcidev->domain, pcidev->bus, pcidev->dev,
+   pcidev->func);


Please use GCSPRINTF macro.


Okay.




+int read_items;
+unsigned long pci_device_vendor;


uint16_t?


Yes, I can but I don't see other similar helpers are doing this in this 
file :)




Same comments apply to _get_device function.


And especially, if we really set that as uint16_t,




+
+FILE *f = fopen(pci_device_vendor_path, "r");
+if (!f) {
+LOGE(ERROR,
+ "pci device "PCI_BDF" does not have vendor attribute",
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+return 0x;
+}
+read_items = fscanf(f, "0x%lx\n", &pci_device_vendor);


we have to refactor this as well,

read_items = fscanf(f, "0x%hx\n", &pci_device_vendor);

Right?


+fclose(f);
+if (read_items != 1) {
+LOGE(ERROR,
+ "cannot read vendor of pci device "PCI_BDF,
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+return 0x;
+}
+
+return pci_device_vendor;
+}
+


[...]


+/*
+ * Some devices may need some ways to work well. Here like IGD,
+ * we have to pass a specific option to qemu.
+ */
+int libxl__is_igd_vga_passthru(libxl__gc *gc,


bool.


Okay.




+   const libxl_domain_config *d_config)
+{
+unsigned int i, j, num = ARRAY_SIZE(fixup_ids);
+uint16_t vendor, device;
+
+for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+libxl_device_pci *pcidev = &d_config->pcidevs[i];
+
+for (j = 0 ; j < num ; j++) {
+vendor = fixup_ids[j].vendor;
+device = fixup_ids[j].device;
+
+if (sysfs_dev_get_vendor(gc, pcidev) == vendor &&
+sysfs_dev_get_device(gc, pcidev) == device)
+return 1;


Get vendor and device in outer loop to avoid wasting cpu cycles. :-)



Yeah.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream

[Chen, Tiejun]

On 2015/3/6 20:28, Wei Liu wrote:

On Fri, Mar 06, 2015 at 05:08:21PM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,igd-passthru=on". This need
to bring a change on tool side.

After a discussion with Campbell, we'd like to construct a table to record
all IGD devices we can support. If we hit that table, we should pass that
option. And so we also introduce a new field of type, 'gfx_passthru_kind',
to cooperate with 'gfx_passthru' to cover all scenarios like this,

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to false and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD


Tiejun Chen (2):
   libxl: introduce libxl__is_igd_vga_passthru
   libxl: introduce gfx_passthru_kind

  tools/libxl/libxl_dm.c   |  13 +
  tools/libxl/libxl_internal.h |   2 ++
  tools/libxl/libxl_pci.c  | 124 



It might be helpful to use --stat-width=72 or some other options to
limit the width of your diffstat. :-)



Will do.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

On 2015/3/6 17:08, Tiejun Chen wrote:

Although we already have 'gfx_passthru' in b_info, this doesn' suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

 gfx_passthru = 0=> sets build_info.u.gfx_passthru to false
 gfx_passthru = 1=> sets build_info.u.gfx_passthru to false and
build_info.u.gfx_passthru_kind to DEFAULT
 gfx_passthru = "igd"=> sets build_info.u.gfx_passthru to false
and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

Signed-off-by: Tiejun Chen 
---
  tools/libxl/libxl_dm.c  | 13 +
  tools/libxl/libxl_types.idl |  6 ++
  tools/libxl/xl_cmdimpl.c| 19 +--
  3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..780dd2a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c


Sorry, looks I'm missing to remove '-gfx_passthru' since this should be 
gone in the case of qemu upstream,


@@ -710,9 +710,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");

I will fold this into next revision after this review.

Thanks
Tiejun


@@ -757,6 +757,19 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+} else if (b_info->u.hvm.gfx_passthru_kind ==
+LIBXL_GFX_PASSTHRU_KIND_IGD) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+} else {
+LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..e209460 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
  (3, "native_paravirt"),
  ])

+libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
+(0, "default"),
+(1, "igd"),
+], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT")
+
  # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
  libxl_timer_mode = Enumeration("timer_mode", [
  (-1, "unknown"),
@@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 ("spice",libxl_spice_info),

 ("gfx_passthru", libxl_defbool),
+   ("gfx_passthru_kind", 
libxl_gfx_passthru_kind),

 ("serial",   string),
 ("boot", string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 440db78..3f7fede 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,23 @@ skip_vfb:
  xlu_cfg_replace_string (config, "spice_streaming_video",
  &b_info->u.hvm.spice.streaming_video, 0);
  xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 0);
-xlu_cfg_get_defbool(config, "gfx_passthru",
-&b_info->u.hvm.gfx_passthru, 0);
+if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+if (!l) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+} else if (i == 1) {
+libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+} else {
+fprintf(stderr, "ERROR: invalid value %ld for 
\"gfx_passthru\"\n", l);
+exit (1);
+}
+} else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+if (li

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

Campbell,

Are you free to look at my reply?

Thanks
Tiejun

On 2015/3/2 9:20, Chen, Tiejun wrote:

Here I just mean when Qemu realizes IGD is passed through but without
that appropriate option set, Qemu can post something to explicitly
notify user that this option is needed in his case. But it may be a lazy
idea.


In any case I think the additions of such warnings in qemu are a
separate to the discussion in this thread, so I propose to leave it
alone for now.


Okay.




So now I think I'd better go back handling this on Xen side with your
comments. As you said the Boolean doesn't suffice to indicate that IGD
workarounds are needed. So I think we can reuse that existing bool
'gfx_passthru'.

Firstly we can redefine this as string,


Unfortunately not since libxl's API guarantee requires older clients to
keep working, i.e. those who use libxl_defbool_set on this field.

Probably the best which can be done is to deprecate this field in favour
of a new one (the old field would need to be obeyed only if the new one
was set to its default value).

Probably an Enumeration would be better than a raw string here as well.

This approach doesn't allow for the possibility of multiple such
workarounds though. It's unclear to me if this matters or not.

The other option which I've mentioned is to leave gfx_passthru and have
libxl figure out which workarounds to enable based on the set of PCI
devices passed through. I guess you don't like that approach? (due to
the need to maintain the pci vid:did list?)


No, I like that approach currently :) Please see the below,





-   ("gfx_passthru",
libxl_defbool),
+   ("gfx_passthru", string),

Then

+
+if (libxl__is_igd_vga_passthru(gc, guest_config) ||


This is just working out that way that I already posted previously, and
here I paste this code fragment again,

+static const pci_info fixup_ids[] = {
+/* Intel HSW Classic */
+{0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */
+{0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */
+{0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */
+{0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */
+{0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */
+/* Intel HSW ULT */
+{0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */
+{0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */
+{0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */
+{0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */
+{0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */
+{0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */
+/* Intel HSW CRW */
+{0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */
+{0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */
+/* Intel HSW Server */
+{0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */
+/* Intel HSW SRVR */
+{0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */
+/* Intel BSW */
+{0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */
+{0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */
+{0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */
+{0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */
+{0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */
+{0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */
+{0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */
+{0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */
+{0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */
+{0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */
+{0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+/*
+ * Some devices may need some ways to work well. Here like IGD,
+ * we have to pass a specific option to qemu.
+ */
+int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config *d_config)
+{
+unsigned int i, j, num = ARRAY_SIZE(fixup_ids);
+uint16_t vendor, device;
+
+for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+libxl_device_pci *pcidev = &d_config->pcidevs[i];
+
+for (j = 0 ; j < num ; j++) {
+vendor = fixup_ids[j].vendor;
+device = fixup_ids[j].device;
+
+if (sysfs_dev_get_vendor(gc, pcidev) == vendor &&
+sysfs_dev_get_device(gc, pcidev) == device)
+return 1;
+}
+}
+
+return 0;
+}
+

Is this expected?


+(b_info->u.hvm.gfx_passthru &&
+ strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) {


But as you mentioned previously,

"
You might like to optionally consider add a forcing option somehow so
that people with new devices not in the list can control things without
the need to recompile (e.g. gfx_passthru_kind_override?).
"

Here I was trying to convert "gfx_passthru" to address this thing.
According to your comment right now, you prefer we should introduce a
new field instead of the original 'gfx_passthru' to enumerate such a
type. So what about this?

libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [
 (0, "unknown"),
 (1, "igd&

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

Here I just mean when Qemu realizes IGD is passed through but without
that appropriate option set, Qemu can post something to explicitly
notify user that this option is needed in his case. But it may be a lazy
idea.


In any case I think the additions of such warnings in qemu are a
separate to the discussion in this thread, so I propose to leave it
alone for now.


Okay.




So now I think I'd better go back handling this on Xen side with your
comments. As you said the Boolean doesn't suffice to indicate that IGD
workarounds are needed. So I think we can reuse that existing bool
'gfx_passthru'.

Firstly we can redefine this as string,


Unfortunately not since libxl's API guarantee requires older clients to
keep working, i.e. those who use libxl_defbool_set on this field.

Probably the best which can be done is to deprecate this field in favour
of a new one (the old field would need to be obeyed only if the new one
was set to its default value).

Probably an Enumeration would be better than a raw string here as well.

This approach doesn't allow for the possibility of multiple such
workarounds though. It's unclear to me if this matters or not.

The other option which I've mentioned is to leave gfx_passthru and have
libxl figure out which workarounds to enable based on the set of PCI
devices passed through. I guess you don't like that approach? (due to
the need to maintain the pci vid:did list?)


No, I like that approach currently :) Please see the below,





-   ("gfx_passthru", libxl_defbool),
+   ("gfx_passthru", string),

Then

+
+if (libxl__is_igd_vga_passthru(gc, guest_config) ||


This is just working out that way that I already posted previously, and 
here I paste this code fragment again,


+static const pci_info fixup_ids[] = {
+/* Intel HSW Classic */
+{0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */
+{0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */
+{0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */
+{0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */
+{0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */
+/* Intel HSW ULT */
+{0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */
+{0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */
+{0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */
+{0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */
+{0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */
+{0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */
+/* Intel HSW CRW */
+{0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */
+{0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */
+/* Intel HSW Server */
+{0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */
+/* Intel HSW SRVR */
+{0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */
+/* Intel BSW */
+{0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */
+{0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */
+{0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */
+{0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */
+{0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */
+{0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */
+{0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */
+{0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */
+{0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */
+{0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */
+{0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+/*
+ * Some devices may need some ways to work well. Here like IGD,
+ * we have to pass a specific option to qemu.
+ */
+int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config *d_config)
+{
+unsigned int i, j, num = ARRAY_SIZE(fixup_ids);
+uint16_t vendor, device;
+
+for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+libxl_device_pci *pcidev = &d_config->pcidevs[i];
+
+for (j = 0 ; j < num ; j++) {
+vendor = fixup_ids[j].vendor;
+device = fixup_ids[j].device;
+
+if (sysfs_dev_get_vendor(gc, pcidev) == vendor &&
+sysfs_dev_get_device(gc, pcidev) == device)
+return 1;
+}
+}
+
+return 0;
+}
+

Is this expected?


+(b_info->u.hvm.gfx_passthru &&
+ strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) {


But as you mentioned previously,

"
You might like to optionally consider add a forcing option somehow so
that people with new devices not in the list can control things without
the need to recompile (e.g. gfx_passthru_kind_override?).
"

Here I was trying to convert "gfx_passthru" to address this thing. 
According to your comment right now, you prefer we should introduce a 
new field instead of the original 'gfx_passthru' to enumerate such a 
type. So what about this?


libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [
(0, "unknown"),
(1, "igd"),
])

Then if we want to override this, just submit the following line into .cfg:

gfx_passthru_kind_override = "igd"

Thanks
Tiejun


+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+

Of course we need modify something el

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

Ian,

Just ping this, or do you think I should send this as a patch?

Thanks
Tiejun

On 2015/2/11 10:45, Chen, Tiejun wrote:

On 2015/2/9 19:05, Ian Campbell wrote:

On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote:


What about this?


I've not read the code in detail,since I'm travelling but from a quick
glance it looks to be implementing the sort of thing I meant, thanks.


Thanks for your time.



A couple of higher level comments:

I'd suggest to put the code for reading the vid/did into a helper
function so it can be reused.


Looks good.



You might like to optionally consider add a forcing option somehow so
that people with new devices not in the list can control things without
the need to recompile (e.g. gfx_passthru_kind_override?). Perhaps that
isn't needed for a first cut though and it would be a libxl API so
thought required.


What about 'gfx_passthru_force'? Because what we're doing is, we want to
make sure if we have a such a IGD that needs to workaround by posting a
parameter to qemu. So in case of non-listed devices we just need to
provide a bool to force this regardless of that real device.



I think it should probably log something at a lowish level when it has
autodetected IGD.



So I tried to rebase that according to your all comments,

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 98687bd..398d9da 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
  libxl_defbool_setdefault(&b_info->u.hvm.nographic, false);

  libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
+libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force,
false);

  break;
  case LIBXL_DOMAIN_TYPE_PV:
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..507034f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,6 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
  } else {
  if (!sdl && !vnc) {
  flexarray_append(dm_args, "-nographic");
@@ -757,6 +754,11 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg,
max_ram_below_4g);
  }
  }
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL;
i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..35ec5fc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc
*gc, uint32_t domid,
libxl_device_pci *pcidev, int num);
  _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);

+_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config
*d_config);
+
  /*- xswait: wait for a xenstore node to be suitable -*/

  typedef struct libxl__xswait_state libxl__xswait_state;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..07b9e22 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc,
libxl_device_pci *pcidev,
  return 0;
  }

+static unsigned long sysfs_dev_get_vendor(libxl__gc *gc,
+  libxl_device_pci *pcidev)
+{
+char *pci_device_vendor_path =
+libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+   pcidev->domain, pcidev->bus, pcidev->dev,
+   pcidev->func);
+int read_items;
+unsigned long pci_device_vendor;
+
+FILE *f = fopen(pci_device_vendor_path, "r");
+if (!f) {
+LOGE(ERROR,
+ "pci device "PCI_BDF" does not have vendor attribute",
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+return 0x;
+}
+read_items = fscanf(f, "0x%lx\n", &pci_device_vendor);
+fclose(f);
+if (read_items != 1) {
+LOGE(ERROR,
+ "cannot read vendor of pci device "PCI_BDF,
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/11 11:46, Peter Maydell wrote:

On 11 February 2015 at 02:50, Chen, Tiejun  wrote:

On 2015/2/11 10:03, Peter Maydell wrote:

The linux-headers/ directory contains header files which can only
validly be included if the host we're compiling on is Linux. Some
of them will cause compile failures on OSX or Windows if they
are in the include path. The idea of this patch is that the
standard-headers/ directory has "sanitized" header files which
have had the linux-specific types and includes stripped out.
So if we take the route this patch proposes we do need two
directories.



This confounds me since for instance, one of goals based on this patch is,
it exposes those Virtio devices ID definition to hw/virtio, instead of my
original patch, right? So without this sort of standard-hearders, how can we
compile virtio? Or you mean we still keep those original stuff in
include/hw/virtio*, but somehow update them once we execute that script
manually.


I'm confused about why you're confused. We have two basic
approaches we can take:

(1) What we do at the moment. There are headers defining the virtio
interface in include/hw/virtio, and these are basically manually
created and updated as necessary.

(2) What this patch is proposing. The headers defining virtio are
automatically copied into standard-headers/ and fixed up to make
them work with QEMU on all the hosts we support. This happens when
this script is run by a developer to update QEMU's headers based
on some new upstream kernel.


I guess this mean this patch should be extended to smooth something in 
include/hw/virtio* in some ways.




Personally I think that option 1 is more reliable and overall


Agreed.


less effort, since automatiing the fixups is hard and virtio
doesn't change very much.



So sounds my original patch is fine to you.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/11 10:03, Peter Maydell wrote:

On 11 February 2015 at 01:36, Chen, Tiejun  wrote:

On 2015/2/10 3:56, Michael S. Tsirkin wrote:


It doesn't make sense to copy values manually:
the only issue with getting headers from linux
seems to be dealing with linux/types, we
can easily fix that automatically while importing.

Signed-off-by: Michael S. Tsirkin 
---

FYI this is what I propose instead of the recently
suggested
  virtio: uniform virtio device IDs
we can then rework existing code to include these headers.

Will automatically bring in goodies as they arrive in linux.

This doesn't yet import virtio ccw header,
that won't be hard to add later.

   scripts/update-linux-headers.sh | 10 ++
   1 file changed, 10 insertions(+)

diff --git a/scripts/update-linux-headers.sh
b/scripts/update-linux-headers.sh
index c8e026d..0bd8437 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -76,4 +76,14 @@ else
   cp "$linux/COPYING" "$output/linux-headers"
   fi

+rm -rf "$output/standard-headers/linux"
+mkdir -p "$output/standard-headers/linux"



Shouldn't we add something in configure file to execute this automatically?


No. We want to run this script only when we're updating the
header files, which only happens when we have a valid
kernel source tree available and you're a developer doing
it as a specific action. configure is run by everybody and
should definitely not be doing header updates.


Or instead of creating 'standard-headers/, why can't we go that existing
linux-headers/?


The linux-headers/ directory contains header files which can only
validly be included if the host we're compiling on is Linux. Some
of them will cause compile failures on OSX or Windows if they
are in the include path. The idea of this patch is that the
standard-headers/ directory has "sanitized" header files which
have had the linux-specific types and includes stripped out.
So if we take the route this patch proposes we do need two
directories.



This confounds me since for instance, one of goals based on this patch 
is, it exposes those Virtio devices ID definition to hw/virtio, instead 
of my original patch, right? So without this sort of standard-hearders, 
how can we compile virtio? Or you mean we still keep those original 
stuff in include/hw/virtio*, but somehow update them once we execute 
that script manually.


Thanks
Tiejun

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/9 19:05, Ian Campbell wrote:

On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote:


What about this?


I've not read the code in detail,since I'm travelling but from a quick
glance it looks to be implementing the sort of thing I meant, thanks.


Thanks for your time.



A couple of higher level comments:

I'd suggest to put the code for reading the vid/did into a helper
function so it can be reused.


Looks good.



You might like to optionally consider add a forcing option somehow so
that people with new devices not in the list can control things without
the need to recompile (e.g. gfx_passthru_kind_override?). Perhaps that
isn't needed for a first cut though and it would be a libxl API so
thought required.


What about 'gfx_passthru_force'? Because what we're doing is, we want to 
make sure if we have a such a IGD that needs to workaround by posting a 
parameter to qemu. So in case of non-listed devices we just need to 
provide a bool to force this regardless of that real device.




I think it should probably log something at a lowish level when it has
autodetected IGD.



So I tried to rebase that according to your all comments,

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 98687bd..398d9da 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
 libxl_defbool_setdefault(&b_info->u.hvm.nographic, false);

 libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
+libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force, false);

 break;
 case LIBXL_DOMAIN_TYPE_PV:
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..507034f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -757,6 +754,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..35ec5fc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc 
*gc, uint32_t domid,

   libxl_device_pci *pcidev, int num);
 _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);

+_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config 
*d_config);

+
 /*- xswait: wait for a xenstore node to be suitable -*/

 typedef struct libxl__xswait_state libxl__xswait_state;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..07b9e22 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, 
libxl_device_pci *pcidev,

 return 0;
 }

+static unsigned long sysfs_dev_get_vendor(libxl__gc *gc,
+  libxl_device_pci *pcidev)
+{
+char *pci_device_vendor_path =
+libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+   pcidev->domain, pcidev->bus, pcidev->dev,
+   pcidev->func);
+int read_items;
+unsigned long pci_device_vendor;
+
+FILE *f = fopen(pci_device_vendor_path, "r");
+if (!f) {
+LOGE(ERROR,
+ "pci device "PCI_BDF" does not have vendor attribute",
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+return 0x;
+}
+read_items = fscanf(f, "0x%lx\n", &pci_device_vendor);
+fclose(f);
+if (read_items != 1) {
+LOGE(ERROR,
+ "cannot read vendor of pci device "PCI_BDF,
+ pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+return 0x;
+}
+
+return pci_device_vendor;
+}
+
+static unsigned long sysfs_dev_get_device(libxl__gc *gc

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/10 3:56, Michael S. Tsirkin wrote:

It doesn't make sense to copy values manually:
the only issue with getting headers from linux
seems to be dealing with linux/types, we
can easily fix that automatically while importing.

Signed-off-by: Michael S. Tsirkin 
---

FYI this is what I propose instead of the recently
suggested
 virtio: uniform virtio device IDs
we can then rework existing code to include these headers.

Will automatically bring in goodies as they arrive in linux.

This doesn't yet import virtio ccw header,
that won't be hard to add later.

  scripts/update-linux-headers.sh | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index c8e026d..0bd8437 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -76,4 +76,14 @@ else
  cp "$linux/COPYING" "$output/linux-headers"
  fi

+rm -rf "$output/standard-headers/linux"
+mkdir -p "$output/standard-headers/linux"


Shouldn't we add something in configure file to execute this automatically?

Or instead of creating 'standard-headers/, why can't we go that existing 
linux-headers/?


Thanks
Tiejun


+for f in $tmpdir/include/linux/virtio*h; do
+header=$(expr "$f" : '.*/\(.*\)');
+sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
+-e 's/linux\/types/inttypes/' \
+-e 's/__bitwise__//' \
+"$tmpdir/include/linux/$header" > \
+"$output/standard-headers/linux/$header";
+done
  rm -rf "$tmpdir"

Re: [Qemu-devel] [v2][RFC][PATCH] virtio: uniform virtio device IDs

[Chen, Tiejun]

On 2015/2/9 15:02, Michael S. Tsirkin wrote:

On Mon, Feb 09, 2015 at 03:01:15PM +0800, Chen, Tiejun wrote:

On 2015/2/8 18:48, Michael S. Tsirkin wrote:

On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote:

On Fri,  6 Feb 2015 13:41:26 +0800
Tiejun Chen  wrote:


Actually we define these device IDs in virtio standard, so
we'd better put them into one common place to manage conveniently.
Here I also add VIRTIO_ID_RESERVE according to virtio spec.

Signed-off-by: Tiejun Chen 


We really should just write a script to import the headers

>from the linux kernel.

They will need some tweaks to avoid dependencies on
linux/types, but this seems easy to do - better than
trying to keep things in sync manually.


I prefer Cornelia's comment since actually we're trying to define a little
bit according to a spec, so the following may be enough?

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f24997d..4afb0b7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -23,6 +23,22 @@
  #include "hw/virtio/virtio-9p.h"
  #endif

+/* Refer to VirtIO Spec 1.0. */
+
+#define VIRTIO_ID_RESERVED  0   /* reserved (invalid)*/
+#define VIRTIO_ID_NET   1   /* network card */
+#define VIRTIO_ID_BLOCK 2   /* block device */
+#define VIRTIO_ID_CONSOLE   3   /* console */
+#define VIRTIO_ID_RNG   4   /* entropy source */
+#define VIRTIO_ID_BALLOON   5   /* memory ballooning */
+#define VIRTIO_ID_IOMEMORY  6   /* ioMemory */
+#define VIRTIO_ID_RPMSG 7   /* rpmsg */
+#define VIRTIO_ID_SCSI  8   /* SCSI host */
+#define VIRTIO_ID_9P9   /* 9P transport */
+#define VIRTIO_ID_MAC80211_WALN 10  /* mac80211 wlan */
+#define VIRTIO_ID_RPROC_SERIAL  11  /* rproc seria */
+#define VIRTIO_ID_CAIF  12  /* virtio CAIF */
+
  /* from Linux's linux/virtio_config.h */

  /* Status byte for guest to report progress, and synchronize features. */

Thanks
Tiejun


This still means each change has to be done in two places.


Are you saying another head file, pc-bios/s390-ccw/virtio.h?

But seems Cornelia thought in case of s390-ccw, -quote-

"Even though this one is incomplete; but we don't need anything but the
block id anyway."

So I'm not sure we really should do this :)

Thanks
Tiejun

Re: [Qemu-devel] [v2][RFC][PATCH] virtio: uniform virtio device IDs

[Chen, Tiejun]

On 2015/2/8 18:48, Michael S. Tsirkin wrote:

On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote:

On Fri,  6 Feb 2015 13:41:26 +0800
Tiejun Chen  wrote:


Actually we define these device IDs in virtio standard, so
we'd better put them into one common place to manage conveniently.
Here I also add VIRTIO_ID_RESERVE according to virtio spec.

Signed-off-by: Tiejun Chen 


We really should just write a script to import the headers
from the linux kernel.
They will need some tweaks to avoid dependencies on
linux/types, but this seems easy to do - better than
trying to keep things in sync manually.


I prefer Cornelia's comment since actually we're trying to define a 
little bit according to a spec, so the following may be enough?


diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f24997d..4afb0b7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -23,6 +23,22 @@
 #include "hw/virtio/virtio-9p.h"
 #endif

+/* Refer to VirtIO Spec 1.0. */
+
+#define VIRTIO_ID_RESERVED  0   /* reserved (invalid)*/
+#define VIRTIO_ID_NET   1   /* network card */
+#define VIRTIO_ID_BLOCK 2   /* block device */
+#define VIRTIO_ID_CONSOLE   3   /* console */
+#define VIRTIO_ID_RNG   4   /* entropy source */
+#define VIRTIO_ID_BALLOON   5   /* memory ballooning */
+#define VIRTIO_ID_IOMEMORY  6   /* ioMemory */
+#define VIRTIO_ID_RPMSG 7   /* rpmsg */
+#define VIRTIO_ID_SCSI  8   /* SCSI host */
+#define VIRTIO_ID_9P9   /* 9P transport */
+#define VIRTIO_ID_MAC80211_WALN 10  /* mac80211 wlan */
+#define VIRTIO_ID_RPROC_SERIAL  11  /* rproc seria */
+#define VIRTIO_ID_CAIF  12  /* virtio CAIF */
+
 /* from Linux's linux/virtio_config.h */

 /* Status byte for guest to report progress, and synchronize features. */

Thanks
Tiejun

Re: [Qemu-devel] [v2][RFC][PATCH] virtio: uniform virtio device IDs

[Chen, Tiejun]

On 2015/2/7 0:28, Stefan Hajnoczi wrote:

On Fri, Feb 06, 2015 at 01:41:26PM +0800, Tiejun Chen wrote:

Actually we define these device IDs in virtio standard, so
we'd better put them into one common place to manage conveniently.
Here I also add VIRTIO_ID_RESERVE according to virtio spec.

Signed-off-by: Tiejun Chen 
---
  hw/9pfs/virtio-9p.h|  2 --
  include/hw/virtio/virtio-balloon.h |  3 ---
  include/hw/virtio/virtio-blk.h |  3 ---
  include/hw/virtio/virtio-rng.h |  3 ---
  include/hw/virtio/virtio-scsi.h|  3 ---
  include/hw/virtio/virtio-serial.h  |  3 ---
  include/hw/virtio/virtio.h | 16 
  pc-bios/s390-ccw/virtio.h  |  8 +---
  8 files changed, 17 insertions(+), 24 deletions(-)


Reviewed-by: Stefan Hajnoczi 


Thanks for you review.

Just a little change will be introduced in next revision.

Tiejun

Re: [Qemu-devel] [v2][RFC][PATCH] virtio: uniform virtio device IDs

[Chen, Tiejun]

On 2015/2/6 20:14, Cornelia Huck wrote:

On Fri,  6 Feb 2015 13:41:26 +0800
Tiejun Chen  wrote:


Actually we define these device IDs in virtio standard, so
we'd better put them into one common place to manage conveniently.
Here I also add VIRTIO_ID_RESERVE according to virtio spec.

Signed-off-by: Tiejun Chen 
---
  hw/9pfs/virtio-9p.h|  2 --
  include/hw/virtio/virtio-balloon.h |  3 ---
  include/hw/virtio/virtio-blk.h |  3 ---
  include/hw/virtio/virtio-rng.h |  3 ---
  include/hw/virtio/virtio-scsi.h|  3 ---
  include/hw/virtio/virtio-serial.h  |  3 ---
  include/hw/virtio/virtio.h | 16 
  pc-bios/s390-ccw/virtio.h  |  8 +---
  8 files changed, 17 insertions(+), 24 deletions(-)




diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f24997d..9ad6bb2 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -23,6 +23,22 @@
  #include "hw/virtio/virtio-9p.h"
  #endif

+/* Refer to Linux's linux/virtio_ids.h */


Why not refer to the virtio spec instead? :) And maybe add in the ids


Actually they're same now but this really is a potential risk.


that already have been reserved.


So what about this?

@@ -23,6 +23,22 @@
 #include "hw/virtio/virtio-9p.h"
 #endif

+/* Refer to VirtIO Spec 1.0. */
+
+#define VIRTIO_ID_RESERVED  0   /* reserved (invalid)*/
+#define VIRTIO_ID_NET   1   /* network card */
+#define VIRTIO_ID_BLOCK 2   /* block device */
+#define VIRTIO_ID_CONSOLE   3   /* console */
+#define VIRTIO_ID_RNG   4   /* entropy source */
+#define VIRTIO_ID_BALLOON   5   /* memory ballooning */
+#define VIRTIO_ID_IOMEMORY  6   /* ioMemory */
+#define VIRTIO_ID_RPMSG 7   /* rpmsg */
+#define VIRTIO_ID_SCSI  8   /* SCSI host */
+#define VIRTIO_ID_9P9   /* 9P transport */
+#define VIRTIO_ID_MAC80211_WALN 10  /* mac80211 wlan */
+#define VIRTIO_ID_RPROC_SERIAL  11  /* rproc seria */
+#define VIRTIO_ID_CAIF  12  /* virtio CAIF */
+
 /* from Linux's linux/virtio_config.h */

 /* Status byte for guest to report progress, and synchronize features. */





+
+enum virtio_dev_type {
+VIRTIO_ID_RESERVED  = 0, /* invalid virtio device */
+VIRTIO_ID_NET = 1, /* virtio net */
+VIRTIO_ID_BLOCK= 2, /* virtio block */
+VIRTIO_ID_CONSOLE = 3, /* virtio console */
+VIRTIO_ID_RNG = 4, /* virtio rng */
+VIRTIO_ID_BALLOON = 5, /* virtio balloon */


/* virtio balloon (legacy) */


+VIRTIO_ID_RPMSG= 7, /* virtio remote processor messaging */
+VIRTIO_ID_SCSI = 8, /* virtio scsi */
+VIRTIO_ID_9P = 9, /* 9p virtio console */
+VIRTIO_ID_RPROC_SERIAL = 11, /* virtio remoteproc serial link */
+VIRTIO_ID_CAIF = 12, /* Virtio caif */
+};
+
  /* from Linux's linux/virtio_config.h */

  /* Status byte for guest to report progress, and synchronize features. */
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index c23466b..2eabcb4 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -11,6 +11,7 @@
  #ifndef VIRTIO_H
  #define VIRTIO_H

+#include "hw/virtio/virtio.h"


This won't work, the bios can't use the common headers.


Thanks for your caught.




  #include "s390-ccw.h"

  /* Status byte for guest to report progress, and synchronize features. */
@@ -23,13 +24,6 @@
  /* We've given up on this device. */
  #define VIRTIO_CONFIG_S_FAILED  0x80

-enum virtio_dev_type {
-VIRTIO_ID_NET = 1,
-VIRTIO_ID_BLOCK = 2,
-VIRTIO_ID_CONSOLE = 3,
-VIRTIO_ID_BALLOON = 5,
-};


Even though this one is incomplete; but we don't need anything but the
block id anyway.


-
  struct virtio_dev_header {
  enum virtio_dev_type type : 8;
  u8  num_vq;





I will remove all s390 stuff in this patch.

Thanks
Tiejun

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/6 9:01, Chen, Tiejun wrote:

On 2015/2/5 17:52, Ian Campbell wrote:

On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:

Indeed this is not something workaround, and I think in any type of VGA
devices, we'd like to diminish this sort of thing gradually, right?

This mightn't come true in real world :)


It's not really something we can control, the h/w guys will do what they
do, including integrating on-board graphics tightly with the N/S-bridges
etc.


Yeah.





I think there are three ways to achieve that:

* Make the libxl/xl option something which is not generic e.g.
  igd_passthru=1
* Add a second option to allow the user to configure the
kind of
  graphics device being passed thru (e.g. gfx_passthru=1,
  passthru_device="igd"), or combine the two by making the
  gfx_passthru option a string instead of a boolean.


It makes more sense but this mean we're going to change that existing
rule in qemu-traditional. But here I guess we shouldn't consider that
case.


qemu-trad is frozen so we wouldn't be adding new features such as
support for new graphics passthru devices, so we can ignore it's
deficiencies in this area and just improve things in the qemu-xen case.
(we may want to add a compat handling for any new option we add to libxl
to translate to -gfx_passthru, but that's about it)


Understood.




* Make libxl detect the type of graphics device somehow and
  therefore automatically determine when gfx_passthru=1 =>
  igd-passthru


This way confounds me all. Can libxl detect the graphics device *before*
we intend to pass a parameter to active qemu?


We know the BDF of all devices which we are going to pass to the guest
and we can observe various properties of that device
via /sys/bus/pci/devices/:B:D:F.


So sounds what you're saying is Xen already have this sort of example in
libxl.








   Currently, we have to set
something as follows,

gfx_passthru=1
pci=["00:02.0"]

This always works for qemu-xen-traditional.

But you should know '00:02.0' doesn't mean we are passing IGD through.


But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
and other properties) we can unambiguously determine if it is an IGD
device or not, can't we?


Again, like what I said above, I'm not sure if its possible in my case.
If I'm wrong please correct me.


Is my reply above sufficient?


Yes, I can understand what you mean but I need to take close look at
exactly what should be done in libxl :)

In high level, this way may come out as follows,

#1 libxl parse 'pci=[]' to get SBDF
#2 Scan SBDF by accessing sysfs to get vendor/device IDs.
#3 If this pair of IDs is identified to our target device, IGD,
"igd-passthru" would be delivered to qemu.

Right?


What about this?

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..507034f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -757,6 +754,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..35ec5fc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc 
*gc, uint32_t domid,

   libxl_device_pci *pcidev, int num);
 _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);

+_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
+   const libxl_domain_config 
*d_config);

+
 /*- xswait: wait for a xenstore node to be suitable -*/

 typedef struct libxl__xswait_state libxl__xswait_state;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..9cccbfe 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,108 @@ static int sysfs_dev_unbind

Re: [Qemu-devel] [RFC][PATCH] virtio: uniform virtio device IDs

[Chen, Tiejun]

Sorry please ignore this to review another revision.

Thanks
Tiejun

On 2015/2/6 13:24, Tiejun Chen wrote:

Actually we define these device IDs in virtio standard, so
we'd better put them into one common place to manage conveniently.
Here I also add VIRTIO_ID_RESERVE according to virtio spec.

Signed-off-by: Tiejun Chen 
---
  hw/9pfs/virtio-9p.h|  2 --
  include/hw/virtio/virtio-balloon.h |  3 ---
  include/hw/virtio/virtio-blk.h |  3 ---
  include/hw/virtio/virtio-rng.h |  3 ---
  include/hw/virtio/virtio-scsi.h|  3 ---
  include/hw/virtio/virtio-serial.h  |  3 ---
  include/hw/virtio/virtio.h | 14 ++
  pc-bios/s390-ccw/virtio.h  |  7 ---
  8 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 2c3603a..228e05d 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -146,8 +146,6 @@ struct V9fsPDU

  /* from Linux's linux/virtio_9p.h */

-/* The ID for virtio console */
-#define VIRTIO_ID_9P9
  #define MAX_REQ 128
  #define MAX_TAG_LEN 32

diff --git a/include/hw/virtio/virtio-balloon.h 
b/include/hw/virtio/virtio-balloon.h
index f863bfe..6e0a775 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -24,9 +24,6 @@

  /* from Linux's linux/virtio_balloon.h */

-/* The ID for virtio_balloon */
-#define VIRTIO_ID_BALLOON 5
-
  /* The feature bitmap for virtio balloon */
  #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
  #define VIRTIO_BALLOON_F_STATS_VQ 1   /* Memory stats virtqueue */
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 4652b70..6ee3e8f 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -25,9 +25,6 @@

  /* from Linux's linux/virtio_blk.h */

-/* The ID for virtio_block */
-#define VIRTIO_ID_BLOCK 2
-
  /* Feature bits */
  #define VIRTIO_BLK_F_BARRIER0   /* Does host support barriers? */
  #define VIRTIO_BLK_F_SIZE_MAX   1   /* Indicates maximum segment size */
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index 14e85a5..e2bb6ce 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -21,9 +21,6 @@
  #define VIRTIO_RNG_GET_PARENT_CLASS(obj) \
  OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_RNG)

-/* The Virtio ID for the virtio rng device */
-#define VIRTIO_ID_RNG4
-
  struct VirtIORNGConf {
  RngBackend *rng;
  uint64_t max_bytes;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index bf17cc9..9606f43 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -29,9 +29,6 @@
  OBJECT_CHECK(VirtIOSCSI, (obj), TYPE_VIRTIO_SCSI)


-/* The ID for virtio_scsi */
-#define VIRTIO_ID_SCSI  8
-
  /* Feature Bits */
  #define VIRTIO_SCSI_F_INOUT0
  #define VIRTIO_SCSI_F_HOTPLUG  1
diff --git a/include/hw/virtio/virtio-serial.h 
b/include/hw/virtio/virtio-serial.h
index 11af978..1dcced6 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -20,9 +20,6 @@

  /* == Interface shared between the guest kernel and qemu == */

-/* The Virtio ID for virtio console / serial ports */
-#define VIRTIO_ID_CONSOLE  3
-
  /* Features supported */
  #define VIRTIO_CONSOLE_F_MULTIPORT1

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f24997d..c2d3731 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -23,6 +23,20 @@
  #include "hw/virtio/virtio-9p.h"
  #endif

+/* from Linux's linux/virtio_ids.h */
+
+#define VIRTIO_ID_RESERVED  0 /* invalid virtio device */
+#define VIRTIO_ID_NET  1 /* virtio net */
+#define VIRTIO_ID_BLOCK2 /* virtio block */
+#define VIRTIO_ID_CONSOLE  3 /* virtio console */
+#define VIRTIO_ID_RNG  4 /* virtio rng */
+#define VIRTIO_ID_BALLOON  5 /* virtio balloon */
+#define VIRTIO_ID_RPMSG7 /* virtio remote processor messaging 
*/
+#define VIRTIO_ID_SCSI 8 /* virtio scsi */
+#define VIRTIO_ID_9P   9 /* 9p virtio console */
+#define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
+#define VIRTIO_ID_CAIF12 /* Virtio caif */
+
  /* from Linux's linux/virtio_config.h */

  /* Status byte for guest to report progress, and synchronize features. */
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index c23466b..6713a45 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -23,13 +23,6 @@
  /* We've given up on this device. */
  #define VIRTIO_CONFIG_S_FAILED  0x80

-enum virtio_dev_type {
-VIRTIO_ID_NET = 1,
-VIRTIO_ID_BLOCK = 2,
-VIRTIO_ID_CONSOLE = 3,
-VIRTIO_ID_BALLOON = 5,
-};
-
  struct virtio_dev_header {
  enum virtio_dev_type type : 8;
  u8  num_vq;

Re: [Qemu-devel] [Xen-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/5 17:52, Ian Campbell wrote:

On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:

Indeed this is not something workaround, and I think in any type of VGA
devices, we'd like to diminish this sort of thing gradually, right?

This mightn't come true in real world :)


It's not really something we can control, the h/w guys will do what they
do, including integrating on-board graphics tightly with the N/S-bridges
etc.


Yeah.





I think there are three ways to achieve that:

* Make the libxl/xl option something which is not generic e.g.
  igd_passthru=1
* Add a second option to allow the user to configure the kind of
  graphics device being passed thru (e.g. gfx_passthru=1,
  passthru_device="igd"), or combine the two by making the
  gfx_passthru option a string instead of a boolean.


It makes more sense but this mean we're going to change that existing
rule in qemu-traditional. But here I guess we shouldn't consider that case.


qemu-trad is frozen so we wouldn't be adding new features such as
support for new graphics passthru devices, so we can ignore it's
deficiencies in this area and just improve things in the qemu-xen case.
(we may want to add a compat handling for any new option we add to libxl
to translate to -gfx_passthru, but that's about it)


Understood.




* Make libxl detect the type of graphics device somehow and
  therefore automatically determine when gfx_passthru=1 =>
  igd-passthru


This way confounds me all. Can libxl detect the graphics device *before*
we intend to pass a parameter to active qemu?


We know the BDF of all devices which we are going to pass to the guest
and we can observe various properties of that device
via /sys/bus/pci/devices/:B:D:F.


So sounds what you're saying is Xen already have this sort of example in 
libxl.









   Currently, we have to set
something as follows,

gfx_passthru=1
pci=["00:02.0"]

This always works for qemu-xen-traditional.

But you should know '00:02.0' doesn't mean we are passing IGD through.


But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
and other properties) we can unambiguously determine if it is an IGD
device or not, can't we?


Again, like what I said above, I'm not sure if its possible in my case.
If I'm wrong please correct me.


Is my reply above sufficient?


Yes, I can understand what you mean but I need to take close look at 
exactly what should be done in libxl :)


In high level, this way may come out as follows,

#1 libxl parse 'pci=[]' to get SBDF
#2 Scan SBDF by accessing sysfs to get vendor/device IDs.
#3 If this pair of IDs is identified to our target device, IGD, 
"igd-passthru" would be delivered to qemu.


Right?




If not then that _might_ suggest we should deprecate the gdx_passthru
option at the libxl interface and switch to something more expressive,
such as an Enumeration of card types (with a singleton of IGD right
now), but I'm not really very familiar with passthru nor the qemu side
of this.

What happens if you try to pass two different GFX cards to a guest?



Are you saying two IGDs?


Yes, or any combination of two cards, perhaps from different vendors
(AIUI some laptops have this with IGD and Nvidia or ATI?).


One IGD and multiple other type of Graphic display cards can coexist.


... but if they both need special handling then we need a way to
communicate that.


   Its not possible since as I said above, IGD is
tricky because it depends on something from ISA bridge and host bridge.
So we can't provide two or more different setting configurations to own
more IGDs just in one platform.


This is because IGD must be a "primary" VGA device? I understand that


No. I mean ISA bridge and host bridge just provide one set of IGD
resource so its difficult to configure two or more IGDs.


there can only be one of those in a system, but I think it is possible
to have multiple secondary VGA devices or different types in one system.



What I'm saying is, its impossible to own two same IGDs in our current
platform :)


Understood, but systems needn't be homogeneous wrt graphics devices.



Ian,

Thanks for your kind discussion.

Tiejun

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/4 18:41, Ian Campbell wrote:

On Wed, 2015-02-04 at 09:34 +0800, Chen, Tiejun wrote:



   "-machine xxx,igd-passthru=on", to enable/disable that feature.
   And we also remove that old option, "-gfx_passthru", just from
   the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
   no any qemu stream version really need or use that.

 ^up ?

What happens if you pass this new option to an older version of qemu
upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
would have done.


Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of
qemu upstream. As I mentioned previously, just now we're starting to
support this in qemu upstream :)

But you known, on Xen side we have two qemu versions,
qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted
in both two versions, just qemu-xen-traditional supports IGD passthrough
completely. For qemu-xen, we just have this term definition but without
that IGD passthrough feature support actually.


I'm afraid I don't follow, you seem to be simultaneously saying that
qemu-xen both does and does not support -gfx_passthru, which cannot be
right. I think from the following paragraph that what you mean is that
upstream qemu-xen has no support for any kind of -gfx_passthru command
line option in any form in any existing version, including the git tree.
Is that correct?


Yes.



(for the purposes of this conversation qemu-traditional is not of
interest)


Yes.




And now we're trying to support IGD passthrough in qemu stream. This
mean we should set 'device_model_version="qemu-xen", right? Then libxl
still pass '-gfx_passthru' to qemu upstream. But when I post other
stuffs to qemu upstream community to support IGD passthrough. Gerd
thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'.
So finally you see this change in Xen/tools/libxl.



I have one more general concern, which is that hardcoding igd-passthru
here may make it harder to support gfx_passthru of other cards in the
future.


Actually gfx_passthrou is introduced to just work for IGD passthrough
since something specific to IGD is tricky, so we have to need such a
flag to handle this precisely, like its fixed at 00:02.0, and expose
some ISA bridge PCI config info and even host bridge PCI config info.

So I don't thing other cards need this.


If one type VGA device needs these sorts of workaround it is not
inconceivable that some other one will also need workarounds in the
future.



Indeed this is not something workaround, and I think in any type of VGA 
devices, we'd like to diminish this sort of thing gradually, right?


This mightn't come true in real world :)


Even if you don't consider non-IGD, what about the possibility of a
future IGD device which needs different (or additional, or fewer)
workarounds?


As far as I know we're trying to drop off those dependencies on ISA 
bridge and host bridge in our IGD's roadmap. Because in any pass through 
cases, theoretically we should access those resources dedicated to that 
device.





Somehow something in the stack needs to either detect or be told which
kind of card to passthrough. Automatic detection would be preferable,
but maybe being told by the user is the only possibility.


Based on the above explanation, something should be done before we
detect to construct that real card , so its difficulty to achieve this
goal currently.



Is there any way, given gfx_passthru as a boolean that libxl can
automatically figure out that IGD passthru is what is actually desired
-- e.g. by scanning the set of PCI devices given to the guest perhaps?


Sorry I don't understand what you mean here.


"gfx_passthru" is a generically named option, but it is being
implemented in an IGD specific way. We need to consider the possibility
of other graphics devices needing special handling in the future and
plan accordingly such that we can try and maintain our API guarantees
when this happens.


Agreed.



I think there are three ways to achieve that:

   * Make the libxl/xl option something which is not generic e.g.
 igd_passthru=1
   * Add a second option to allow the user to configure the kind of
 graphics device being passed thru (e.g. gfx_passthru=1,
 passthru_device="igd"), or combine the two by making the
 gfx_passthru option a string instead of a boolean.


It makes more sense but this mean we're going to change that existing 
rule in qemu-traditional. But here I guess we shouldn't consider that case.



   * Make libxl detect the type of graphics device somehow and
 therefore automatically determine when gfx_passthru=1 =>
 igd-passthru


This way confounds me all. Can libxl detect the graphics device *before* 
we in

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/3 19:07, Ian Campbell wrote:

On Tue, 2015-02-03 at 09:04 +0800, Chen, Tiejun wrote:


On 2015/2/2 20:54, Ian Jackson wrote:

Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX 
passthrough"):

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.

...

My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.


I think the commit message of the xen.git commit should explain what
options are supported by which versions of qemu (including qemu
upstream's future plans).

That would provide (a) something which summarises the communication
etc. with qemu upstream and can be checked with them if necessary and
(b) something against which the libxl changes can be easily judged.



Sorry, looks I'm misleading this to everyone.

Here I picked my reply from another email:

Actually qemu upstream never own this option, '-gfx_passthru' at all.
This just exists alone in qemu-xen-traditional. So here I'm trying to
introduce a new stuff that doesn't clash anything in qemu upstream.

So I guess I should rephrase this as follows:

  libxl: add one machine property to support IGD GFX passthrough

  When we're working to support IGD GFX passthrough with qemu
  upstream, we'd like to introduce a machine option,


Has this new option been acked/accepted into the upstream qemu code base
yet? I think there should also be a reference to the relevant qemu.git


Yes, Gerd just recommend this option and I guess this should be acked as 
well :)



changeset as well as to any useful conversations about it.


I just post this patch prior to those qemu patch supporting IGD 
passthrough since I need this option is acked on Xen side. Then I can 
continue submitting all qemu patches.





  "-machine xxx,igd-passthru=on", to enable/disable that feature.
  And we also remove that old option, "-gfx_passthru", just from
  the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
  no any qemu stream version really need or use that.

^up ?

What happens if you pass this new option to an older version of qemu
upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
would have done.


Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of 
qemu upstream. As I mentioned previously, just now we're starting to 
support this in qemu upstream :)


But you known, on Xen side we have two qemu versions, 
qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted 
in both two versions, just qemu-xen-traditional supports IGD passthrough 
completely. For qemu-xen, we just have this term definition but without 
that IGD passthrough feature support actually.


And now we're trying to support IGD passthrough in qemu stream. This 
mean we should set 'device_model_version="qemu-xen", right? Then libxl 
still pass '-gfx_passthru' to qemu upstream. But when I post other 
stuffs to qemu upstream community to support IGD passthrough. Gerd 
thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'. 
So finally you see this change in Xen/tools/libxl.




I have one more general concern, which is that hardcoding igd-passthru
here may make it harder to support gfx_passthru of other cards in the
future.


Actually gfx_passthrou is introduced to just work for IGD passthrough 
since something specific to IGD is tricky, so we have to need such a 
flag to handle this precisely, like its fixed at 00:02.0, and expose 
some ISA bridge PCI config info and even host bridge PCI config info.


So I don't thing other cards need this.



Somehow something in the stack needs to either detect or be told which
kind of card to passthrough. Automatic detection would be preferable,
but maybe being told by the user is the only possibility.


Based on the above explanation, something should be done before we 
detect to construct that real card , so its difficulty to achieve this 
goal currently.




Is there any way, given gfx_passthru as a boolean that libxl can
automatically figure out that IGD passthru is what is actually desired
-- e.g. by scanning the set of PCI devices given to the guest perhaps?


Sorry I don't understand what you mean here. Currently, we have to set 
something as follows,


gfx_passthru=1
pci=["00:02.0"]

This always works for qemu-xen-traditional.

But you should know '00:02.0' doesn't mean we are passing IGD through.



If not then 

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/3 18:19, Wei Liu wrote:

On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:


On 2015/2/2 20:19, Wei Liu wrote:

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.

Signed-off-by: Tiejun Chen 
---
v2:

* Based on some discussions with Wei we'd like to keep both old
   option, -gfx_passthru, and new machine property option,
   "-machine xxx,-igd-passthru=on" at the same time but deprecate
   the old one. Then finally we remove the old one at that point
   that to give downstream (in this case, Xen) time to cope with the
   change.



My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.


Understood.




  tools/libxl/libxl_dm.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..8405f0b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,6 +701,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,


Note this function is upstream QEMU specfic.


Yeah.




  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
+/*
+ * Although we already introduce 'igd-passthru', but we'd like
+ * to remove this until we give downstream time to cope with
+ * the change.
+ */
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
  flexarray_append(dm_args, "-gfx_passthru");
  }


The comment contradicts what I know (or what I think I know). In our
last email exchange you said there was no "-gfx_passthru" in any version
of upstream QEMU.

So, shouldn't you just remove this `if' statement?


Right. So what about this?

 libxl: add one machine property to support IGD GFX passthrough

 When we're working to support IGD GFX passthrough with qemu
 upstream, we'd like to introduce a machine option,
 "-machine xxx,igd-passthru=on", to enable/disable that feature.
 And we also remove that old option, "-gfx_passthru", just from
 the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
 no any qemu stream version really need or use that.

 Signed-off-by: Tiejun Chen 



Yes. I think a patch like this reflects the reality.

It would be nice, as Ian J suggested, to state which version of QEMU
upstream introduces that new option in commit message.


I have to provide this exact info after those qemu stuffs are merged, 
but now at lease you guys agree this option named as 'igd-passthrough', 
its enough to first continue working all qemu patches out. Once its 
finished I will go back.





diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..b888f19 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,9 +701,6 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
  } else {
  if (!sdl && !vnc) {
  flexarray_append(dm_args, "-nographic");
@@ -748,6 +745,11 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
machinearg);
+}
+


Please use GCSPRINTF macro.



Sure.

@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);


Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/2 20:54, Ian Jackson wrote:

Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX 
passthrough"):

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.

...

My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.


I think the commit message of the xen.git commit should explain what
options are supported by which versions of qemu (including qemu
upstream's future plans).

That would provide (a) something which summarises the communication
etc. with qemu upstream and can be checked with them if necessary and
(b) something against which the libxl changes can be easily judged.



Sorry, looks I'm misleading this to everyone.

Here I picked my reply from another email:

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.


So I guess I should rephrase this as follows:

libxl: add one machine property to support IGD GFX passthrough

When we're working to support IGD GFX passthrough with qemu
upstream, we'd like to introduce a machine option,
"-machine xxx,igd-passthru=on", to enable/disable that feature.
And we also remove that old option, "-gfx_passthru", just from
the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
no any qemu stream version really need or use that.

So is it good?

Thanks
Tiejun

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/2 20:19, Wei Liu wrote:

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.

Signed-off-by: Tiejun Chen 
---
v2:

* Based on some discussions with Wei we'd like to keep both old
   option, -gfx_passthru, and new machine property option,
   "-machine xxx,-igd-passthru=on" at the same time but deprecate
   the old one. Then finally we remove the old one at that point
   that to give downstream (in this case, Xen) time to cope with the
   change.



My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.


Understood.




  tools/libxl/libxl_dm.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..8405f0b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,6 +701,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,


Note this function is upstream QEMU specfic.


Yeah.




  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
+/*
+ * Although we already introduce 'igd-passthru', but we'd like
+ * to remove this until we give downstream time to cope with
+ * the change.
+ */
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
  flexarray_append(dm_args, "-gfx_passthru");
  }


The comment contradicts what I know (or what I think I know). In our
last email exchange you said there was no "-gfx_passthru" in any version
of upstream QEMU.

So, shouldn't you just remove this `if' statement?


Right. So what about this?

libxl: add one machine property to support IGD GFX passthrough

When we're working to support IGD GFX passthrough with qemu
upstream, we'd like to introduce a machine option,
"-machine xxx,igd-passthru=on", to enable/disable that feature.
And we also remove that old option, "-gfx_passthru", just from
the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
no any qemu stream version really need or use that.

Signed-off-by: Tiejun Chen 

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..b888f19 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,9 +701,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "-net");
 flexarray_append(dm_args, "none");
 }
-if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
-}
 } else {
 if (!sdl && !vnc) {
 flexarray_append(dm_args, "-nographic");
@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 machinearg, max_ram_below_4g);
 }
 }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = libxl__sprintf(gc, "%s,igd-passthru=on", 
machinearg);

+}
+
 flexarray_append(dm_args, machinearg);
 for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)

 flexarray_append(dm_args, b_info->extra_hvm[i]);

Thanks
Tiejun



Wei.


@@ -748,6 +753,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = libxl__sprintf(gc, "%s,igd-passthru=on", machinearg);
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);
--
1.9.1

Re: [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/2/2 19:08, Ian Campbell wrote:

On Mon, 2015-02-02 at 09:17 +0800, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.



From which Qemu version is this new option accepted? What will a verison

of qemu prior to then do when presented with the new option?


Sorry, maybe I'm not describing this correctly.

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.


So I guess I should rephrase this as follows:

libxl: add one machine property to support IGD GFX passthrough

When we're working to support IGD GFX passthrough with qemu
upstream, we'd like to introduce a machine option,
"-machine xxx,igd-passthru=on", to enable/disable that feature.
And we also remove that old option, "-gfx_passthru", just from
the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
no any qemu stream version really need or use that.



(nb: you have an extra '-' in the description I think)


Yeah, I will remove that.

Thanks
Tiejun





Signed-off-by: Tiejun Chen 
---
v2:

* Based on some discussions with Wei we'd like to keep both old
   option, -gfx_passthru, and new machine property option,
   "-machine xxx,-igd-passthru=on" at the same time but deprecate
   the old one. Then finally we remove the old one at that point
   that to give downstream (in this case, Xen) time to cope with the
   change.

  tools/libxl/libxl_dm.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..8405f0b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,6 +701,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "-net");
  flexarray_append(dm_args, "none");
  }
+/*
+ * Although we already introduce 'igd-passthru', but we'd like
+ * to remove this until we give downstream time to cope with
+ * the change.
+ */
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
  flexarray_append(dm_args, "-gfx_passthru");
  }
@@ -748,6 +753,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = libxl__sprintf(gc, "%s,igd-passthru=on", machinearg);
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/30 20:26, Wei Liu wrote:

On Fri, Jan 30, 2015 at 08:56:48AM +0800, Chen, Tiejun wrote:
[...]


Just remember to handle old option in libxl if your old option is already
released by some older version of QEMUs.


I just drop that old option, -gfx_passthru, if we're under qemu upstream
circumstance, like this,



The question is, is there any version of qemu upstream that has
been released that has the old option (-gfx-passthru)?


No. Just now we're starting to support IGD passthrough in qemu upstream.



Right, as of QEMU 2.2.0 there's no support of IGD passthrough in QMEU
upstream.



This gives us a situation that we need to support both the old
(-gfx-passthru) and new (-igd-passthru) options. Presumably we (libxl)
would need to fork a qemu process to determine which option it has and
pass the right one.

Or you can try to keep both old and new option at the same time but


Yeah, actually I also have considered to keep both two options at the same
time. Its really friendly to any qemu version.


deprecate the old one. Then in a few qemu release cycles later (or


This should be like 'accel=kvm' versus 'enable-kvm' in qemu upstream.
They're coexisted now but just the former is a modern option.


probably one year or two?) you can finally remove the old one. The point
is that to give downstream (in this case, Xen) time to cope with the
change.


Here I'm fine to this way.

So Gerd,



So you don't actually need to ask Gerd this question because there is no
old option to keep in qemu upstream.

Libxl (or any sensible toolstack) will just do the right thing to either
pass -igd-passthru (or whatever you guys agree upon) to qemu upstream or
pass -gfx-passthru to qemu traditional. :-)



Okay let me try do this.

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/29 18:50, Wei Liu wrote:

On Thu, Jan 29, 2015 at 08:41:24AM +0800, Chen, Tiejun wrote:

On 2015/1/28 19:12, Wei Liu wrote:

On Wed, Jan 28, 2015 at 08:42:56AM +0800, Chen, Tiejun wrote:

On 2015/1/27 22:40, Ian Jackson wrote:

Chen, Tiejun writes ("Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine 
property to support IGD GFX passthrough"):

On 2015/1/23 8:43, Chen, Tiejun wrote:

On 2015/1/22 8:51, Chen, Tiejun wrote:

At this point I just concern here if we still use 'gfx_passthrou', at
least it may look like a backward compatibility with older versions of
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your
final option.

...

Any feedback to this option I should follow here?


Ping...


I think this is a question that qemu upstream should answer.



Actually this is just commented by Gerd in qemu upstream. So now looks in
Xen side you guys don't have any objection to use 'igd-passthru' as well. If
yes, I'm fine to adopt this.



Yes, we would like to stay in line with upstream.


Wei,

Thanks for your response.



Just remember to handle old option in libxl if your old option is already
released by some older version of QEMUs.


I just drop that old option, -gfx_passthru, if we're under qemu upstream
circumstance, like this,



The question is, is there any version of qemu upstream that has
been released that has the old option (-gfx-passthru)?


No. Just now we're starting to support IGD passthrough in qemu upstream.



This gives us a situation that we need to support both the old
(-gfx-passthru) and new (-igd-passthru) options. Presumably we (libxl)
would need to fork a qemu process to determine which option it has and
pass the right one.

Or you can try to keep both old and new option at the same time but


Yeah, actually I also have considered to keep both two options at the 
same time. Its really friendly to any qemu version.



deprecate the old one. Then in a few qemu release cycles later (or


This should be like 'accel=kvm' versus 'enable-kvm' in qemu upstream. 
They're coexisted now but just the former is a modern option.



probably one year or two?) you can finally remove the old one. The point
is that to give downstream (in this case, Xen) time to cope with the
change.


Here I'm fine to this way.

So Gerd,

What about this?




--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -318,7 +318,10 @@ static char **
libxl__build_device_model_args_old(libxl__gc *gc,
  flexarray_vappend(dm_args, "-net", "none", NULL);
  }
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}


I don't think this is right if upstream qemu also supports gfx-passthru.

However, you're modifying libxl__build_device_model_args_old, it
strongly suggests that it only affects qemu-trad. That means you don't
even need this patch...


You're right and thanks.

Thanks
Tiejun



Wei.


  }
  } else {
  if (!sdl && !vnc)
@@ -702,7 +705,10 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "none");
  }
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}
  }
  } else {
  if (!sdl && !vnc) {

Is this good enough?

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/28 19:12, Wei Liu wrote:

On Wed, Jan 28, 2015 at 08:42:56AM +0800, Chen, Tiejun wrote:

On 2015/1/27 22:40, Ian Jackson wrote:

Chen, Tiejun writes ("Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine 
property to support IGD GFX passthrough"):

On 2015/1/23 8:43, Chen, Tiejun wrote:

On 2015/1/22 8:51, Chen, Tiejun wrote:

At this point I just concern here if we still use 'gfx_passthrou', at
least it may look like a backward compatibility with older versions of
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your
final option.

...

Any feedback to this option I should follow here?


Ping...


I think this is a question that qemu upstream should answer.



Actually this is just commented by Gerd in qemu upstream. So now looks in
Xen side you guys don't have any objection to use 'igd-passthru' as well. If
yes, I'm fine to adopt this.



Yes, we would like to stay in line with upstream.


Wei,

Thanks for your response.



Just remember to handle old option in libxl if your old option is already
released by some older version of QEMUs.


I just drop that old option, -gfx_passthru, if we're under qemu upstream 
circumstance, like this,


--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -318,7 +318,10 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,

 flexarray_vappend(dm_args, "-net", "none", NULL);
 }
 if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}
 }
 } else {
 if (!sdl && !vnc)
@@ -702,7 +705,10 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,

 flexarray_append(dm_args, "none");
 }
 if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}
 }
 } else {
 if (!sdl && !vnc) {

Is this good enough?

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/27 22:40, Ian Jackson wrote:

Chen, Tiejun writes ("Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine 
property to support IGD GFX passthrough"):

On 2015/1/23 8:43, Chen, Tiejun wrote:

On 2015/1/22 8:51, Chen, Tiejun wrote:

At this point I just concern here if we still use 'gfx_passthrou', at
least it may look like a backward compatibility with older versions of
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your
final option.

...

Any feedback to this option I should follow here?


Ping...


I think this is a question that qemu upstream should answer.



Actually this is just commented by Gerd in qemu upstream. So now looks 
in Xen side you guys don't have any objection to use 'igd-passthru' as 
well. If yes, I'm fine to adopt this.


Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/23 8:43, Chen, Tiejun wrote:

On 2015/1/22 8:51, Chen, Tiejun wrote:

On 2015/1/21 21:48, Gerd Hoffmann wrote:

On Mi, 2015-01-21 at 11:37 +, Ian Jackson wrote:

Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property
to support IGD GFX passthrough"):

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,gfx_passthru=on". This need
to bring several changes on tool side.


Has the corresponding patch to qemu-upstream been accepted yet ?

I'd like to see a confirmation from the qemu side that this is going
into their tree and that the command-line option syntax has been
agreed.


Suggested at patch review, not merged (guess thats why it is tagged
'rfc', to get both qemu+xen on the same page).


Yeah, this is exactly what I intended to do here :)



While being at it:  Should we name this 'igd-passthru' instead of
'gfx-passthru'?  The hostbridge / isabridge quirks needed are actually
specific to igd and are not needed for -- say -- nvidia gfx cards.



At this point I just concern here if we still use 'gfx_passthrou', at
least it may look like a backward compatibility with older versions of
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your
final option.



Any feedback to this option I should follow here?



Ping...

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/22 8:51, Chen, Tiejun wrote:

On 2015/1/21 21:48, Gerd Hoffmann wrote:

On Mi, 2015-01-21 at 11:37 +, Ian Jackson wrote:

Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property
to support IGD GFX passthrough"):

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,gfx_passthru=on". This need
to bring several changes on tool side.


Has the corresponding patch to qemu-upstream been accepted yet ?

I'd like to see a confirmation from the qemu side that this is going
into their tree and that the command-line option syntax has been
agreed.


Suggested at patch review, not merged (guess thats why it is tagged
'rfc', to get both qemu+xen on the same page).


Yeah, this is exactly what I intended to do here :)



While being at it:  Should we name this 'igd-passthru' instead of
'gfx-passthru'?  The hostbridge / isabridge quirks needed are actually
specific to igd and are not needed for -- say -- nvidia gfx cards.



At this point I just concern here if we still use 'gfx_passthrou', at
least it may look like a backward compatibility with older versions of
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your
final option.



Any feedback to this option I should follow here?

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/21 19:56, Ian Campbell wrote:

On Wed, 2015-01-21 at 11:37 +, Ian Jackson wrote:

Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property to support IGD 
GFX passthrough"):

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,gfx_passthru=on". This need
to bring several changes on tool side.


Has the corresponding patch to qemu-upstream been accepted yet ?

I'd like to see a confirmation from the qemu side that this is going
into their tree and that the command-line option syntax has been
agreed.


Do we need to detect old vs. new qemu to know when this option is valid?


Do you mean we should introduce these two options into qemu upstream at 
the same time? Here I just keep that new option, and that old is gone.


Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

On 2015/1/21 21:48, Gerd Hoffmann wrote:

On Mi, 2015-01-21 at 11:37 +, Ian Jackson wrote:

Tiejun Chen writes ("[RFC][PATCH 1/1] libxl: add one machine property to support IGD 
GFX passthrough"):

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,gfx_passthru=on". This need
to bring several changes on tool side.


Has the corresponding patch to qemu-upstream been accepted yet ?

I'd like to see a confirmation from the qemu side that this is going
into their tree and that the command-line option syntax has been
agreed.


Suggested at patch review, not merged (guess thats why it is tagged
'rfc', to get both qemu+xen on the same page).


Yeah, this is exactly what I intended to do here :)



While being at it:  Should we name this 'igd-passthru' instead of
'gfx-passthru'?  The hostbridge / isabridge quirks needed are actually
specific to igd and are not needed for -- say -- nvidia gfx cards.



At this point I just concern here if we still use 'gfx_passthrou', at 
least it may look like a backward compatibility with older versions of 
qemu in Xen side, qemu-xen-traditional. But I'd like to follow your 
final option.


Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 1/1] libxl: add one machine property to support IGD GFX passthrough

[Chen, Tiejun]

CCed Stefano.

Thanks
Tiejun

On 2015/1/21 15:19, Tiejun Chen wrote:

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,gfx_passthru=on". This need
to bring several changes on tool side.

Signed-off-by: Tiejun Chen 
---
  tools/libxl/libxl_dm.c | 19 +--
  1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..2b59d2c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -318,7 +318,10 @@ static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
  flexarray_vappend(dm_args, "-net", "none", NULL);
  }
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}
  }
  } else {
  if (!sdl && !vnc)
@@ -702,7 +705,10 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  flexarray_append(dm_args, "none");
  }
  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-flexarray_append(dm_args, "-gfx_passthru");
+if (b_info->device_model_version !=
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+flexarray_append(dm_args, "-gfx_passthru");
+}
  }
  } else {
  if (!sdl && !vnc) {
@@ -748,6 +754,15 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
  machinearg, max_ram_below_4g);
  }
  }
+
+if (b_info->device_model_version ==
+LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+machinearg = libxl__sprintf(gc, "%s,gfx_passthru=on",
+machinearg);
+}
+}
+
  flexarray_append(dm_args, machinearg);
  for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
  flexarray_append(dm_args, b_info->extra_hvm[i]);

Re: [Qemu-devel] [v6][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support

[Chen, Tiejun]

On 2015/1/20 16:14, Gerd Hoffmann wrote:

On Di, 2015-01-20 at 11:14 +0800, Chen, Tiejun wrote:

On 2015/1/19 19:45, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

+DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
+"-gfx_passthru   enable Intel IGD passthrough by XEN\n",
+QEMU_ARCH_ALL)
+STEXI
+@item -gfx_passthru
+@findex -gfx_passthru
+Enable Intel IGD passthrough by XEN
+ETEXI


Make that a machine option, i.e. "-machine pc,igd-passthru=on"?


Yeah but I think we need "-machine xenfv,igd-passthru=on" here.


IIRC xen decided to stop using xenfv and use pc-$version instead (with
$version being what was current at release time, 1.6 for xen 4.4 I
think, to avoid surprises like the address space layout changes in more
recent machine types).



To be more exact, 'xen_platform_pci' should determine this at this point,

if (!libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
/* Switching here to the machine "pc" which does not add
 * the xen-platform device instead of the default "xenfv" 
machine.

 */
machinearg = libxl__sprintf(gc, "pc,accel=xen");
} else {
machinearg = libxl__sprintf(gc, "xenfv");
}

But you may remember, in our case we always set 'xen_platform_pci=0' 
since we need to release slot 2 for IGD. So finally we really go pc case.


Anyway this means something should be changed to pass such a new machine 
property in Xen side. And I'll send a patch to address this firstly, 
then go back here.


Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 08/10] xen, gfx passthrough: support Intel IGD passthrough with VT-D

[Chen, Tiejun]

+uint32_t xen_igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
+{
+XenHostPCIDevice dev;
+uint32_t val;
+int r;
+
+/* IGD read/write is through the host bridge.
+ */
+assert(pci_dev->devfn == 0x00);
+
+if (!is_igd_passthrough(pci_dev)) {
+goto read_default;
+}
+
+/* Just work for the i915 driver. */
+switch (config_addr) {
+case 0x08:/* revision id */
+case 0x2c:/* sybsystem vendor id */
+case 0x2e:/* sybsystem id */
+case 0x50:/* SNB: processor graphics control register */
+case 0x52:/* processor graphics control register */
+case 0xa0:/* top of memory */


Is this host physical memory? If yes how can using it in guest work?


This is just a threshold value, not a start or end address :)




+case 0xa4:/* SNB: graphics base of stolen memory */
+case 0xa8:/* SNB: base of GTT stolen memory */


Same question for above two.


I shouldn't matter since I remember we already discussed this previously 
but I can't sort out this from those emails now.


Allen,

Could you reexplain this?




+break;
+default:
+/* Just gets the emulated values. */
+goto read_default;
+}
+
+/* Host read */
+r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);
+if (r) {
+goto err_out;
+}
+
+r = xen_host_pci_get_block(&dev, config_addr, (uint8_t *)&val, len);
+if (r) {
+goto err_out;
+}
+
+xen_host_pci_device_put(&dev);
+
+return val;
+
+read_default:
+return pci_default_read_config(pci_dev, config_addr, len);
+
+err_out:
+XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+return -1;
+}


Do any of the above registers change with time?


Think about we just provide read ops, so they're not changed based on my 
experiential.



Does it work if we just read them when device is created
and put in dev->config?


I think this is a good idea so I will go there and thank you.

Tiejun

Re: [Qemu-devel] [v6][PATCH 07/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

+static void
+xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
+  XenHostPCIDevice *dev)
+{


I suggest this implementation, and the table, are moved
to the same file where igd-passthrough-isa-bridge
is implemented. The function can get PCIDevice *d, this way
it's not xen specific.


Absolutely, you're right.

Actually I already start to work this way since Gerd said this should 
have a common between Xen and Kvm(KvmGT).







+struct PCIDevice *pci_dev;


pls rename bridge_dev;


Okay.

Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 07/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

+
+if (pch_dev_id == 0x) {
+fprintf(stderr, "unsupported PCH!\n");


I would drop this fprintf: this likely means a newer
card, so the bridge is not necessary.


Okay.




+return;
+}
+
+/* Currently IGD drivers always need to access PCH by 1f.0. */
+pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
+"xen-igd-passthrough-isa-bridge");
+
+/*
+ * Identify PCH card with its own real vendor/device ids.


This no longer holds I think.


You're right.

Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 01/10] i440fx: make types configurable at run-time

[Chen, Tiejun]

On 2015/1/19 19:36, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

From: "Michael S. Tsirkin" 

Xen wants to supply a different pci and host devices,
inheriting i440fx devices. Make types configurable.


Description is misleading, this isn't about xen but about IGD
passthrough.  Guess kvm needs this too once kvmgt lands upstream.



So just rephrase this as follows:

i440fx: make types configurable at run-time

IGD passthrough wants to supply a different pci and
host devices, inheriting i440fx devices. Make types
configurable.

Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/1/20 12:28, Jike Song wrote:

On 01/20/2015 10:52 AM, Chen, Tiejun wrote:

On 2015/1/19 19:40, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

+static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass,
+  void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->desc = "IGD PT XEN Host bridge";
+}


IMO "xen" naming should go away here too.



I would agree with this.

In fact, this piece of code could possibly be used by:

  a) IGD passthru for Xen and KVM, and/or:
  b) IGD Mediated passthru for Xen and KVM, i.e. XenGT/KVMGT

So it looks better if have "xen" naming purged :)


Okay, I'll do this in next revision.

Thanks
Tiejun





Its easy to do but we need to wait KvmGT guys' response, so now it makes
sense to leave "xen" as a prefix since this just work in xen side.

Thanks
Tiejun



--
Thanks,
Jike

Re: [Qemu-devel] [v6][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support

[Chen, Tiejun]

On 2015/1/19 19:45, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

+DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
+"-gfx_passthru   enable Intel IGD passthrough by XEN\n",
+QEMU_ARCH_ALL)
+STEXI
+@item -gfx_passthru
+@findex -gfx_passthru
+Enable Intel IGD passthrough by XEN
+ETEXI


Make that a machine option, i.e. "-machine pc,igd-passthru=on"?


Yeah but I think we need "-machine xenfv,igd-passthru=on" here.

Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 03/10] piix: create host bridge to passthrough

[Chen, Tiejun]

On 2015/1/19 19:40, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

+static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass,
+  void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->desc = "IGD PT XEN Host bridge";
+}


IMO "xen" naming should go away here too.



Its easy to do but we need to wait KvmGT guys' response, so now it makes 
sense to leave "xen" as a prefix since this just work in xen side.


Thanks
Tiejun

Re: [Qemu-devel] [v6][PATCH 01/10] i440fx: make types configurable at run-time

[Chen, Tiejun]

On 2015/1/19 19:36, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

From: "Michael S. Tsirkin" 

Xen wants to supply a different pci and host devices,
inheriting i440fx devices. Make types configurable.


Description is misleading, this isn't about xen but about IGD


Its about IGD passthrough in Xen side.


passthrough.  Guess kvm needs this too once kvmgt lands upstream.



But I'm not sure if KvmGT is really going to be this so just CCed Jike 
who are focusing on KvmGT currently, and he also promised to have a 
closer loot at this.


Thanks
Tiejun


Otherwise looks sane to me.

cheers,
   Gerd

Re: [Qemu-devel] [v6][PATCH 07/10] xen, gfx passthrough: register a isa bridge

[Chen, Tiejun]

On 2015/1/19 21:58, Michael S. Tsirkin wrote:

On Mon, Jan 19, 2015 at 12:57:18PM +0100, Gerd Hoffmann wrote:

On Mo, 2015-01-19 at 17:28 +0800, Tiejun Chen wrote:

Currently IGD drivers always need to access PCH by 1f.0. But we


Obvious question: q35?

q35 already has a isa bridge @ 0x1f.0.  Guess that needs to be extended
for the pass-through then (simliar to the host bridge) instead of adding
a dummy bridge.

Also: again xen naming here.

cheers,
   Gerd


This hack probably doesn't have a chance to work well for q35
due to the above conflict.


Yeah, and I remember as we discussed previously, something else also 
conflict with Xen and Windows currently so Xen doesn't intend to go Q35.


Thanks
Tiejun


Happily future cards won't need it.

Re: [Qemu-devel] [RFC][PATCH] qemu_opt_get_bool_helper: back finding desc by name just if !opt->desc

[Chen, Tiejun]

On 2015/1/6 22:56, Stefan Hajnoczi wrote:

On Tue, Jan 06, 2015 at 10:39:13AM +0800, Chen, Tiejun wrote:

On 2015/1/6 9:21, Chen, Tiejun wrote:

On 2015/1/6 1:13, Eric Blake wrote:

On 01/04/2015 10:35 PM, Tiejun Chen wrote:

After one commit 49d2e648e808, "machine: remove qemu_machine_opts
global list", is introduced, QEMU doesn't keep a global list of
options but set desc lately. Then we can see the following,

$ x86_64-softmmu/qemu-system-x86_64 -usb
qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \
 Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
Aborted (core dumped)

So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name()
to work parse_option_bool() out just in case of !opt->desc.

Signed-off-by: Tiejun Chen 
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..7cb3601 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
*opts, const char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if ((opt == NULL) || !opt->desc)  {


Over-parenthesized, and looks like you also introduced a spurious space.
  Simpler to just have:

if (!opt || !opt->desc) {


Looks good.



Also, there are other threads about the same topic.
https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html


Yeah, I already notice that.



Then I realize I need to extend this to all qemu_opt_get_*.


Marcel is working on a fix.

I think hacking qemu_opt_get_* is not a clean solution.  The intention
was to move away from QemuOpts to QOM properties.  The patch that
removed -machine QemuOptsList descriptions was buggy, but the proper fix
is to make that approach work instead of hacking qemu_opt_get_*.



Understand.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH] vl.c: fix -usb option assertion failure in qemu_opt_get_bool_helper()

[Chen, Tiejun]

On 2015/1/6 14:20, Shannon Zhao wrote:

On 2015/1/6 10:37, Chen, Tiejun wrote:

On 2015/1/5 20:14, Marcel Apfelbaum wrote:

On 01/05/2015 01:50 PM, Stefan Hajnoczi wrote:

On Mon, Jan 5, 2015 at 11:37 AM, Jan Kiszka 
wrote:

On 2015-01-05 12:22, Stefan Hajnoczi wrote:

Commit 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6 ("machine: remove
qemu_machine_opts global list") removed option descriptions from the
-machine QemuOptsList to avoid repeating MachineState's QOM properties.

This change broke vl.c:usb_enabled() because qemu_opt_get_bool() cannot
be used on QemuOptsList without option descriptions since QemuOpts
doesn't know the type and therefore left an unparsed string value.

This patch avoids calling qemu_opt_get_bool() to fix the assertion
failure:

$ qemu-system-x86_64 -usb
qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type
== QEMU_OPT_BOOL' failed.

Test the presence of -usb using qemu_opt_find() but use the
MachineState->usb field instead of qemu_opt_get_bool().

Cc: Marcel Apfelbaum 
Cc: Tiejun Chen 
Signed-off-by: Stefan Hajnoczi 
---
   vl.c | 7 +--
   1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index bea9656..6e8889c 100644
--- a/vl.c
+++ b/vl.c
@@ -999,8 +999,11 @@ static int parse_name(QemuOpts *opts, void
*opaque)

   bool usb_enabled(bool default_usb)
   {
-return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
- has_defaults && default_usb);
+if (qemu_opt_find(qemu_get_machine_opts(), "usb")) {
+return current_machine->usb;
+} else {
+return has_defaults && default_usb;
+}
   }

   #ifndef _WIN32



That still leaves the other boolean machine options broken. A generic
fix would be good. Or revert the original commit until we have one.


I think we should revert the original commit.

qemu_option_get_*() callers need to be converted to query MachineState
fields instead of using QemuOpts functions.


Can this work out currently?


Hi Tiejun,

I apply your following patch and it works. At least it doesn't crash.



Thanks for your test.

Tiejun


Thanks,
Shannon


---
  util/qemu-option.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..933b885 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -317,7 +317,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
  }

  opt = qemu_opt_find(opts, name);
-if (!opt) {
+if (!opt || !opt->desc)  {
  const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
  if (desc && desc->def_value_str) {
  return desc->def_value_str;
@@ -341,7 +341,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
  }

  opt = qemu_opt_find(opts, name);
-if (!opt) {
+if (!opt || !opt->desc)  {
  desc = find_desc_by_name(opts->list->desc, name);
  if (desc && desc->def_value_str) {
  str = g_strdup(desc->def_value_str);
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const 
char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
  const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
  if (desc && desc->def_value_str) {
  parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
@@ -413,7 +413,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, 
const char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
  const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
  if (desc && desc->def_value_str) {
  parse_option_number(name, desc->def_value_str, &ret, 
&error_abort);
@@ -450,7 +450,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, 
const char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
  const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
  if (desc && desc->def_value_str) {
  parse_option_size(name, desc->def_value_str, &ret, &error_abort);

Re: [Qemu-devel] [RFC][PATCH] qemu_opt_get_bool_helper: back finding desc by name just if !opt->desc

[Chen, Tiejun]

On 2015/1/6 9:21, Chen, Tiejun wrote:

On 2015/1/6 1:13, Eric Blake wrote:

On 01/04/2015 10:35 PM, Tiejun Chen wrote:

After one commit 49d2e648e808, "machine: remove qemu_machine_opts
global list", is introduced, QEMU doesn't keep a global list of
options but set desc lately. Then we can see the following,

$ x86_64-softmmu/qemu-system-x86_64 -usb
qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \
 Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
Aborted (core dumped)

So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name()
to work parse_option_bool() out just in case of !opt->desc.

Signed-off-by: Tiejun Chen 
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..7cb3601 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
*opts, const char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if ((opt == NULL) || !opt->desc)  {


Over-parenthesized, and looks like you also introduced a spurious space.
  Simpler to just have:

if (!opt || !opt->desc) {


Looks good.



Also, there are other threads about the same topic.
https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html


Yeah, I already notice that.



Then I realize I need to extend this to all qemu_opt_get_*.

Tiejun

Re: [Qemu-devel] [PATCH] vl.c: fix -usb option assertion failure in qemu_opt_get_bool_helper()

[Chen, Tiejun]

On 2015/1/5 20:14, Marcel Apfelbaum wrote:

On 01/05/2015 01:50 PM, Stefan Hajnoczi wrote:

On Mon, Jan 5, 2015 at 11:37 AM, Jan Kiszka 
wrote:

On 2015-01-05 12:22, Stefan Hajnoczi wrote:

Commit 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6 ("machine: remove
qemu_machine_opts global list") removed option descriptions from the
-machine QemuOptsList to avoid repeating MachineState's QOM properties.

This change broke vl.c:usb_enabled() because qemu_opt_get_bool() cannot
be used on QemuOptsList without option descriptions since QemuOpts
doesn't know the type and therefore left an unparsed string value.

This patch avoids calling qemu_opt_get_bool() to fix the assertion
failure:

   $ qemu-system-x86_64 -usb
   qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type
== QEMU_OPT_BOOL' failed.

Test the presence of -usb using qemu_opt_find() but use the
MachineState->usb field instead of qemu_opt_get_bool().

Cc: Marcel Apfelbaum 
Cc: Tiejun Chen 
Signed-off-by: Stefan Hajnoczi 
---
  vl.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index bea9656..6e8889c 100644
--- a/vl.c
+++ b/vl.c
@@ -999,8 +999,11 @@ static int parse_name(QemuOpts *opts, void
*opaque)

  bool usb_enabled(bool default_usb)
  {
-return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
- has_defaults && default_usb);
+if (qemu_opt_find(qemu_get_machine_opts(), "usb")) {
+return current_machine->usb;
+} else {
+return has_defaults && default_usb;
+}
  }

  #ifndef _WIN32



That still leaves the other boolean machine options broken. A generic
fix would be good. Or revert the original commit until we have one.


I think we should revert the original commit.

qemu_option_get_*() callers need to be converted to query MachineState
fields instead of using QemuOpts functions.


Can this work out currently?

---
 util/qemu-option.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..933b885 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -317,7 +317,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char 
*name)

 }

 opt = qemu_opt_find(opts, name);
-if (!opt) {
+if (!opt || !opt->desc)  {
 const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);

 if (desc && desc->def_value_str) {
 return desc->def_value_str;
@@ -341,7 +341,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
 }

 opt = qemu_opt_find(opts, name);
-if (!opt) {
+if (!opt || !opt->desc)  {
 desc = find_desc_by_name(opts->list->desc, name);
 if (desc && desc->def_value_str) {
 str = g_strdup(desc->def_value_str);
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, 
const char *name,

 }

 opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
 const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);

 if (desc && desc->def_value_str) {
 parse_option_bool(name, desc->def_value_str, &ret, 
&error_abort);
@@ -413,7 +413,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts 
*opts, const char *name,

 }

 opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
 const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);

 if (desc && desc->def_value_str) {
 parse_option_number(name, desc->def_value_str, &ret, 
&error_abort);
@@ -450,7 +450,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts 
*opts, const char *name,

 }

 opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if (!opt || !opt->desc)  {
 const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);

 if (desc && desc->def_value_str) {
 parse_option_size(name, desc->def_value_str, &ret, 
&error_abort);

--

Thanks
Tiejun



This means we need to modify the way default values are processed.
MachineState fields should start with the default value and be
overridden by command-line options.  Right now it works differently:
we query the command-line option and fall back to the default if the
option wasn't set.  That approach doesn't work for MachineState fields
since C types (bool, int, etc) aren't tristate in the general case, so
they cannot indicate whether or not they were set.

The benefit of doing this work is that we eliminate the QemuOpts layer
and work directly with QOM properties instead.

Marcel: Do you want to do this or do you have another fix in mind?

Hi, sorry for not getting to this earlier, I thought Paolo already fixed
it, but I was wrong.

I think this is the right direction, I'll am going to take care of it.
(At least I'll try...)

Thanks,
Marcel


Stefan

Re: [Qemu-devel] [RFC][PATCH] qemu_opt_get_bool_helper: back finding desc by name just if !opt->desc

[Chen, Tiejun]

On 2015/1/6 1:13, Eric Blake wrote:

On 01/04/2015 10:35 PM, Tiejun Chen wrote:

After one commit 49d2e648e808, "machine: remove qemu_machine_opts
global list", is introduced, QEMU doesn't keep a global list of
options but set desc lately. Then we can see the following,

$ x86_64-softmmu/qemu-system-x86_64 -usb
qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \
 Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
Aborted (core dumped)

So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name()
to work parse_option_bool() out just in case of !opt->desc.

Signed-off-by: Tiejun Chen 
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..7cb3601 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const 
char *name,
  }

  opt = qemu_opt_find(opts, name);
-if (opt == NULL) {
+if ((opt == NULL) || !opt->desc)  {


Over-parenthesized, and looks like you also introduced a spurious space.
  Simpler to just have:

if (!opt || !opt->desc) {


Looks good.



Also, there are other threads about the same topic.
https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html


Yeah, I already notice that.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Chen, Tiejun]

On 2014/12/27 22:52, Paolo Bonzini wrote:



On 26/12/2014 18:59, Peter Maydell wrote:

Mm, but once you're into such microoptimisations as this you really
need to have a good justification for the effort, in the form of
profiling measurements that indicate that this is a hot path.
In this case that seems pretty unlikely, because I'd expect all
the systems where we care about performance will support irqfds,
so we won't be taking the early-exit code path anyhow. (And
not supporting irqfds is leaving much more performance on the
table than we could possibly be talking about in this function.)


Also, it's even possible for a compiler to figure this out.  All in all,
I don't see any advantage to this patch...



Indeed, its just a cleanup to make codes readable and comprehensible 
since oftentimes we don't initially write such a subsequent code just 
because we have this possibility to figure them out by the compiler, or 
others. And this is why I'm CCing Qemu trivial.


Tiejun

Re: [Qemu-devel] [v5][PATCH 0/4] xen: introduce new machine for IGD passthrough

[Chen, Tiejun]

On 2014/11/23 18:10, Michael S. Tsirkin wrote:

On Tue, Aug 12, 2014 at 05:49:13PM +0800, Tiejun Chen wrote:

v5:

* Simplify to make sure its really inherited from the standard one in patch #3
* Then drop the original patch #3


I carried
i440fx: make types configurable at run-time
pc_init1: pass parameters just with types
xen:hw:pci-host:piix: create host bridge to passthrough
qom-test: blacklist xenigd
xen:hw:i386:pc_piix: introduce new machine for IGD passthrough

on my branch for a while now, but I'm not sure it's all still needed.


They may not be necessary since you and Paolo already are fine to live 
without that separate machine specific to IGD passthrough.



If yes simply include these patches next time you repost the
patchset.


Anyway, I will comment this point once I can issue next revision.

Thanks for your reminder.

Tiejun

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/17 18:13, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:

On 2014/11/17 17:25, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:

On 2014/11/17 14:10, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:

On 2014/11/5 22:09, Michael S. Tsirkin wrote:

On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:

Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.

Signed-off-by: Tiejun Chen 
---


[snip]


Cleaner:
 if (!pci_dev) {
fprintf
return;
}
  pci_config_set_device_id(pci_dev->config, pch_id);


I will address all comments and thanks.




+}
+}
+
  /* init */

  static int xen_pt_initfn(PCIDevice *d)
@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
  return -1;
  }

+/* Register ISA bridge for passthrough GFX. */
+xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
+
  /* reinitialize each config register to be emulated */
  if (xen_pt_config_init(s)) {
  XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");

Note I will introduce a inline function in another patch,

+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
+&& ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}

Thanks
Tiejun


Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?



If this is just used for IGD, its always fine without checking vendor_id.


You need to match device ID to *know* it's IGD.


So after remove that check, I guess I need to rename that as
is_igd_vga_passthrough() to make sense.

Thanks
Tiejun


There is no need to check class code or xen_has_gfx_passthru flag.
Device ID + vendor ID identifies each device uniquely.



Yeah.

Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means 
I also need to check that table to do something like,


is_igd_vga_passthugh(dev)
{   
int i;
int num = ARRAY_SIZE(xen_igd_combo_id_infos);
for (i = 0; i < num; i++) {
if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
return 1;
return 0;
}

Then we can simplify the subsequent codes based on this, right?

Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/17 17:25, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:

On 2014/11/17 14:10, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:

On 2014/11/5 22:09, Michael S. Tsirkin wrote:

On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:

Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.

Signed-off-by: Tiejun Chen 
---


[snip]


Cleaner:
 if (!pci_dev) {
fprintf
return;
}
  pci_config_set_device_id(pci_dev->config, pch_id);


I will address all comments and thanks.




+}
+}
+
  /* init */

  static int xen_pt_initfn(PCIDevice *d)
@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
  return -1;
  }

+/* Register ISA bridge for passthrough GFX. */
+xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
+
  /* reinitialize each config register to be emulated */
  if (xen_pt_config_init(s)) {
  XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");

Note I will introduce a inline function in another patch,

+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
+&& ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}

Thanks
Tiejun


Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?



If this is just used for IGD, its always fine without checking vendor_id.

So after remove that check, I guess I need to rename that as 
is_igd_vga_passthrough() to make sense.


Thanks
Tiejun

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/17 14:10, Michael S. Tsirkin wrote:

On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:

On 2014/11/5 22:09, Michael S. Tsirkin wrote:

On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:

Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.

Signed-off-by: Tiejun Chen 
---
  hw/i386/pc_piix.c | 28 +++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b559181..b19c7a9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,7 @@
  #include "cpu.h"
  #include "qemu/error-report.h"
  #ifdef CONFIG_XEN
-#  include 
+#include 
  #endif

  #define MAX_IDE_BUS 2
@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
  }

  #ifdef CONFIG_XEN
+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+struct PCIDevice *dev;
+Error *local_err = NULL;
+uint16_t device_id = 0x;
+
+/* Currently IGD drivers always need to access PCH by 1f.0. */
+dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+"xen-igd-passthrough-isa-bridge");
+
+/* Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+if (dev) {
+device_id = object_property_get_int(OBJECT(dev), "device-id",
+&local_err);
+if (!local_err && device_id != 0x) {
+pci_config_set_device_id(dev->config, device_id);
+return;
+}
+}
+
+fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+}
+
  static void pc_xen_hvm_init(MachineState *machine)
  {
  PCIBus *bus;
@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
  bus = pci_find_primary_bus();
  if (bus != NULL) {
  pci_create_simple(bus, -1, "xen-platform");
+xen_igd_passthrough_isa_bridge_create(bus);
  }
  }
  #endif


Can't we defer this step until the GPU is added?


Sounds great but I can't figure out where we can to do this exactly.


This way there won't be need to poke at host device
directly, you could get all info from dev->config
of the host device.


As I understand We have two steps here:

#1 At first I have to write something to check if we're registering 00:02.0
& IGD, right? But where? While registering each pci device?


In xen_pt_initfn.
Just check the device and vendor ID against the table you have.



Okay. Please see the follows which is just compiled:

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c6466dc..f3ea313 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
 .priority = 10,
 };

+typedef struct {
+uint16_t gpu_device_id;
+uint16_t pch_device_id;
+} XenIGDDeviceIDInfo;
+
+/* In real world different GPU should have different PCH. But actually
+ * the different PCH DIDs likely map to different PCH SKUs. We do the
+ * same thing for the GPU. For PCH, the different SKUs are going to be
+ * all the same silicon design and implementation, just different
+ * features turn on and off with fuses. The SW interfaces should be
+ * consistent across all SKUs in a given family (eg LPT). But just same
+ * features may not be supported.
+ *
+ * Most of these different PCH features probably don't matter to the
+ * Gfx driver, but obviously any difference in display port connections
+ * will so it should be fine with any PCH in case of passthrough.
+ *
+ * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
+ * scenarios, 0x9cc3 for BDW(Broadwell).
+ */
+static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
+/* HSW Classic */
+{0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
+{0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
+{0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
+{0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
+{0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
+/* HSW ULT */
+{0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
+{0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
+{0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
+{0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
+{0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
+{0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
+/* HSW CRW */
+{0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
+{0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
+/* HSW Server */
+{0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
+/* HSW SRVR */
+{0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
+/* BSW */
+{0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
+{0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
+{0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
+{0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
+{0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
+{0x1602, 0x9cc3}, /* BDWHALOGT1

Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/5 22:09, Michael S. Tsirkin wrote:

On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:

Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.

Signed-off-by: Tiejun Chen 
---
  hw/i386/pc_piix.c | 28 +++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b559181..b19c7a9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,7 @@
  #include "cpu.h"
  #include "qemu/error-report.h"
  #ifdef CONFIG_XEN
-#  include 
+#include 
  #endif

  #define MAX_IDE_BUS 2
@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
  }

  #ifdef CONFIG_XEN
+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+struct PCIDevice *dev;
+Error *local_err = NULL;
+uint16_t device_id = 0x;
+
+/* Currently IGD drivers always need to access PCH by 1f.0. */
+dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+"xen-igd-passthrough-isa-bridge");
+
+/* Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+if (dev) {
+device_id = object_property_get_int(OBJECT(dev), "device-id",
+&local_err);
+if (!local_err && device_id != 0x) {
+pci_config_set_device_id(dev->config, device_id);
+return;
+}
+}
+
+fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+}
+
  static void pc_xen_hvm_init(MachineState *machine)
  {
  PCIBus *bus;
@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
  bus = pci_find_primary_bus();
  if (bus != NULL) {
  pci_create_simple(bus, -1, "xen-platform");
+xen_igd_passthrough_isa_bridge_create(bus);
  }
  }
  #endif


Can't we defer this step until the GPU is added?


Sounds great but I can't figure out where we can to do this exactly.


This way there won't be need to poke at host device
directly, you could get all info from dev->config
of the host device.


As I understand We have two steps here:

#1 At first I have to write something to check if we're registering 
00:02.0 & IGD, right? But where? While registering each pci device?


#2 Then if that condition is matched, we register this ISA bridge on its 
associated bus.


Thanks
Tiejun


Additionally the correct bridge would be initialized automatically.



--
1.9.1

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

[Chen, Tiejun]

On 2014/11/3 21:10, Michael S. Tsirkin wrote:

On Mon, Nov 03, 2014 at 01:01:03PM +0100, Paolo Bonzini wrote:



On 03/11/2014 12:47, Chen, Tiejun wrote:

On 2014/11/3 19:36, Chen, Tiejun wrote:

On 2014/11/3 19:35, Paolo Bonzini wrote:

On 03/11/2014 08:48, Chen, Tiejun wrote:

I think the point was mostly to reserve 1f to prevent
devices from using it.
As we populate slots in order it doesn't seem to important ...


If we populate slot at !1f GFX driver can't find this ISA bridge.


Right, but I mean if no special options are used, 1f will typically
stay free without any effort on our side.


Yeah.

Actually based on current info we know, seems 1f is just specific to
our
scenario :) So I always think we can occupy that. But Paolo and you
can
really determine this point.


What's your idea?


I do not have any objection to always occupying 1f for Xen IGD
passthrough.


After I go back to look at this again, I hope you don't misunderstand
what Michael mean now. He was saying we don't need to create a new
separate machine specific to IGD passthrough. But that idea is just from
you :)


It's difficult for me to follow, because xen_igd_passthrough_pc_hvm_init
does not exist in the current tree.

The patches seem good to me; I was assuming that the new machine type
would call xen_igd_passthrough_pc_hvm_init, but apparently I'm wrong?
Paolo


Discussed on irc, Paolo said
 so i don't really care how the ISA bridge is created



This means all those previous patches creating new separate machine 
should be gone.


I would rebase these two patches to resend again as RFC.

Thanks
Tiejun