Re: [PATCH 1/6] libxc: introduce xc_domain_get_address_size

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/07/13 17:48, Dario Faggioli wrote:
> As a wrapper to XEN_DOMCTL_get_address_size, and use it
> wherever the call was being issued directly via do_domctl(),
> saving quite some line of code.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>

While this is certainly a sensible improvement, there seems to be some
confusion in the code.

> ---
>  tools/libxc/xc_core.c         |   21 ++-------------------
>  tools/libxc/xc_cpuid_x86.c    |    8 +++-----
>  tools/libxc/xc_domain.c       |   15 +++++++++++++++
>  tools/libxc/xc_offline_page.c |    9 ++-------
>  tools/libxc/xc_pagetab.c      |    8 +++-----
>  tools/libxc/xc_resume.c       |   23 ++++++-----------------
>  tools/libxc/xenctrl.h         |   12 ++++++++++++
>  tools/libxc/xg_save_restore.h |    9 ++-------
>  tools/xentrace/xenctx.c       |    9 +++------
>  9 files changed, 48 insertions(+), 66 deletions(-)
>
> diff --git a/tools/libxc/xc_core.c b/tools/libxc/xc_core.c
> index 4207eed..c8bade5 100644
> --- a/tools/libxc/xc_core.c
> +++ b/tools/libxc/xc_core.c
> @@ -417,24 +417,6 @@ elfnote_dump_format_version(xc_interface *xch,
>      return dump_rtn(xch, args, (char*)&format_version, sizeof(format_version));
>  }
>  
> -static int
> -get_guest_width(xc_interface *xch,
> -                uint32_t domid,
> -                unsigned int *guest_width)
> -{
> -    DECLARE_DOMCTL;
> -
> -    memset(&domctl, 0, sizeof(domctl));
> -    domctl.domain = domid;
> -    domctl.cmd = XEN_DOMCTL_get_address_size;
> -
> -    if ( do_domctl(xch, &domctl) != 0 )
> -        return 1;
> -        
> -    *guest_width = domctl.u.address_size.size / 8;
> -    return 0;
> -}
> -

Here, guest_width is measured in bytes.

