Re: [PATCH v1 20/20] libxl/acpi: Build ACPI tables for HVMlite guests

[Julien Grall <julien.grall@arm.com>]

(CC Stefano)

Hi Boris,

On 05/07/16 20:05, Boris Ostrovsky wrote:
> 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);

This file contains arch specific function with are called by the generic 
libxl code.

I don't think this is the right file for an x86 specific function which 
has a generic name. IHMO, this should go in libxl_x86_acpi.h with "x86" 
in the name and with '_hidden' attribute.

>   #endif

[...]

> +static int populate_acpi_pages(struct xc_dom_image *dom,
> +                               xen_pfn_t *extents,
> +                               unsigned int num_pages,
> +                               struct acpi_ctxt *ctxt)
> +{
> +    int rc;
> +    xc_interface *xch = dom->xch;
> +    uint32_t domid = dom->guest_domid;
> +    unsigned long idx, first_high_idx = (1ull << (32 - ctxt->page_shift));
> +
> +    for (; num_pages; num_pages--, extents++) {
> +
> +        if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) == 1)

It looks like this is working because libxl is setting the maximum size 
of the domain with some slack (1MB). You might get in trouble if the 
slack is reduced or used by someone or the ACPI blob is increasing.

> +            continue;
> +
> +        if (dom->highmem_end) {
> +            idx = --dom->highmem_end;
> +            if ( idx == first_high_idx )
> +                dom->highmem_end = 0;
> +        } else
> +            idx = --dom->lowmem_end;
> +
> +        rc = xc_domain_add_to_physmap(xch, domid,
> +                                      XENMAPSPACE_gmfn,
> +                                      idx, *extents);
> +        if (rc)
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +int libxl__dom_load_acpi(libxl__gc *gc,
> +			 libxl_domain_build_info *info,
> +			 struct xc_dom_image *dom)
> +{
> +    struct acpi_config config = {0};
> +    struct acpi_ctxt ctxt;
> +    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, *guest_acpi_pages = NULL;
> +    unsigned long page_mask;
> +
> +    if ((info->type != LIBXL_DOMAIN_TYPE_HVM) ||
> +        (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE))
> +        return 0;
> +
> +    ctxt.page_size = XC_DOM_PAGE_SIZE(dom);
> +    ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom);
> +    page_mask = (1UL << ctxt.page_shift) - 1;
> +
> +    ctxt.mem_ops.alloc = mem_alloc;
> +    ctxt.mem_ops.v2p = virt_to_phys;
> +
> +    rc = init_acpi_config(gc, dom, info, &config);
> +    if (rc) {
> +        LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc);
> +        return rc;
> +    }
> +
> +    /* Map page that will hold RSDP */
> +    extent = RSDP_ADDRESS >> ctxt.page_shift;
> +    rc = populate_acpi_pages(dom, &extent, 1, &ctxt);
> +    if (rc) {
> +        LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d",
> +            __FUNCTION__, rc);
> +        goto out;
> +    }
> +    config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid,
> +                                                      ctxt.page_size,
> +                                                      PROT_READ | PROT_WRITE,
> +                                                      RSDP_ADDRESS >> ctxt.page_shift);
> +    if (!config.rsdp) {
> +        LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__);
> +        rc = -1;
> +        goto out;
> +    }
> +
> +    /* Map acpi_info page */
> +    extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift;
> +    rc = populate_acpi_pages(dom, &extent, 1, &ctxt);
> +    if (rc) {
> +        LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed with %d",
> +            __FUNCTION__, rc);
> +        goto out;
> +    }
> +
> +    config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid,
> +                                                        ctxt.page_size,
> +                                                        PROT_READ | PROT_WRITE,
> +                                                        ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift);

Loading the ACPI blob on ARM will be very similar except for the base 
address. So it would be nice to share some code with it.

However, as mentioned in the ACPI thread [1], all the blobs are 
generally loaded by libxc and not libxl. This is more true on ARM 
because the guest address space is controlled by libxc (the position of 
all the blob are decided by it).

Regards,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00040.html

-- 
Julien Grall

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