Re: [Xen-devel] [PATCH v2 22/23] libxl/acpi: Build ACPI tables for HVMlite guests

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 04, 2016 at 05:06:50PM -0400, Boris Ostrovsky wrote:
[...]
>  
>  distclean: clean
>       $(RM) -f xenlight.pc.in xlutil.pc.in
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 34a853c..7c6536b 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -62,4 +62,7 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
>                                          uint32_t domid,
>                                          struct xc_dom_image *dom);
>  
> +int libxl__dom_load_acpi(libxl__gc *gc,
> +                      libxl_domain_build_info *info,
> +                      struct xc_dom_image *dom);
>  #endif
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 3a1daae..7dbf614 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -657,6 +657,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "xc_dom_build_image failed");
>          goto out;
>      }
> +

Stray blank line.

>      if ( (ret = xc_dom_boot_image(dom)) != 0 ) {
>          LOGE(ERROR, "xc_dom_boot_image failed");
>          goto out;
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 7654e20..42f2139 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -8,15 +8,18 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>                                        xc_domain_configuration_t *xc_config)
>  {
>  
> -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> -        d_config->b_info.device_model_version !=
> -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
> -        /* HVM domains with a device model. */
> -        xc_config->emulation_flags = XEN_X86_EMU_ALL;
> -    } else {
> -        /* PV or HVM domains without a device model. */
> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
> +        if (d_config->b_info.device_model_version !=
> +            LIBXL_DEVICE_MODEL_VERSION_NONE)
> +            xc_config->emulation_flags = XEN_X86_EMU_ALL;
> +        else if (libxl_defbool_val(d_config->b_info.u.hvm.apic))
> +            /*
> +             * HVM guests without device model may want
> +             * to have LAPIC emulation.
> +             */
> +            xc_config->emulation_flags = XEN_X86_EMU_LAPIC;
> +    } else 

Please retain {} for the else branch. We recently updated the
CODING_STYLE a bit.

>          xc_config->emulation_flags = 0;
> -    }
>  
>      return 0;
>  }
> @@ -366,7 +369,15 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc 
> *gc,
>                                                 libxl_domain_build_info *info,
>                                                 struct xc_dom_image *dom)
>  {
> -    return 0;
> +    int ret = 0;
> +
> +    if ((info->type == LIBXL_DOMAIN_TYPE_HVM) &&
> +        (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)) {
> +        if ( (ret = libxl__dom_load_acpi(gc, info, dom)) != 0 )
> +            LOGE(ERROR, "libxl_dom_load_acpi failed");
> +    }
> +
> +    return ret;
>  }
>  
>  /* Return 0 on success, ERROR_* on failure. */
> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> new file mode 100644
> index 0000000..88e69e2
> --- /dev/null
> +++ b/tools/libxl/libxl_x86_acpi.c

I only skimmed this file. I think it's mostly code movement and believe
the bug here should be easy to fix.

> @@ -0,0 +1,199 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#include "libxl_internal.h"
> +#include "libxl_arch.h"
> +#include <xen/hvm/hvm_info_table.h>
> +#include <xen/hvm/e820.h>
> +#include "libacpi/libacpi.h"
> +
> +#include <xc_dom.h>
> +
> + /* Number of pages holding ACPI tables */
> +#define NUM_ACPI_PAGES 16
> +/* Store RSDP in the last 64 bytes of BIOS RO memory */
> +#define RSDP_ADDRESS (0x100000 - 64)
> +#define ACPI_INFO_PHYSICAL_ADDRESS 0xfc000000
> +
> +extern const unsigned char dsdt_pvh[];
> +extern const unsigned int dsdt_pvh_len;
> +
> +/* Assumes contiguous physical space */
> +static unsigned long virt_to_phys(struct acpi_ctxt *ctxt, void *v)
> +{
> +    return (((unsigned long)v - ctxt->alloc_base_vaddr) +
> +            ctxt->alloc_base_paddr);
> +}
> +
> +static void *mem_alloc(struct acpi_ctxt *ctxt, uint32_t size, uint32_t align)
> +{
> +    unsigned long s, e;
> +
> +    /* Align to at least 16 bytes. */
> +    if (align < 16)
> +        align = 16;
> +
> +    s = (ctxt->alloc_currp + align) & ~((unsigned long)align - 1);
> +    e = s + size - 1;
> +
> +    /* TODO: Reallocate memory */
> +    if ((e < s) || (e >= ctxt->alloc_end)) return NULL;
> +

Please put return NULL on a new line.

> +    while (ctxt->alloc_currp >> ctxt->page_shift != 
> +           e >> ctxt->page_shift)
> +        ctxt->alloc_currp += ctxt->page_size;
> +
> +    ctxt->alloc_currp = e;
> +
> +    return (void *)s;
> +}
> +
> +static int init_acpi_config(libxl__gc *gc, 
> +                            struct xc_dom_image *dom,
> +                            libxl_domain_build_info *b_info,
> +                            struct acpi_config *config)
> +{
> +    xc_interface *xch = dom->xch;
> +    uint32_t domid = dom->guest_domid;
> +    xc_dominfo_t info;
> +    int i, rc;
> +
> +    config->dsdt_anycpu = config->dsdt_15cpu = dsdt_pvh;
> +    config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_pvh_len;
> +
> +    rc = xc_domain_getinfo(xch, domid, 1, &info);
> +    if (rc < 0) {
> +        LOG(ERROR, "%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc);
> +        return rc;
> +    }
> +
> +    config->hvminfo = libxl__zalloc(gc, sizeof(*config->hvminfo));
> +
> +    config->hvminfo->apic_mode = libxl_defbool_val(b_info->u.hvm.apic);
> +
> +    if (dom->nr_vnodes) {
> +        struct acpi_numa *numa = &config->numa;
> +
> +        rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
> +                                &numa->nr_vmemranges,
> +                                &config->hvminfo->nr_vcpus, NULL, NULL, 
> NULL);
> +     if (rc) {
> +            LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)",
> +                __FUNCTION__, rc);
> +            return rc;
> +        }

Indentation.

> +
> +        numa->vmemrange = libxl__zalloc(gc, dom->nr_vmemranges *
> +                                        sizeof(*numa->vmemrange));
> +        numa->vdistance = libxl__zalloc(gc, dom->nr_vnodes *
> +                                         sizeof(*numa->vdistance));
> +        numa->vcpu_to_vnode = libxl__zalloc(gc, config->hvminfo->nr_vcpus *
> +                                             sizeof(*numa->vcpu_to_vnode));
> +        rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
> +                                &numa->nr_vmemranges,
> +                                &config->hvminfo->nr_vcpus, numa->vmemrange,
> +                                numa->vdistance, numa->vcpu_to_vnode);
> +     if (rc) {
> +            LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)",
> +                __FUNCTION__, rc);
> +            return rc;
> +        }
> +    }
> +    else
> +        config->hvminfo->nr_vcpus = info.max_vcpu_id + 1;
> +
> +    for (i=0; i<config->hvminfo->nr_vcpus; i++)
> +        config->hvminfo->vcpu_online[i / 8] |= 1 << (i & 7);
> +
> +    return 0;
> +}
> +
> +int libxl__dom_load_acpi(libxl__gc *gc,
> +                       libxl_domain_build_info *info,
> +                       struct xc_dom_image *dom)

Indentation.

And please constify stuff that don't get changed -- info.


Wei.

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