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

[Anthony PERARD <anthony.perard@citrix.com>]

On Wed, Mar 23, 2011 at 10:43, Alexander Graf <agraf@suse.de> wrote:
>
> On 01.03.2011, at 19:35, Anthony.Perard@citrix.com wrote:
>
>> From: Anthony PERARD <anthony.perard@citrix.com>
>>
>> This patch updates 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>
>> ---
>> configure            |   67 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> hw/xen_backend.c     |   21 ++++++++-------
>> hw/xen_backend.h     |    6 ++--
>> hw/xen_common.h      |   64 +++++++++++++++++++++++++++++++++++++----------
>> hw/xen_disk.c        |    4 +-
>> hw/xen_domainbuild.c |    3 +-
>> 6 files changed, 133 insertions(+), 32 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 3036faf..a84d974 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=""
>> @@ -1147,20 +1148,81 @@ 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
>> -    libs_softmmu="$xen_libs $libs_softmmu"
>> +
>> +  # 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"
>>     fi
>>     xen=no
>>   fi
>> +
>> +  if test "$xen" = yes; then
>> +    libs_softmmu="$xen_libs $libs_softmmu"
>> +  fi
>> fi
>>
>> ##########################################
>> @@ -2755,6 +2817,7 @@ if test "$bluez" = "yes" ; then
>> fi
>> if test "$xen" = "yes" ; then
>>   echo "CONFIG_XEN=y" >> $config_host_mak
>> +  echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak
>> fi
>> if test "$io_thread" = "yes" ; then
>>   echo "CONFIG_IOTHREAD=y" >> $config_host_mak
>> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
>> index 9f4ec4b..3907b83 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;
>>
>> @@ -214,8 +215,8 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>>     xendev->debug      = debug;
>>     xendev->local_port = -1;
>>
>> -    xendev->evtchndev = xc_evtchn_open();
>> -    if (xendev->evtchndev < 0) {
>> +    xendev->evtchndev = xc_evtchn_open(NULL, 0);
>> +    if (xendev->evtchndev == XC_HANDLER_INITIAL_VALUE) {
>>         xen_be_printf(NULL, 0, "can't open evtchn device\n");
>>         qemu_free(xendev);
>>         return NULL;
>> @@ -223,15 +224,15 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>>     fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
>>
>>     if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
>> -        xendev->gnttabdev = xc_gnttab_open();
>> -        if (xendev->gnttabdev < 0) {
>> +        xendev->gnttabdev = xc_gnttab_open(NULL, 0);
>> +        if (xendev->gnttabdev == XC_HANDLER_INITIAL_VALUE) {
>>             xen_be_printf(NULL, 0, "can't open gnttab device\n");
>>             xc_evtchn_close(xendev->evtchndev);
>>             qemu_free(xendev);
>>             return NULL;
>>         }
>>     } else {
>> -        xendev->gnttabdev = -1;
>> +        xendev->gnttabdev = XC_HANDLER_INITIAL_VALUE;
>>     }
>>
>>     QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
>> @@ -277,10 +278,10 @@ static struct XenDevice *xen_be_del_xendev(int dom, int dev)
>>             qemu_free(xendev->fe);
>>         }
>>
>> -        if (xendev->evtchndev >= 0) {
>> +        if (xendev->evtchndev != XC_HANDLER_INITIAL_VALUE) {
>>             xc_evtchn_close(xendev->evtchndev);
>>         }
>> -        if (xendev->gnttabdev >= 0) {
>> +        if (xendev->gnttabdev != XC_HANDLER_INITIAL_VALUE) {
>>             xc_gnttab_close(xendev->gnttabdev);
>>         }
>>
>> @@ -664,8 +665,8 @@ int xen_be_init(void)
>>         goto err;
>>     }
>>
>> -    xen_xc = xc_interface_open();
>> -    if (xen_xc == -1) {
>> +    xen_xc = xc_interface_open(0, 0, 0);
>> +    if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
>>         xen_be_printf(NULL, 0, "can't open xen interface\n");
>>         goto err;
>>     }
>> diff --git a/hw/xen_backend.h b/hw/xen_backend.h
>> index 1b428e3..6401c85 100644
>> --- a/hw/xen_backend.h
>> +++ b/hw/xen_backend.h
>> @@ -45,8 +45,8 @@ struct XenDevice {
>>     int                remote_port;
>>     int                local_port;
>>
>> -    int                evtchndev;
>> -    int                gnttabdev;
>> +    XenEvtchn          evtchndev;
>> +    XenGnttab          gnttabdev;
>>
>>     struct XenDevOps   *ops;
>>     QTAILQ_ENTRY(XenDevice) next;
>> @@ -55,7 +55,7 @@ struct XenDevice {
>> /* ------------------------------------------------------------- */
>>
>> /* variables */
>> -extern int xen_xc;
>> +extern XenXC xen_xc;
>> extern struct xs_handle *xenstore;
>> extern const char *xen_protocol;
>>
>> diff --git a/hw/xen_common.h b/hw/xen_common.h
>> index 8a55b44..7e123ec 100644
>> --- a/hw/xen_common.h
>> +++ b/hw/xen_common.h
>> @@ -1,6 +1,8 @@
>> #ifndef QEMU_HW_XEN_COMMON_H
>> #define QEMU_HW_XEN_COMMON_H 1
>>
>> +#include "config-host.h"
>> +
>> #include <stddef.h>
>> #include <inttypes.h>
>>
>> @@ -13,22 +15,56 @@
>> #include "qemu-queue.h"
>>
>> /*
>> - * tweaks needed to build with different xen versions
>> - *  0x00030205 -> 3.1.0
>> - *  0x00030207 -> 3.2.0
>> - *  0x00030208 -> unstable
>> + * We don't support Xen prior to 3.3.0.
>>  */
>> -#include <xen/xen-compat.h>
>> -#if __XEN_LATEST_INTERFACE_VERSION__ < 0x00030205
>> -# define evtchn_port_or_error_t int
>> -#endif
>> -#if __XEN_LATEST_INTERFACE_VERSION__ < 0x00030207
>> -# define xc_map_foreign_pages xc_map_foreign_batch
>> +
>> +/* Xen before 4.0 */
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 400
>> +static inline void *xc_map_foreign_bulk(int xc_handle, uint32_t dom, int prot,
>> +                                        xen_pfn_t *arr, int *err,
>> +                                        unsigned int num)
>> +{
>> +    return xc_map_foreign_batch(xc_handle, dom, prot, arr, num);
>> +}
>> #endif
>> -#if __XEN_LATEST_INTERFACE_VERSION__ < 0x00030208
>> -# define xen_mb()  mb()
>> -# define xen_rmb() rmb()
>> -# define xen_wmb() wmb()
>> +
>> +
>> +/* Xen before 4.1 */
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 410
>> +
>> +typedef int XenXC;
>> +typedef int XenEvtchn;
>> +typedef int XenGnttab;
>> +
>> +#  define XC_INTERFACE_FMT "%i"
>> +#  define XC_HANDLER_INITIAL_VALUE    -1
>> +
>> +#  define xc_interface_open(logger, dombuild_logger, open_flags) \
>> +    xc_interface_open()
>> +#  define xc_evtchn_open(logger, open_flags) xc_evtchn_open()
>> +#  define xc_gnttab_open(logger, open_flags) xc_gnttab_open()
>
>
> defines really bite me when abstracting the interface through an indirect calling table. I had to change the code this way to make it work with xenner:
>
>
> diff --git a/hw/xen_common.h b/hw/xen_common.h
> index a5fc74b..0c09b37 100644
> --- a/hw/xen_common.h
> +++ b/hw/xen_common.h
> @@ -39,10 +39,23 @@ typedef int XenGnttab;
>  #  define XC_INTERFACE_FMT "%i"
>  #  define XC_HANDLER_INITIAL_VALUE    -1
>
> -#  define xc_interface_open(logger, dombuild_logger, open_flags) \
> -    xc_interface_open()
> -#  define xc_evtchn_open(logger, open_flags) xc_evtchn_open()
> -#  define xc_gnttab_open(logger, open_flags) xc_gnttab_open()
> +static inline XenEvtchn xen_xc_evtchn_open(void *logger,
> +                                           unsigned int open_flags)
> +{
> +    return xc_evtchn_open();
> +}
> +
> +static inline XenGnttab xen_xc_gnttab_open(void *logger,
> +                                           unsigned int open_flags)
> +{
> +    return xc_gnttab_open();
> +}
> +
> +static inline XenXC xen_xc_interface_open(void *logger, void *dombuild_logger,
> +                                          unsigned int open_flags)
> +{
> +    return xc_interface_open();
> +}
>
>  static inline int xc_fd(int xen_xc)
>  {
> @@ -69,6 +82,24 @@ typedef xc_gnttab *XenGnttab;
>  #  define XC_INTERFACE_FMT "%p"
>  #  define XC_HANDLER_INITIAL_VALUE    NULL
>
> +static inline XenEvtchn xen_xc_evtchn_open(void *logger,
> +                                           unsigned int open_flags)
> +{
> +    return xc_evtchn_open(logger, open_flags);
> +}
> +
> +static inline XenGnttab xen_xc_gnttab_open(void *logger,
> +                                           unsigned int open_flags)
> +{
> +    return xc_gnttab_open(logger, open_flags);
> +}
> +
> +static inline XenXC xen_xc_interface_open(void *logger, void *dombuild_logger,
> +                                          int open_flags)
> +{
> +    return xc_interface_open(logger, dombuild_logger, open_flags);
> +}
> +
>  /* FIXME There is now way to have the xen fd */
>  static inline int xc_fd(xc_interface *xen_xc)
>  {
>
>
> Leave it as is in your patch and I'll change it in my xenner patch set again, but I wanted to make sure that you're aware of the issues going along with #defines.

I understand the issues with the defines. I just trying to address
both comment from Anthony and you. But, I think these static inline
instead of the #defines will be ok. So I will put it in my series.

-- 
Anthony PERARD