|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 18/20] libxc/acpi: Build ACPI tables for HVMlite guests
>>> On 06.04.16 at 03:25, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/tools/libxc/xc_acpi.c
> @@ -0,0 +1,268 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <inttypes.h>
> +#include <assert.h>
> +
> +#include <xen/xen.h>
> +#include <xen/foreign/x86_32.h>
> +#include <xen/foreign/x86_64.h>
Why are these two needed here? The code here shouldn't care
about the mode the guest may later want to run in.
> +#include <xen/hvm/hvm_info_table.h>
> +#include <xen/io/protocols.h>
> +
> +#include "xg_private.h"
> +#include "xc_dom.h"
> +#include "xenctrl.h"
> +
> +#include "acpi2_0.h"
> +
> +#define RESERVED_MEMORY_DYNAMIC_START 0xFC001000
> +#define ACPI_PHYSICAL_ADDRESS 0x000EA020
> +
> +/* Initial allocation for ACPI tables */
> +#define NUM_ACPI_PAGES 16
With which other definitions do these three need to remain in sync?
> +#define PFN(paddr) ((paddr) >> PAGE_SHIFT)
> +
> +extern unsigned char dsdt_anycpu[], dsdt_15cpu[], dsdt_empty[];
const
> +extern int dsdt_anycpu_len, dsdt_15cpu_len, dsdt_empty_len;
And even these may want to be const (and certainly unsigned).
> +static int init_acpi_config(struct xc_dom_image *dom,
> + struct acpi_config *config)
> +{
> + xc_interface *xch = dom->xch;
> + uint32_t domid = dom->guest_domid;
> + xc_dominfo_t info;
> + int i, rc;
> +
> + memset(config, 0, sizeof(*config));
> +
> + config->dsdt_anycpu = config->dsdt_15cpu = dsdt_empty;
> + config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_empty_len;
What good does an empty DSDT do? (Perhaps this question is a
result of there not being any description of this change.)
> + rc = xc_domain_getinfo(xch, domid, 1, &info);
> + if ( rc < 0 )
> + {
> + DOMPRINTF("%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc);
> + return rc;
> + }
> +
> + config->apic_mode = 1;
> +
> + if ( dom->nr_vnodes )
> + {
> + struct acpi_numa *numa = &config->numa;
> +
> + numa->vmemrange = calloc(dom->nr_vmemranges,
> + sizeof(*numa->vmemrange));
> + numa->vdistance = calloc(dom->nr_vnodes,
> + sizeof(*numa->vdistance));
> + numa->vcpu_to_vnode = calloc(config->nr_vcpus,
> + sizeof(*numa->vcpu_to_vnode));
> + if ( !numa->vmemrange || !numa->vdistance || !numa->vcpu_to_vnode )
> + {
> + DOMPRINTF("%s: Out of memory", __FUNCTION__);
> + free(numa->vmemrange);
> + free(numa->vdistance);
> + free(numa->vcpu_to_vnode);
> + return -ENOMEM;
> + }
> +
> + rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
> + &numa->nr_vmemranges,
> + &config->nr_vcpus, numa->vmemrange,
> + numa->vdistance, numa->vcpu_to_vnode);
> +
> + if ( rc )
> + {
> + DOMPRINTF("%s: xc_domain_getvnuma failed (rc=%d)", __FUNCTION__,
> rc);
> + return rc;
> + }
> + }
> + else
> + config->nr_vcpus = info.max_vcpu_id + 1;
> +
> + config->vcpu_online = calloc((HVM_MAX_VCPUS + 7) / 8,
> + sizeof(*config->vcpu_online));
> + if ( config->vcpu_online == NULL )
> + {
> + DOMPRINTF("%s: Can't allocate vcpu_online", __FUNCTION__);
> + return -ENOMEM;
> + }
> +
> + for (i=0; i<config->nr_vcpus; i++)
You seem to be mixing styles here.
> +int xc_dom_build_acpi(struct xc_dom_image *dom)
> +{
> + struct acpi_config config;
> + uint32_t domid = dom->guest_domid;
> + xc_interface *xch = dom->xch;
> + int rc, i, acpi_pages_num;
> + xen_pfn_t extent, *extents;
> + void *acpi_pages, *acpi_physical;
> + void *guest_info_page, *guest_acpi_pages;
> +
> + rc = init_acpi_config(dom, &config);
> + if ( rc )
> + {
> + DOMPRINTF("%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc);
> + return rc;
> + }
> +
> + /*
> + * Pages to hold ACPI tables and one page for acpi_info, which
> + * will be the first one in this region.
> + */
> + acpi_pages = xc_memalign(xch, PAGE_SIZE, NUM_ACPI_PAGES * PAGE_SIZE);
> + if ( !acpi_pages )
> + {
> + DOMPRINTF("%s: Can't allocate acpi pages", __FUNCTION__);
> + rc = -1;
> + goto out;
> + }
> +
> + config.acpi_info_page = acpi_pages;
> +
> + /* Set up allocator memory */
> + base_addr = alloc_up = (unsigned long)acpi_pages + PAGE_SIZE;
> + alloc_down = (unsigned long)acpi_pages + (NUM_ACPI_PAGES * PAGE_SIZE);
> +
> + /* Map page that will hold RSDP */
> + extent = PFN(ACPI_PHYSICAL_ADDRESS);
> + rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &extent);
> + if ( rc )
> + {
> + DOMPRINTF("%s: xc_domain_populate_physmap failed with %d",
> + __FUNCTION__, rc);
> + goto out;
> + }
> + acpi_physical = xc_map_foreign_range(xch, domid, PAGE_SIZE,
> + PROT_READ | PROT_WRITE,
> + PFN(ACPI_PHYSICAL_ADDRESS));
> + if ( !acpi_physical )
> + {
> + DOMPRINTF("%s: Can't map acpi_physical", __FUNCTION__);
> + rc = -1;
> + goto out;
> + }
> +
> + /* Build the tables */
> + acpi_build_tables(&config, (unsigned long)acpi_physical);
> +
> + /* Copy acpi_info page into guest's memory */
> + extent = PFN(ACPI_INFO_PHYSICAL_ADDRESS);
> + rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &extent);
> + if ( rc )
> + {
> + DOMPRINTF("%s: xc_domain_populate_physmap failed with %d\n",
> + __FUNCTION__, rc);
> + goto out;
> + }
> + guest_info_page = xc_map_foreign_range(xch, domid, PAGE_SIZE,
> + PROT_READ | PROT_WRITE,
> + PFN(ACPI_INFO_PHYSICAL_ADDRESS));
> + if ( !guest_info_page )
> + {
> + DOMPRINTF("%s: Can't map acpi_info_page", __FUNCTION__);
> + rc = -1;
> + goto out;
> + }
> + memcpy(guest_info_page, acpi_pages, PAGE_SIZE);
> +
> + /* Copy ACPI tables into guest's memory */
> + acpi_pages_num = ((alloc_up - (unsigned long)acpi_pages +
> + (PAGE_SIZE - 1)) >> PAGE_SHIFT) - 1;
This looks like there are acpi_pages_num pages starting at
acpi_pages, yet ...
> + extents = malloc(acpi_pages_num * sizeof(*extents));
> + if ( !extents )
> + {
> + DOMPRINTF("%s: Can't allocate extents array", __FUNCTION__);
> + rc = -ENOMEM;
> + goto out;
> + }
> + for (i = 0; i < acpi_pages_num; i++)
> + extents[i] = PFN(RESERVED_MEMORY_DYNAMIC_START) + i;
> + rc = xc_domain_populate_physmap_exact(xch, domid, acpi_pages_num,
> + 0, 0, extents);
> + if ( rc )
> + {
> + DOMPRINTF("%s: xc_domain_populate_physmap failed with %d",
> + __FUNCTION__, rc);
> + goto out;
> + }
> + guest_acpi_pages = xc_map_foreign_range(xch, domid,
> + PAGE_SIZE * acpi_pages_num,
> + PROT_READ | PROT_WRITE,
> +
> PFN(RESERVED_MEMORY_DYNAMIC_START));
> + if ( !guest_acpi_pages )
> + {
> + DOMPRINTF("%s Can't map guest_acpi_pages", __FUNCTION__);
> + rc = -1;
> + goto out;
> + }
> +
> + memcpy(guest_acpi_pages, acpi_pages + PAGE_SIZE,
> + acpi_pages_num * PAGE_SIZE);
... this looks like there are acpi_pages_num pages starting at
acpi_pages + PAGE_SIZE.
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -643,6 +643,13 @@ static int alloc_magic_pages_hvm(struct xc_dom_image
> *dom)
> DOMPRINTF("Unable to reserve memory for the start info");
> goto out;
> }
> +
> + rc = xc_dom_build_acpi(dom);
> + if ( rc != 0 )
> + {
> + DOMPRINTF("Unable to build ACPI tables");
> + goto out;
> + }
Iirc there is an "acpi=" guest config setting, yet neither here nor
down the call tree I have been able to find a respective check. Is
that option not relevant anymore? Do we really want to always
have those tables built?
> --- 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
> + /*
> + * HVM guests without device model will want
> + * to have LAPIC emulation.
> + */
> + xc_config->emulation_flags = XEN_X86_EMU_LAPIC;
> + } else
> xc_config->emulation_flags = 0;
> - }
I'm not maintainer of this code, but I think this would read better if
you used an if/else if/else sequence instead of nested if.
> --- a/xen/common/libacpi/build.c
> +++ b/xen/common/libacpi/build.c
> @@ -480,7 +480,7 @@ static int new_vm_gid(struct acpi_config *config)
> return 1;
> }
>
> -void acpi_build_tables(struct acpi_config *config, unsigned int physical)
> +void acpi_build_tables(struct acpi_config *config, unsigned long physical)
I'm having some difficulty seeing how this change belongs here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |