Andre Przywara
2010-Sep-08 09:19 UTC
[Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build
Add a cpuid parameter into libxl_domain_build_info and use it''s content while setting up the domain. This is a only paving the way, the real functionality is implemented in a later patch. Signed-off-by: Andre Przywara <andre.przywara@amd.com> -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-08 09:47 UTC
Re: [Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build
On Wed, 2010-09-08 at 10:19 +0100, Andre Przywara wrote:> Add a cpuid parameter into libxl_domain_build_info and use > it''s content while setting up the domain. This is a only paving the way, > the real functionality is implemented in a later patch. > > Signed-off-by: Andre Przywara <andre.przywara@amd.com>The destructor function should free the type but not the reference to it, so:> @@ -102,6 +102,21 @@ void > libxl_key_value_list_destroy(libxl_key_value_list *pkvl) > free(kvl); > } > > +void libxl_cpuid_destroy(libxl_cpuid_policy_list cpuid_list)This should be *cpuid_list and the function should be adjusted to free the elements of *cpuid_list but not cpuid_list itself.> --- a/tools/libxl/libxl.idl > +++ b/tools/libxl/libxl.idl > @@ -11,6 +11,7 @@ libxl_console_consback = Builtin("console_consback") > libxl_console_constype = Builtin("console_constype") > libxl_disk_phystype = Builtin("disk_phystype") > libxl_nic_type = Builtin("nic_type") > +libxl_cpuid_policy_list = Builtin("cpuid_policy_list", destructor_fn="libxl_cpuid_destroy")And this should have passby=PASS_BY_REFERENCE. See 22078:573ddf5cc145 for a similar change to the libxl_string_list and libxl_key_value_list destructor functions.> --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -172,6 +172,22 @@ typedef enum { > NICTYPE_VIF, > } libxl_nic_type; > > +/* holds the CPUID response for a single CPUID leaf > + * input contains the value of the EAX and ECX register, > + * and each policy string contains a filter to apply to > + * the host given values for that particular leaf. > + */ > +struct libxl_cpuid_policy { > + uint32_t input[2]; > + char *policy[4]; > +};I realise (at least I think I do) that this is just exposing the existing hypervisor/libxc interface warts and all but would this be more obvious to users if it was: struct libxl_cpuid_policy { uint32_t eax; uint32_t ecx; char *eax_filter; char *ebx_filter; char *ecx_filter; char *edx_filter; } could either translate in libxl or push the change down into libxc. Alternatively #define LIBXL_CPUID_INPUT_EAX 0 #define LIBXL_CPUID_INPUT_ECX 1 #define LIBXL_CPUID_FILTER_EAX 0 #define LIBXL_CPUID_FILTER_EBX 1 ... would at least make the code (or at least the data structure) a bit more obvious. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andre Przywara
2010-Sep-09 19:16 UTC
Re: [Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build
Ian Campbell wrote:> On Wed, 2010-09-08 at 10:19 +0100, Andre Przywara wrote: >> Add a cpuid parameter into libxl_domain_build_info and use >> it''s content while setting up the domain. This is a only paving the way, >> the real functionality is implemented in a later patch. >> >> Signed-off-by: Andre Przywara <andre.przywara@amd.com> > > The destructor function should free the type but not the reference to > it, so: > >> @@ -102,6 +102,21 @@ void >> libxl_key_value_list_destroy(libxl_key_value_list *pkvl) >> free(kvl); >> } >> >> +void libxl_cpuid_destroy(libxl_cpuid_policy_list cpuid_list) > > This should be *cpuid_list and the function should be adjusted to free > the elements of *cpuid_list but not cpuid_list itself. > >> --- a/tools/libxl/libxl.idl >> +++ b/tools/libxl/libxl.idl >> @@ -11,6 +11,7 @@ libxl_console_consback = Builtin("console_consback") >> libxl_console_constype = Builtin("console_constype") >> libxl_disk_phystype = Builtin("disk_phystype") >> libxl_nic_type = Builtin("nic_type") >> +libxl_cpuid_policy_list = Builtin("cpuid_policy_list", destructor_fn="libxl_cpuid_destroy") > > And this should have passby=PASS_BY_REFERENCE. > > See 22078:573ddf5cc145 for a similar change to the libxl_string_list and > libxl_key_value_list destructor functions.Do you mean like in the attached patch?> >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -172,6 +172,22 @@ typedef enum { >> NICTYPE_VIF, >> } libxl_nic_type; >> >> +/* holds the CPUID response for a single CPUID leaf >> + * input contains the value of the EAX and ECX register, >> + * and each policy string contains a filter to apply to >> + * the host given values for that particular leaf. >> + */ >> +struct libxl_cpuid_policy { >> + uint32_t input[2]; >> + char *policy[4]; >> +}; > > I realise (at least I think I do) that this is just exposing the > existing hypervisor/libxc interface warts and all but would this be more > obvious to users if it was: > struct libxl_cpuid_policy { > uint32_t eax; > uint32_t ecx; > > char *eax_filter; > char *ebx_filter; > char *ecx_filter; > char *edx_filter; > } > > could either translate in libxl or push the change down into libxc.Actually I consider this structure not a real interface, but more an opaque kludge to transport the data from the configuration parsing to domain creation. If you want to change the data here, I''d like to see the parse functions used. Actually I designed them such that one can alter the policy at any time by chaining calls to these functions. This is my plan to use in the upcoming multi-core patch, it would simply call libxl_cpuid_parse_config(&b_info->cpuid, "proccount=4"); to make it ultimately readable. Beside that I''d rather hide the dynamic array nature of it.> Alternatively > #define LIBXL_CPUID_INPUT_EAX 0 > #define LIBXL_CPUID_INPUT_ECX 1 > > #define LIBXL_CPUID_FILTER_EAX 0 > #define LIBXL_CPUID_FILTER_EBX 1 > ... > would at least make the code (or at least the data structure) a bit more > obvious.I am not sure whether that would help. The interface is too error-prone to be directly used by other functions than those parsers, so I''d like not to promote it with defining macros (which I probably wouldn''t use in my own code, since I mostly either propagate the reg number or enumerate over all registers). If you like I could introduce a kind of low-level function, like: libxl_cpuid_set_policy(libxl_cpuid_policy_list *list, uint32_t leaf, int bit, char policy); That could be used by other parts of libxl as well and would care about the string fiddling and allocation. Do we need this function? Shall I state the opaque nature of this structure in a comment? Regards, Andre. -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-10 08:06 UTC
Re: [Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build
On Thu, 2010-09-09 at 20:16 +0100, Andre Przywara wrote:> Ian Campbell wrote: > > On Wed, 2010-09-08 at 10:19 +0100, Andre Przywara wrote: > >> Add a cpuid parameter into libxl_domain_build_info and use > >> it''s content while setting up the domain. This is a only paving the way, > >> the real functionality is implemented in a later patch. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@amd.com> > > > > The destructor function should free the type but not the reference to > > it, so: > > > >> @@ -102,6 +102,21 @@ void > >> libxl_key_value_list_destroy(libxl_key_value_list *pkvl) > >> free(kvl); > >> } > >> > >> +void libxl_cpuid_destroy(libxl_cpuid_policy_list cpuid_list) > > > > This should be *cpuid_list and the function should be adjusted to free > > the elements of *cpuid_list but not cpuid_list itself. > > > >> --- a/tools/libxl/libxl.idl > >> +++ b/tools/libxl/libxl.idl > >> @@ -11,6 +11,7 @@ libxl_console_consback = Builtin("console_consback") > >> libxl_console_constype = Builtin("console_constype") > >> libxl_disk_phystype = Builtin("disk_phystype") > >> libxl_nic_type = Builtin("nic_type") > >> +libxl_cpuid_policy_list = Builtin("cpuid_policy_list", destructor_fn="libxl_cpuid_destroy") > > > > And this should have passby=PASS_BY_REFERENCE. > > > > See 22078:573ddf5cc145 for a similar change to the libxl_string_list and > > libxl_key_value_list destructor functions. > Do you mean like in the attached patch?Yes it looks good from the IDL perspective.> > > >> --- a/tools/libxl/libxl.h > >> +++ b/tools/libxl/libxl.h > >> @@ -172,6 +172,22 @@ typedef enum { > >> NICTYPE_VIF, > >> } libxl_nic_type; > >> > >> +/* holds the CPUID response for a single CPUID leaf > >> + * input contains the value of the EAX and ECX register, > >> + * and each policy string contains a filter to apply to > >> + * the host given values for that particular leaf. > >> + */ > >> +struct libxl_cpuid_policy { > >> + uint32_t input[2]; > >> + char *policy[4]; > >> +}; > > > > I realise (at least I think I do) that this is just exposing the > > existing hypervisor/libxc interface warts and all but would this be more > > obvious to users if it was: > > struct libxl_cpuid_policy { > > uint32_t eax; > > uint32_t ecx; > > > > char *eax_filter; > > char *ebx_filter; > > char *ecx_filter; > > char *edx_filter; > > } > > > > could either translate in libxl or push the change down into libxc. > Actually I consider this structure not a real interface, but more an > opaque kludge to transport the data from the configuration parsing to > domain creation. > If you want to change the data here, I''d like to see > the parse functions used. Actually I designed them such that one can > alter the policy at any time by chaining calls to these functions. This > is my plan to use in the upcoming multi-core patch, it would simply call > libxl_cpuid_parse_config(&b_info->cpuid, "proccount=4"); > to make it ultimately readable. > Beside that I''d rather hide the dynamic array nature of it. > > > Alternatively > > #define LIBXL_CPUID_INPUT_EAX 0 > > #define LIBXL_CPUID_INPUT_ECX 1 > > > > #define LIBXL_CPUID_FILTER_EAX 0 > > #define LIBXL_CPUID_FILTER_EBX 1 > > ... > > would at least make the code (or at least the data structure) a bit more > > obvious. > I am not sure whether that would help. The interface is too error-prone > to be directly used by other functions than those parsers, so I''d like > not to promote it with defining macros (which I probably wouldn''t use in > my own code, since I mostly either propagate the reg number or enumerate > over all registers).OK, I think that''s all very reasonable.> If you like I could introduce a kind of low-level function, like: > libxl_cpuid_set_policy(libxl_cpuid_policy_list *list, uint32_t leaf, > int bit, char policy); > That could be used by other parts of libxl as well and would care about > the string fiddling and allocation. > Do we need this function?I don''t think we need to do this unless/until we have a user which specifically requires it and we can always add it when such a thing shows up.> Shall I state the opaque nature of this structure in a comment?If it is truly opaque to the users of libxl (and from your patches it seems that it is) then even better would be to move struct libxl_cpuid_policy to libxl_internal.h and rename it to struct libxl__cpuid_policy. Then libxl.h simply contains public typedefs typedef struct libxl__cpuid_policy libxl_cpuid_policy; typedef libxl_cpuid_policy * libxl_cpuid_policy_list; This is similar to how the libxl__device_model_starting type is handled. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andre Przywara
2010-Sep-16 13:05 UTC
[Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build
Add a cpuid parameter into libxl_domain_build_info and use it''s content while setting up the domain. This is a only paving the way, the real functionality is implemented in the later patches. Signed-off-by: Andre Przywara <andre.przywara@amd.com> -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-16 15:34 UTC
[Xen-devel] Re: [PATCH 1/5] libxl: introduce cpuid interface to domain build
On Thu, 2010-09-16 at 14:05 +0100, Andre Przywara wrote:> Add a cpuid parameter into libxl_domain_build_info and use > it''s content while setting up the domain. This is a only paving the way, > the real functionality is implemented in the later patches. > > Signed-off-by: Andre Przywara <andre.przywara@amd.com>Looks good to me, thanks! Acked-by: Ian Campbell <ian.campbell@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel