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

[Anthony PERARD]

On Thu, Feb 24, 2011 at 17:29, Anthony Liguori <address@hidden> wrote:
> On 02/02/2011 08:49 AM, address@hidden wrote:
>>
>> From: Anthony PERARD<address@hidden>
>>
>> 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<address@hidden>
>> Signed-off-by: Stefano Stabellini<address@hidden>
>> Acked-by: Alexander Graf<address@hidden>
>> ---
>>  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.

Regards,

-- 
Anthony PERARD