Re: [Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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