|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 20/21] libxl/acpi: Build ACPI tables for HVMlite guests
On Mon, Sep 19, 2016 at 08:19:38PM -0400, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
The code mostly looks good. Some nits below.
> ---
> Changes in v4:
> * Remove allocation-specific fields from struct acpi_ctxt and use
> an enclosing struct libxl_acpi_ctxt.
> * Use private struct hvminfo (to deal with constified struct
> acpi_config->hvminfo)
>
> .gitignore | 12 ++-
> tools/libacpi/build.c | 7 +-
> tools/libacpi/libacpi.h | 6 +-
> tools/libxl/Makefile | 18 +++-
> tools/libxl/libxl_arch.h | 3 +
> tools/libxl/libxl_x86.c | 30 ++++--
> tools/libxl/libxl_x86_acpi.c | 243
> +++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_x86_acpi.h | 35 +++++++
> 8 files changed, 334 insertions(+), 20 deletions(-)
> create mode 100644 tools/libxl/libxl_x86_acpi.c
> create mode 100644 tools/libxl/libxl_x86_acpi.h
>
> diff --git a/.gitignore b/.gitignore
> index 5720e0f..9b6f58e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -173,15 +173,19 @@ tools/include/xen/*
> tools/include/xen-xsm/*
> tools/include/xen-foreign/*.(c|h|size)
> tools/include/xen-foreign/checker
> -tools/libxl/libxlu_cfg_y.output
> +tools/libxl/_libxl.api-for-check
> +tools/libxl/*.api-ok
> tools/libxl/*.pc
> tools/libxl/*.pc.in
> -tools/libxl/xl
> +tools/libxl/dsdt*.c
> +tools/libxl/dsdt_*.asl
> +tools/libxl/libxlu_cfg_y.output
> +tools/libxl/mk_dsdt
> +tools/libxl/ssdt_*.h
> tools/libxl/testenum
> tools/libxl/testenum.c
> tools/libxl/tmp.*
> -tools/libxl/_libxl.api-for-check
> -tools/libxl/*.api-ok
> +tools/libxl/xl
> tools/misc/cpuperf/cpuperf-perfcntr
> tools/misc/cpuperf/cpuperf-xen
> tools/misc/xc_shadow
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index 00fb67e..47dae01 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -20,6 +20,7 @@
> #include "ssdt_s4.h"
> #include "ssdt_tpm.h"
> #include "ssdt_pm.h"
> +#include <xen/hvm/hvm_info_table.h>
> #include <xen/hvm/hvm_xs_strings.h>
> #include <xen/hvm/params.h>
> #include <xen/memory.h>
> @@ -496,7 +497,7 @@ static int new_vm_gid(struct acpi_ctxt *ctxt,
> return 1;
> }
>
> -void acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
> +int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
> {
> struct acpi_info *acpi_info;
> struct acpi_20_rsdp *rsdp;
> @@ -631,11 +632,11 @@ void acpi_build_tables(struct acpi_ctxt *ctxt, struct
> acpi_config *config)
> if ( !new_vm_gid(ctxt, config, acpi_info) )
> goto oom;
>
> - return;
> + return 0;
>
> oom:
> printf("unable to build ACPI tables: out of memory\n");
> -
> + return -1;
> }
>
> /*
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index e386362..1d388f9 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -78,10 +78,10 @@ struct acpi_config {
> * This must match the OperationRegion(BIOS, SystemMemory, ....)
> * definition in the DSDT
> */
> - unsigned int infop;
> + unsigned long infop;
>
> /* RSDP address */
> - unsigned int rsdp;
> + unsigned long rsdp;
>
> /* x86-specific parameters */
> uint8_t (*lapic_id)(unsigned cpu);
> @@ -91,7 +91,7 @@ struct acpi_config {
> uint8_t ioapic_id;
> };
>
> -void acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config);
> +int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config);
>
> #endif /* __LIBACPI_H__ */
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 90427ff..336358c 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -75,7 +75,21 @@ else
> LIBXL_OBJS-y += libxl_no_colo.o
> endif
>
> -LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
> +ACPI_PATH = $(XEN_ROOT)/tools/libacpi
> +ACPI_FILES = dsdt_pvh.c
> +ACPI_OBJS = $(patsubst %.c,%.o,$(ACPI_FILES)) build.o static_tables.o
> +$(ACPI_FILES): acpi
> +$(ACPI_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"$(CURDIR)/libxl_x86_acpi.h\"
> +vpath build.c $(ACPI_PATH)/
> +vpath static_tables.c $(ACPI_PATH)/
> +LIBXL_OBJS-$(CONFIG_X86) += $(ACPI_OBJS)
> +
> +.PHONY: acpi
> +acpi:
> + $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR)
> +-include acpi
Useless include?
> +
> +LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
> libxl_x86_acpi.o
> LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
>
> ifeq ($(CONFIG_NetBSD),y)
> @@ -167,6 +181,7 @@ $(XL_OBJS): CFLAGS += $(CFLAGS_XL)
> $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h
> needs it.
>
> libxl_dom.o: CFLAGS += -I$(XEN_ROOT)/tools # include libacpi/x86.h
> +libxl_x86_acpi.o: CFLAGS += -I$(XEN_ROOT)/tools
>
> SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o
> $(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenevtchn)
> @@ -309,6 +324,7 @@ clean:
> $(RM) -f testidl.c.new testidl.c *.api-ok
> $(RM) -f xenlight.pc
> $(RM) -f xlutil.pc
> + $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) clean
>
> 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 253a037..7a70b01 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -66,6 +66,9 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
>
> #define LAPIC_BASE_ADDRESS 0xfee00000
>
> +int libxl__dom_load_acpi(libxl__gc *gc,
> + const libxl_domain_build_info *b_info,
> + struct xc_dom_image *dom);
Use space instead of tab please.
> #endif
>
> #endif
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 2b221aa..d10b04b 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -8,15 +8,19 @@ 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
> xc_config->emulation_flags = 0;
> - }
>
Coding style nit: We now forbid an else branch without braces. Please
add them here.
> return 0;
> }
> @@ -366,7 +370,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;
Use rc please.
> +
> + 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 )
Extraneous spaces here. And lift ret= bit out of if statement.
> + 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..1b38116
> --- /dev/null
> +++ b/tools/libxl/libxl_x86_acpi.c
> @@ -0,0 +1,243 @@
> +/*
> + * 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
> +
> +struct libxl_acpi_ctxt {
> + struct acpi_ctxt c;
> +
> + unsigned int page_size;
> + unsigned int page_shift;
> +
> + /* Memory allocator */
> + unsigned long alloc_base_paddr;
> + unsigned long alloc_base_vaddr;
> + unsigned long alloc_currp;
> + unsigned long alloc_end;
> +};
> +
> +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)
> +{
> + struct libxl_acpi_ctxt *libxl_ctxt =
> + CONTAINER_OF(ctxt, struct libxl_acpi_ctxt, c);
> +
> + return (((unsigned long)v - libxl_ctxt->alloc_base_vaddr) +
> + libxl_ctxt->alloc_base_paddr);
> +}
> +
> +static void *mem_alloc(struct acpi_ctxt *ctxt,
> + uint32_t size, uint32_t align)
> +{
> + struct libxl_acpi_ctxt *libxl_ctxt =
> + CONTAINER_OF(ctxt, struct libxl_acpi_ctxt, c);
> + unsigned long s, e;
> +
> + /* Align to at least 16 bytes. */
> + if (align < 16)
> + align = 16;
> +
> + s = (libxl_ctxt->alloc_currp + align) & ~((unsigned long)align - 1);
> + e = s + size - 1;
> +
> + /* TODO: Reallocate memory */
> + if ((e < s) || (e >= libxl_ctxt->alloc_end))
> + return NULL;
> +
> + while (libxl_ctxt->alloc_currp >> libxl_ctxt->page_shift !=
> + e >> libxl_ctxt->page_shift)
> + libxl_ctxt->alloc_currp += libxl_ctxt->page_size;
> +
> + libxl_ctxt->alloc_currp = e;
> +
> + return (void *)s;
> +}
> +
> +static void acpi_mem_free(struct acpi_ctxt *ctxt,
> + void *v, uint32_t size)
> +{
> +}
> +
> +static uint8_t acpi_lapic_id(unsigned cpu)
> +{
> + return cpu * 2;
Is this in accordance with hardware spec?
> +}
> +
> +static int init_acpi_config(libxl__gc *gc,
> + struct xc_dom_image *dom,
> + const libxl_domain_build_info *b_info,
> + struct acpi_config *config)
> +{
Use goto style error handling in this function please.
> + xc_interface *xch = dom->xch;
> + uint32_t domid = dom->guest_domid;
> + xc_dominfo_t info;
> + struct hvm_info_table *hvminfo;
> + 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);
No need to have __FUNCTION__ -- LOG already prints that.
> + return rc;
> + }
> +
> + hvminfo = libxl__zalloc(gc, sizeof(*hvminfo));
> +
> + hvminfo->apic_mode = libxl_defbool_val(b_info->u.hvm.apic);
> +
> + if (dom->nr_vnodes) {
> + unsigned int *vcpu_to_vnode, *vdistance;
> + struct xen_vmemrange *vmemrange;
> + struct acpi_numa *numa = &config->numa;
> +
> + rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
> + &numa->nr_vmemranges,
> + &hvminfo->nr_vcpus, NULL, NULL, NULL);
> + if (rc) {
> + LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)",
> + __FUNCTION__, rc);
> + return rc;
> + }
> +
> + vmemrange = libxl__zalloc(gc, dom->nr_vmemranges *
> sizeof(*vmemrange));
> + vdistance = libxl__zalloc(gc, dom->nr_vnodes * sizeof(*vdistance));
> + vcpu_to_vnode = libxl__zalloc(gc, hvminfo->nr_vcpus *
> + sizeof(*vcpu_to_vnode));
> + rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
> + &numa->nr_vmemranges, &hvminfo->nr_vcpus,
> + vmemrange, vdistance, vcpu_to_vnode);
> + if (rc) {
> + LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)",
> + __FUNCTION__, rc);
> + return rc;
> + }
> + numa->vmemrange = vmemrange;
> + numa->vdistance = vdistance;
> + numa->vcpu_to_vnode = vcpu_to_vnode;
> + }
> + else
> + hvminfo->nr_vcpus = info.max_vcpu_id + 1;
"else" should be right after "}" and please add braces.
> +
> + for (i = 0; i < hvminfo->nr_vcpus; i++)
> + hvminfo->vcpu_online[i / 8] |= 1 << (i & 7);
> +
> + config->hvminfo = hvminfo;
> +
> + config->lapic_base_address = LAPIC_BASE_ADDRESS;
> + config->lapic_id = acpi_lapic_id;
> +
> + return 0;
> +}
> +
> +int libxl__dom_load_acpi(libxl__gc *gc,
> + const libxl_domain_build_info *b_info,
> + struct xc_dom_image *dom)
> +{
> + struct acpi_config config = {0};
> + struct libxl_acpi_ctxt libxl_ctxt;
> + int rc, acpi_pages_num;
> + void *acpi_pages;
> + unsigned long page_mask;
> +
> + if ((b_info->type != LIBXL_DOMAIN_TYPE_HVM) ||
> + (b_info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE))
> + return 0;
Please don't use mixed-style error handling.
> +
> + libxl_ctxt.page_size = XC_DOM_PAGE_SIZE(dom);
> + libxl_ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom);
> + page_mask = (1UL << libxl_ctxt.page_shift) - 1;
> +
> + libxl_ctxt.c.mem_ops.alloc = mem_alloc;
> + libxl_ctxt.c.mem_ops.v2p = virt_to_phys;
> + libxl_ctxt.c.mem_ops.free = acpi_mem_free;
> +
> + rc = init_acpi_config(gc, dom, b_info, &config);
> + if (rc) {
> + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc);
> + return rc;
> + }
> +
> + config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> + config.infop = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> + /* Pages to hold ACPI tables */
> + acpi_pages = libxl__malloc(gc, (NUM_ACPI_PAGES + 1) *
> + libxl_ctxt.page_size);
> +
> + /*
> + * Set up allocator memory.
> + * Start next to acpi_info page to avoid fracturing e820.
> + */
> + libxl_ctxt.alloc_base_paddr = ACPI_INFO_PHYSICAL_ADDRESS +
> + libxl_ctxt.page_size;
> + libxl_ctxt.alloc_base_vaddr = libxl_ctxt.alloc_currp =
> + (unsigned long)acpi_pages;
> + libxl_ctxt.alloc_end = (unsigned long)acpi_pages +
> + (NUM_ACPI_PAGES * libxl_ctxt.page_size);
> +
> + /* Build the tables. */
> + rc = acpi_build_tables(&libxl_ctxt.c, &config);
> + if (rc) {
> + LOG(ERROR, "%s: acpi_build_tables failed with %d",
> + __FUNCTION__, rc);
> + goto out;
> + }
> +
> + /* Calculate how many pages are needed for the tables. */
> + acpi_pages_num =
> + ((libxl_ctxt.alloc_currp - (unsigned long)acpi_pages)
> + >> libxl_ctxt.page_shift) +
> + ((libxl_ctxt.alloc_currp & page_mask) ? 1 : 0);
> +
> + dom->acpi_modules[0].data = (void *)config.rsdp;
> + dom->acpi_modules[0].length = 64;
> + dom->acpi_modules[0].guest_addr_out = RSDP_ADDRESS;
> +
> + dom->acpi_modules[1].data = (void *)config.infop;
> + dom->acpi_modules[1].length = 4096;
> + dom->acpi_modules[1].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS;
> +
> + dom->acpi_modules[2].data = acpi_pages;
> + dom->acpi_modules[2].length = acpi_pages_num << libxl_ctxt.page_shift;
> + dom->acpi_modules[2].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS +
> + libxl_ctxt.page_size;
> +
> +out:
> +
> + return rc;
> +
Extraneous line.
> +}
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libxl/libxl_x86_acpi.h b/tools/libxl/libxl_x86_acpi.h
> new file mode 100644
> index 0000000..3622dd0
> --- /dev/null
> +++ b/tools/libxl/libxl_x86_acpi.h
> @@ -0,0 +1,35 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef LIBXL_X86_ACPI_H
> +#define LIBXL_X86_ACPI_H
> +
> +#include "libxl_internal.h"
> +
> +#define ASSERT(x) assert(x)
> +
> +static inline int test_bit(unsigned int b, const void *p)
> +{
> + return !!(((uint8_t *)p)[b>>3] & (1u<<(b&7)));
> +}
> +
> +#endif /* LIBXL_X_86_ACPI_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 1.8.3.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |