Re: [PATCH RFC 18/20] libxc/acpi: Build ACPI tables for HVMlite guests

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.04.16 at 03:25, <boris.ostrovsky@oracle.com> 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@lists.xen.org
http://lists.xen.org/xen-devel