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

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

Hi Boris,

On 06/07/16 16:50, Boris Ostrovsky wrote:
> On 07/06/2016 07:05 AM, Julien Grall wrote:
>>> +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.

The maximum amount of memory that could be assigned to a domain is fixed 
per-domain. This maximum amount does not take into account the size of 
the ACPI blob. So you may end up to fail because all the memory was 
assigned somewhere else.

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

The device tree for ARM are built in libxl and loaded for libxc. IHMO, 
it would be strange to have a different pattern for ACPI.

Regards,

-- 
Julien Grall

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