| Message ID | 1427073466-16956-3-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
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
On Tue, 2015-03-24 at 16:47 +0800, Chen, Tiejun wrote: > 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[]? They are registered with the Python runtime, so they are called from Python code. The first member of the struct is the pythonic function name, e.g. from xc.c: { "domain_create", (PyCFunction)pyxc_domain_create, 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. > 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. Ian.
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
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. NB, the libxl ones are broken and not even compiled right now, you can ignore them. > > > 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. > > > (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 suppose if you really wanted to expose this here then you would need to invent some syntax for doing so. Ian.
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
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. > > 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. Making RMRR work via the (C) interface to libxl used by xl and libvirt is sufficient for a new in tree feature. 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
On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote: > Although we already have 'gfx_passthru' in b_info, this doesn' suffice ^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. > 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 "-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. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > docs/man/xl.cfg.pod.5 | 11 +++++++---- > 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 | 14 ++++++++++++-- > 6 files changed, 68 insertions(+), 9 deletions(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index 408653f..c29bcbf 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<seize|/"seize_boolean"> above. > devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean"> > above. > > -=item B<gfx_passthru=BOOLEAN> > +=item B<gfx_passthru="STRING"> I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate. > 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<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. 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<gfx_passthru> option either disables gfx passthru or enables autodetection. When given as a string the B<gfx_passthru> option describes the type of passthru to enable. Valid options are: =over 4 =item B<gfx_passthru=0> Disables gfx_passthru. =item B<gfx_passthru=1>, B<gfx_passthru="default"> 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!) 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 do it in libxl__build_device_model_args_old using the libxl__detect_gfx_passthru_kind helper you have added e.g. where libxl__build_device_model_args_old has: if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { flexarray_append(dm_args, "-gfx_passthru"); } would become something like: if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { if (libxl__detect_gfx_passthru_kind(gc, guest_config) != LIBXL_GFX_PASSTHRU_KIND_IGD) { LOG("only IGD gfx_passthru is supported with qemu-xen-traditional"); return NULL; } flexarray_append(dm_args, "-gfx_passthru"); } Or something like that. What do you think? > 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); This should be in the previous patch. In fact I think it is and this is a redundant second instance. Ian.
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<seize|/"seize_boolean"> above. >> devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean"> >> above. >> >> -=item B<gfx_passthru=BOOLEAN> >> +=item B<gfx_passthru="STRING"> > > I think B<gfx_passthru=BOOLEAN|"STRING"> 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<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. 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<gfx_passthru> option either > disables gfx passthru or enables autodetection. > > When given as a string the B<gfx_passthru> option describes the > type of passthru to enable. > > Valid options are: > > =over 4 > > =item B<gfx_passthru=0> > > Disables gfx_passthru. > > =item B<gfx_passthru=1>, B<gfx_passthru="default"> > > 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<seize|/"seize_boolean"> above. devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean"> above. -=item B<gfx_passthru=BOOLEAN> +=item B<gfx_passthru=BOOLEAN|"STRING"> 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<gfx_passthru> option either disables gfx +passthru or enables autodetection. + +But when given as a string the B<gfx_passthru> 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<gfx_passthru=0> + +Disables graphics device PCI passthrough. + +=item B<gfx_passthru=1>, B<gfx_passthru="default"> + +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__build_device_model_args_old using the > libxl__detect_gfx_passthru_kind helper you have added > > e.g. where libxl__build_device_model_args_old has: > if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > flexarray_append(dm_args, "-gfx_passthru"); > } > would become something like: > if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > if (libxl__detect_gfx_passthru_kind(gc, guest_config) != > LIBXL_GFX_PASSTHRU_KIND_IGD) { > LOG("only IGD gfx_passthru is supported with qemu-xen-traditional"); > return NULL; > } > flexarray_append(dm_args, "-gfx_passthru"); > } > > Or something like that. What do you think? > >> 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); > > This should be in the previous patch. In fact I think it is and this is > a redundant second instance. Yes, we can remove this completely. Thanks 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 > > >
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. The "nop value" is whatever value should be passed to retain the current behaviour. Ian.
On Wed, 2015-03-25 at 09:10 +0800, Chen, Tiejun wrote: > +But when given as a string the B<gfx_passthru> option describes the type > +of device to enable. Not this behavior is only supported with upstream "Note" and "the upstream..." > +=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. > 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. Ian.
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
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<gfx_passthru> 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
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. Ian.
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
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. Ian.
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
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). Ian.
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 index 408653f..c29bcbf 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<seize|/"seize_boolean"> above. devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean"> above. -=item B<gfx_passthru=BOOLEAN> +=item B<gfx_passthru="STRING"> 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<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. 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. 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/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 a8b08f2..5649024 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -416,6 +416,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, @@ -727,9 +743,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"); @@ -774,6 +787,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_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), ("serial", string), ("boot", string), diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 04faf98..b78b4ec 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -1956,8 +1956,18 @@ 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); + } switch (xlu_cfg_get_list_as_string_list(config, "serial", &b_info->u.hvm.serial_list, 1))
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 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 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 "-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. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- docs/man/xl.cfg.pod.5 | 11 +++++++---- 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 | 14 ++++++++++++-- 6 files changed, 68 insertions(+), 9 deletions(-)