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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 07/06/2016 07:05 AM, Julien Grall wrote:
> (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.

Right. I used to call this routine from libxl_dom.c. I moved the call
site to libxl_x86.c (as you suggested earlier) but forgot to move the
declaration.

>
>>   #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.


I saw your conversation about slack with Stefano and I am not sure I
understood what it was about. If this was about padding guest's memory
to be able to put there some additional data (such ACPI tables) then
this is not my intention here: if I can't populate (because it is
already populated, I guess) then I try to move memory around with code
below. That's what mem_hole_populate_ram() does as well.


>
>> +            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).

The difference is that I not only load the tables here but also build
them. Which may or may not be the right thing to do in libxc.

I suppose I can defer loading (and then keep pointer to tables in
acpitable_blob) but the then I need to keep RSDP descriptor somewhere
else (it is not part of the blob since it lives in lower MB of the guest).


-boris

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



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