>  int
>  xc_domain_dumpcore_via_callback(xc_interface *xch,
>                                  uint32_t domid,
> @@ -478,11 +460,12 @@ xc_domain_dumpcore_via_callback(xc_interface *xch,
>      struct xc_core_section_headers *sheaders = NULL;
>      Elf64_Shdr *shdr;
>   
> -    if ( get_guest_width(xch, domid, &dinfo->guest_width) != 0 )
> +    if ( xc_domain_get_address_size(xch, domid, &dinfo->guest_width) != 0 )
>      {
>          PERROR("Could not get address size for domain");
>          return sts;
>      }
> +    dinfo->guest_width /= 8;

This looks nasty (and frankly looks wrong to a cursory glance), and is
because of the functional difference between get_guest_width() and
xc_domain_get_address_size()

>  
>      xc_core_arch_context_init(&arch_ctxt);
>      if ( (dump_mem_start = malloc(DUMP_INCREMENT*PAGE_SIZE)) == NULL )
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index fa47787..99e3997 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -436,17 +436,15 @@ static void xc_cpuid_pv_policy(
>      const unsigned int *input, unsigned int *regs)
>  {
>      DECLARE_DOMCTL;
> +    unsigned int guest_width;
>      int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
>      char brand[13];
>      uint64_t xfeature_mask;
>  
>      xc_cpuid_brand_get(brand);
>  
> -    memset(&domctl, 0, sizeof(domctl));
> -    domctl.domain = domid;
> -    domctl.cmd = XEN_DOMCTL_get_address_size;
> -    do_domctl(xch, &domctl);
> -    guest_64bit = (domctl.u.address_size.size == 64);
> +    xc_domain_get_address_size(xch, domid, &guest_width);
> +    guest_64bit = (guest_width == 64);

guest_width here is now measured in bytes.

Also, error checking?  I know the old code also failed in that regard.

>  
>      /* Detecting Xen's atitude towards XSAVE */
>      memset(&domctl, 0, sizeof(domctl));
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 3257e2a..d64d0bc 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -270,6 +270,21 @@ out:
>      return ret;
>  }
>  
> +int xc_domain_get_address_size(xc_interface *xch, uint32_t domid,
> +                               unsigned int *addr_size)
> +{
> +    DECLARE_DOMCTL;
> +
> +    memset(&domctl, 0, sizeof(domctl));
> +    domctl.domain = domid;
> +    domctl.cmd = XEN_DOMCTL_get_address_size;
> +
> +    if ( do_domctl(xch, &domctl) != 0 )
> +        return 1;
> +
> +    *addr_size = domctl.u.address_size.size;
> +    return 0;
> +}
>  
>  int xc_domain_getinfo(xc_interface *xch,
>                        uint32_t first_domid,
> diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
> index 36b9812..c5547a8 100644
> --- a/tools/libxc/xc_offline_page.c
> +++ b/tools/libxc/xc_offline_page.c
> @@ -193,20 +193,15 @@ static int get_pt_level(xc_interface *xch, uint32_t domid,
>                          unsigned int *pt_level,
>                          unsigned int *gwidth)
>  {
> -    DECLARE_DOMCTL;
>      xen_capabilities_info_t xen_caps = "";
>  
>      if (xc_version(xch, XENVER_capabilities, &xen_caps) != 0)
>          return -1;
>  
> -    memset(&domctl, 0, sizeof(domctl));
> -    domctl.domain = domid;
> -    domctl.cmd = XEN_DOMCTL_get_address_size;
> -
> -    if ( do_domctl(xch, &domctl) != 0 )
> +    if (xc_domain_get_address_size(xch, domid, gwidth) != 0)
>          return -1;
>  
> -    *gwidth = domctl.u.address_size.size / 8;
> +    *gwidth /= 8;

gwidth is now again measured in bytes.

>  
>      if (strstr(xen_caps, "xen-3.0-x86_64"))
>          /* Depends on whether it's a compat 32-on-64 guest */
> diff --git a/tools/libxc/xc_pagetab.c b/tools/libxc/xc_pagetab.c
> index 27c4e9f..5937a52 100644
> --- a/tools/libxc/xc_pagetab.c
> +++ b/tools/libxc/xc_pagetab.c
> @@ -51,15 +51,13 @@ unsigned long xc_translate_foreign_address(xc_interface *xch, uint32_t dom,
>          pt_levels = (ctx.msr_efer&EFER_LMA) ? 4 : (ctx.cr4&CR4_PAE) ? 3 : 2;
>          paddr = ctx.cr3 & ((pt_levels == 3) ? ~0x1full : ~0xfffull);
>      } else {
> -        DECLARE_DOMCTL;
> +        unsigned int gwidth;
>          vcpu_guest_context_any_t ctx;
>          if (xc_vcpu_getcontext(xch, dom, vcpu, &ctx) != 0)
>              return 0;
> -        domctl.domain = dom;
> -        domctl.cmd = XEN_DOMCTL_get_address_size;
> -        if ( do_domctl(xch, &domctl) != 0 )
> +        if (xc_domain_get_address_size(xch, dom, &gwidth) != 0)
>              return 0;
> -        if (domctl.u.address_size.size == 64) {
> +        if (gwidth == 64) {

but in bits here.  ( I shall ignore pointing out the same further down)

>              pt_levels = 4;
>              paddr = (uint64_t)xen_cr3_to_pfn_x86_64(ctx.x64.ctrlreg[3])
>                  << PAGE_SHIFT;
> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> index 1c43ec6..58ea395 100644
> --- a/tools/libxc/xc_resume.c
> +++ b/tools/libxc/xc_resume.c
> @@ -24,19 +24,6 @@
>  #include <xen/foreign/x86_64.h>
>  #include <xen/hvm/params.h>
>  
> -static int pv_guest_width(xc_interface *xch, uint32_t domid)
> -{
> -    DECLARE_DOMCTL;
> -    domctl.domain = domid;
> -    domctl.cmd = XEN_DOMCTL_get_address_size;
> -    if ( xc_domctl(xch, &domctl) != 0 )
> -    {
> -        PERROR("Could not get guest address size");
> -        return -1;
> -    }
> -    return domctl.u.address_size.size / 8;
> -}
> -
>  static int modify_returncode(xc_interface *xch, uint32_t domid)
>  {
>      vcpu_guest_context_any_t ctxt;
> @@ -71,9 +58,9 @@ static int modify_returncode(xc_interface *xch, uint32_t domid)
>      else
>      {
>          /* Probe PV guest address width. */
> -        dinfo->guest_width = pv_guest_width(xch, domid);
> -        if ( dinfo->guest_width < 0 )
> +        if ( xc_domain_get_address_size(xch, domid, &dinfo->guest_width) )
>              return -1;
> +        dinfo->guest_width /= 8;
>      }
>  
>      if ( (rc = xc_vcpu_getcontext(xch, domid, 0, &ctxt)) != 0 )
> @@ -120,7 +107,8 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid)
>      xc_dominfo_t info;
>      int i, rc = -1;
>  #if defined(__i386__) || defined(__x86_64__)
> -    struct domain_info_context _dinfo = { .p2m_size = 0 };
> +    struct domain_info_context _dinfo = { .guest_width = 0,
> +                                          .p2m_size = 0 };
>      struct domain_info_context *dinfo = &_dinfo;
>      unsigned long mfn;
>      vcpu_guest_context_any_t ctxt;
> @@ -147,7 +135,8 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid)
>          return rc;
>      }
>  
> -    dinfo->guest_width = pv_guest_width(xch, domid);
> +    xc_domain_get_address_size(xch, domid, &dinfo->guest_width);
> +    dinfo->guest_width /= 8;
>      if ( dinfo->guest_width != sizeof(long) )
>      {
>          ERROR("Cannot resume uncooperative cross-address-size guests");
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 388a9c3..907106e 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -561,6 +561,18 @@ int xc_vcpu_getaffinity(xc_interface *xch,
>                          int vcpu,
>                          xc_cpumap_t cpumap);
>  
> +
> +/**
> + * This function will return the address size for the specified domain.
> + *
> + * @param xch a handle to an open hypervisor interface.
> + * @param domid the domain id one wants the address size width of.
> + * @param addr_size the address size.
> + */
> +int xc_domain_get_address_size(xc_interface *xch, uint32_t domid,
> +                               unsigned int *addr_size);
> +
> +
>  /**
>   * This function will return information about one or more domains. It is
>   * designed to iterate over the list of domains. If a single domain is
> diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
> index 6512003..3c11c44 100644
> --- a/tools/libxc/xg_save_restore.h
> +++ b/tools/libxc/xg_save_restore.h
> @@ -301,7 +301,6 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
>  {
>      xen_capabilities_info_t xen_caps = "";
>      xen_platform_parameters_t xen_params;
> -    DECLARE_DOMCTL;
>  
>      if (xc_version(xch, XENVER_platform_parameters, &xen_params) != 0)
>          return 0;
> @@ -313,14 +312,10 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
>  
>      *hvirt_start = xen_params.virt_start;
>  
> -    memset(&domctl, 0, sizeof(domctl));
> -    domctl.domain = dom;
> -    domctl.cmd = XEN_DOMCTL_get_address_size;
> -
> -    if ( do_domctl(xch, &domctl) != 0 )
> +    if ( xc_domain_get_address_size(xch, dom, guest_width) != 0)
>          return 0; 
>  
> -    *guest_width = domctl.u.address_size.size / 8;
> +    *guest_width /= 8;
>  
>      /* 64-bit tools will see the 64-bit hvirt_start, but 32-bit guests 
>       * will be using the compat one. */
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index 060e480..ae4d6a7 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -771,12 +771,9 @@ static void dump_ctx(int vcpu)
>              }
>              ctxt_word_size = (strstr(xen_caps, "xen-3.0-x86_64")) ? 8 : 4;
>          } else {
> -            struct xen_domctl domctl;
> -            memset(&domctl, 0, sizeof domctl);
> -            domctl.domain = xenctx.domid;
> -            domctl.cmd = XEN_DOMCTL_get_address_size;
> -            if (xc_domctl(xenctx.xc_handle, &domctl) == 0)
> -                ctxt_word_size = guest_word_size = domctl.u.address_size.size / 8;
> +            unsigned int gw;
> +            if ( !xc_domain_get_address_size(xenctx.xc_handle, xenctx.domid, &gw) )
> +                ctxt_word_size = guest_word_size = gw / 8;
>          }
>      }
>  #endif

Almost all of the "/= 8"s could be removed by moving it into
xc_domain_get_address_size().

I suggest renaming xc_domain_get_address_size() to
xc_domain_get_address_width() and changing the one location which checks
against 64 (bits) to check against 8 (bytes).

~Andrew

>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel