Re: [Xen-devel] Re: [Qemu-devel] [PATCH V10 03/15] xen: Support new libxc calls from xen unstable.

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/25/2011 08:06 AM, Anthony PERARD wrote:
> On Thu, Feb 24, 2011 at 17:29, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 02/02/2011 08:49 AM, anthony.perard@citrix.com wrote:
>>      
>>> From: Anthony PERARD<anthony.perard@citrix.com>
>>>
>>> This patch adds a generic layer for xc calls, allowing us to choose
>>> between the
>>> xenner and xen implementations at runtime.
>>>
>>> It also update the libxenctrl calls in Qemu to use the new interface,
>>> otherwise Qemu wouldn't be able to build against new versions of the
>>> library.
>>>
>>> We check libxenctrl version in configure, from Xen 3.3.0 to Xen
>>> unstable.
>>>
>>> Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>> Acked-by: Alexander Graf<agraf@suse.de>
>>> ---
>>>   Makefile.target      |    3 +
>>>   configure            |   62 +++++++++++++++-
>>>   hw/xen_backend.c     |   74 ++++++++++---------
>>>   hw/xen_backend.h     |    7 +-
>>>   hw/xen_common.h      |   38 ++++++----
>>>   hw/xen_console.c     |   10 +-
>>>   hw/xen_devconfig.c   |   10 +-
>>>   hw/xen_disk.c        |   28 ++++---
>>>   hw/xen_domainbuild.c |   29 ++++----
>>>   hw/xen_interfaces.c  |  191
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/xen_interfaces.h  |  198
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/xen_nic.c         |   36 +++++-----
>>>   hw/xenfb.c           |   14 ++--
>>>   13 files changed, 584 insertions(+), 116 deletions(-)
>>>   create mode 100644 hw/xen_interfaces.c
>>>   create mode 100644 hw/xen_interfaces.h
>>>
>>> diff --git a/Makefile.target b/Makefile.target
>>> index db29e96..d09719f 100644
>>> --- a/Makefile.target
>>> +++ b/Makefile.target
>>> @@ -205,6 +205,9 @@ QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
>>>   QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
>>>   QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
>>>
>>> +# xen support
>>> +obj-$(CONFIG_XEN) += xen_interfaces.o
>>> +
>>>   # xen backend driver support
>>>   obj-$(CONFIG_XEN) += xen_backend.o xen_devconfig.o
>>>   obj-$(CONFIG_XEN) += xen_console.o xenfb.o xen_disk.o xen_nic.o
>>> diff --git a/configure b/configure
>>> index 5a9121d..fde9bad 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -126,6 +126,7 @@ vnc_jpeg=""
>>>   vnc_png=""
>>>   vnc_thread="no"
>>>   xen=""
>>> +xen_ctrl_version=""
>>>   linux_aio=""
>>>   attr=""
>>>   vhost_net=""
>>> @@ -1144,13 +1145,71 @@ fi
>>>
>>>   if test "$xen" != "no" ; then
>>>     xen_libs="-lxenstore -lxenctrl -lxenguest"
>>> +
>>> +  # Xen unstable
>>>     cat>    $TMPC<<EOF
>>>   #include<xenctrl.h>
>>>   #include<xs.h>
>>> -int main(void) { xs_daemon_open(); xc_interface_open(); return 0; }
>>> +#include<stdint.h>
>>> +#include<xen/hvm/hvm_info_table.h>
>>> +#if !defined(HVM_MAX_VCPUS)
>>> +# error HVM_MAX_VCPUS not defined
>>> +#endif
>>> +int main(void) {
>>> +  xc_interface *xc;
>>> +  xs_daemon_open();
>>> +  xc = xc_interface_open(0, 0, 0);
>>> +  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>>> +  xc_gnttab_open(NULL, 0);
>>> +  return 0;
>>> +}
>>>   EOF
>>>     if compile_prog "" "$xen_libs" ; then
>>> +    xen_ctrl_version=410
>>> +    xen=yes
>>> +
>>> +  # Xen 4.0.0
>>> +  elif (
>>> +      cat>    $TMPC<<EOF
>>> +#include<xenctrl.h>
>>> +#include<xs.h>
>>> +#include<stdint.h>
>>> +#include<xen/hvm/hvm_info_table.h>
>>> +#if !defined(HVM_MAX_VCPUS)
>>> +# error HVM_MAX_VCPUS not defined
>>> +#endif
>>> +int main(void) {
>>> +  xs_daemon_open();
>>> +  xc_interface_open();
>>> +  xc_gnttab_open();
>>> +  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>>> +  return 0;
>>> +}
>>> +EOF
>>> +      compile_prog "" "$xen_libs"
>>> +    ) ; then
>>> +    xen_ctrl_version=400
>>> +    xen=yes
>>> +
>>> +  # Xen 3.3.0, 3.4.0
>>> +  elif (
>>> +      cat>    $TMPC<<EOF
>>> +#include<xenctrl.h>
>>> +#include<xs.h>
>>> +int main(void) {
>>> +  xs_daemon_open();
>>> +  xc_interface_open();
>>> +  xc_gnttab_open();
>>> +  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>>> +  return 0;
>>> +}
>>> +EOF
>>> +      compile_prog "" "$xen_libs"
>>> +    ) ; then
>>> +    xen_ctrl_version=330
>>>       xen=yes
>>> +
>>> +  # Xen not found or unsupported
>>>     else
>>>       if test "$xen" = "yes" ; then
>>>         feature_not_found "xen"
>>> @@ -3009,6 +3068,7 @@ case "$target_arch2" in
>>>       if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then
>>>         echo "CONFIG_XEN=y">>    $config_target_mak
>>>         echo "LIBS+=$xen_libs">>    $config_target_mak
>>> +      echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version">>
>>>   $config_target_mak
>>>       fi
>>>   esac
>>>   case "$target_arch2" in
>>> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
>>> index 860b038..cf081e1 100644
>>> --- a/hw/xen_backend.c
>>> +++ b/hw/xen_backend.c
>>> @@ -43,7 +43,8 @@
>>>   /* ------------------------------------------------------------- */
>>>
>>>   /* public */
>>> -int xen_xc;
>>> +XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
>>> +XenGnttab xen_xcg = XC_HANDLER_INITIAL_VALUE;
>>>   struct xs_handle *xenstore = NULL;
>>>   const char *xen_protocol;
>>>
>>> @@ -58,7 +59,7 @@ int xenstore_write_str(const char *base, const char
>>> *node, const char *val)
>>>       char abspath[XEN_BUFSIZE];
>>>
>>>       snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
>>> -    if (!xs_write(xenstore, 0, abspath, val, strlen(val)))
>>> +    if (!xs_ops.write(xenstore, 0, abspath, val, strlen(val)))
>>>           return -1;
>>>       return 0;
>>>   }
>>> @@ -70,7 +71,7 @@ char *xenstore_read_str(const char *base, const char
>>> *node)
>>>       char *str, *ret = NULL;
>>>
>>>       snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
>>> -    str = xs_read(xenstore, 0, abspath,&len);
>>> +    str = xs_ops.read(xenstore, 0, abspath,&len);
>>>
>>>        
>> I think I gave this feedback before but I'd really like to see static
>> inlines here.
>>
>> It's very likely that you'll either want to have tracing or some commands
>> can have a NULL function pointer in which case having a central location to
>> do this is very useful.
>>
>> Plus, it's more natural to read code that's making a function call instead
>> of going through a function pointer in a structure redirection.
>>
>> Can probably do this with just a sed over the current patch.
>>      
>
> Is it good to have a .h with functions like that? :
>
> static inline XenXC qemu_xc_interface_open(xentoollog_logger *logger,
>                              xentoollog_logger *dombuild_logger,
>                              unsigned open_flags)
> {
> #if CONFIG_XEN_CTRL_INTERFACE_VERSION<  410
>      return xc_interface_open();
> #else
>      return xc_interface_open(logger, dombuild_logger, open_flags);
> #endif
> }
>
>
> So there will have no more structure redirection.
>    

It would be better to have two versions of the header, one that 
implemented the < 410 functions and one that implemented the newer 
functions.

If you're just using the new signature for everything, you could even 
just #define in the later header.

Regards,

Anthony Liguori

> Regards,
>
>