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@xxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home