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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/06/2016 09:29 AM, Jan Beulich wrote:
>>>> 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.

These two are not needed.

>
>> +#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?

NUM_ACPI_PAGES is private to this file.

ACPI_PHYSICAL_ADDRESS (RSDP pointer) needs to be between 0xe0000 and 0xfffff, I picked this number because that's where most systems that I have appear to have it. (And by "most" I mean the two that I checked ;-))

RESERVED_MEMORY_DYNAMIC_START is one page after DSDT's SystemMemory (aka ACPI_INFO_PHYSICAL_ADDRESS). But then it looks like PVHv2 doesn't need SystemMemory so it can be anywhere (and e820 should presumably be aware of this, which it is not right now)


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

DSDT is required to be present by the spec. And ACPICA gets upset if it
doesn't see it.

> +    /* 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.

Yes, I am off-by-one here. The first page (ACPI_INFO_PHYSICAL_ADDRESS)
is managed separately.

>
>> --- 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?

Right, I should check. Come think of it, we should probably also check
"apic=" option when building MADT in libacpi.


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

acpi_build_tables() is called with virtual (i.e. 64-bit) address from libxc.

-boris


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