Re: [RFC XEN PATCH 09/16] tools/libacpi: add callbacks to access XenStore

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Mon, Oct 10, 2016 at 08:32:28AM +0800, Haozhong Zhang wrote:
> libacpi needs to access information placed in XenStore in order to load
> ACPI built by the device model.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/util.c   | 50 +++++++++++++++++++++++++++++++++++++++
>  tools/firmware/hvmloader/util.h   |  2 ++
>  tools/firmware/hvmloader/xenbus.c | 20 ++++++++++++++++
>  tools/libacpi/libacpi.h           | 10 ++++++++
>  tools/libxl/libxl_x86_acpi.c      | 24 +++++++++++++++++++
>  5 files changed, 106 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 504ae6a..dba954a 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -888,6 +888,51 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
>      /* ACPI builder currently doesn't free memory so this is just a stub */
>  }
>  
> +static const char *acpi_xs_read(struct acpi_ctxt *ctxt, const char *path)
> +{
> +    return xenstore_read(path, NULL);
> +}
> +
> +static int acpi_xs_write(struct acpi_ctxt *ctxt,
> +                         const char *path, const char *value)
> +{
> +    return xenstore_write(path, value);
> +}
> +
> +static unsigned int count_strings(const char *strings, unsigned int len)
> +{
> +    const char *p;
> +    unsigned int n;
> +
> +    for ( p = strings, n = 0; p < strings + len; p++ )
> +        if ( *p == '\0' )
> +            n++;
> +
> +    return n;
> +}
> +
> +static char **acpi_xs_directory(struct acpi_ctxt *ctxt,
> +                                const char *path, unsigned int *num)
> +{
> +    const char *strings;
> +    char *s, *p, **ret;
> +    unsigned int len, n;
> +
> +    strings = xenstore_directory(path, &len, NULL);
> +    if ( !strings )
> +        return NULL;
> +
> +    n = count_strings(strings, len);
> +    ret = ctxt->mem_ops.alloc(ctxt, n * sizeof(char *) + len, 0);

sizeof(*s)

But you may also check ret against NULL before you memcpy data in there.


> +    memcpy(&ret[n], strings, len);
> +
> +    s = (char *)&ret[n];
> +    for ( p = s, *num = 0; p < s + len; p+= strlen(p) + 1 )

Perhaps add an space before += ?
> +        ret[(*num)++] = p;
> +
> +    return ret;
> +}
> +
>  static uint8_t acpi_lapic_id(unsigned cpu)
>  {
>      return LAPIC_ID(cpu);
> @@ -975,6 +1020,11 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>      ctxt.min_alloc_unit = PAGE_SIZE;
>      ctxt.min_alloc_align = 16;
>  
> +    ctxt.xs_ops.read = acpi_xs_read;
> +    ctxt.xs_ops.write = acpi_xs_write;
> +    ctxt.xs_ops.directory = acpi_xs_directory;
> +    ctxt.xs_opaque = NULL;
> +
>      acpi_build_tables(&ctxt, config);
>  
>      hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->vm_gid_addr);
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 6a50dae..9443673 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -225,6 +225,8 @@ const char *xenstore_read(const char *path, const char *default_resp);
>   */
>  int xenstore_write(const char *path, const char *value);
>  
> +const char *xenstore_directory(const char *path, uint32_t *len,
> +                               const char *default_resp);
>  
>  /* Get a HVM param.
>   */
> diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c
> index 448157d..70bdadd 100644
> --- a/tools/firmware/hvmloader/xenbus.c
> +++ b/tools/firmware/hvmloader/xenbus.c
> @@ -296,6 +296,26 @@ int xenstore_write(const char *path, const char *value)
>      return ret;
>  }
>  
> +const char *xenstore_directory(const char *path, uint32_t *len,
> +                               const char *default_resp)
> +{
> +    uint32_t type = 0;
> +    const char *answer = NULL;
> +
> +    xenbus_send(XS_DIRECTORY,
> +                path, strlen(path),
> +                "", 1, /* nul separator */
> +                NULL, 0);
> +
> +    if ( xenbus_recv(len, &answer, &type) || (type != XS_DIRECTORY) )
> +        answer = NULL;
> +
> +    if ( (default_resp != NULL) && ((answer == NULL) || (*answer == '\0')) )
> +        answer = default_resp;
> +
> +    return answer;

This function looks very similar to xenstore_read. Could xenstore_read
become __xenstore_read with an extra argument (type) and then the
new xenstore_read along with xenstore_dir would call in it?
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index 0fb16e7..12cafd8 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -50,6 +50,16 @@ struct acpi_ctxt {
>  
>      uint32_t min_alloc_unit;
>      uint32_t min_alloc_align;
> +
> +    struct acpi_xs_ops {
> +        const char *(*read)(struct acpi_ctxt *ctxt, const char *path);
> +        int (*write)(struct acpi_ctxt *ctxt,
> +                     const char *path, const char *value);
> +        char **(*directory)(struct acpi_ctxt *ctxt,
> +                            const char *path, unsigned int *num);
> +    } xs_ops;
> +
> +    void *xs_opaque;
>  };
>  
>  struct acpi_config {
> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> index baf60ac..1afd2e3 100644
> --- a/tools/libxl/libxl_x86_acpi.c
> +++ b/tools/libxl/libxl_x86_acpi.c
> @@ -93,6 +93,25 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
>  {
>  }
>  
> +static const char *acpi_xs_read(struct acpi_ctxt *ctxt, const char *path)
> +{
> +    return libxl__xs_read((libxl__gc *)ctxt->xs_opaque, XBT_NULL, path);
> +}
> +
> +static int acpi_xs_write(struct acpi_ctxt *ctxt,
> +                         const char *path, const char *value)
> +{
> +    return libxl__xs_write_checked((libxl__gc *)ctxt->xs_opaque, XBT_NULL,
> +                                   path, value);
> +}
> +
> +static char **acpi_xs_directory(struct acpi_ctxt *ctxt,
> +                                const char *path, unsigned int *num)
> +{
> +    return libxl__xs_directory((libxl__gc *)ctxt->xs_opaque, XBT_NULL,
> +                               path, num);
> +}
> +
>  static uint8_t acpi_lapic_id(unsigned cpu)
>  {
>      return cpu * 2;
> @@ -190,6 +209,11 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>      libxl_ctxt.c.min_alloc_unit = libxl_ctxt.page_size;
>      libxl_ctxt.c.min_alloc_align = 16;
>  
> +    libxl_ctxt.c.xs_ops.read = acpi_xs_read;
> +    libxl_ctxt.c.xs_ops.write = acpi_xs_write;
> +    libxl_ctxt.c.xs_ops.directory = acpi_xs_directory;
> +    libxl_ctxt.c.xs_opaque = gc;
> +
>      rc = init_acpi_config(gc, dom, b_info, &config);
>      if (rc) {
>          LOG(ERROR, "init_acpi_config failed (rc=%d)", rc);
> -- 
> 2.10.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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