Re: [RFC XEN PATCH 11/16] tools/libacpi: load ACPI built by the device model

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Mon, Oct 10, 2016 at 08:32:30AM +0800, Haozhong Zhang wrote:
> ACPI tables built by the device model, whose signatures do not
> conflict with tables built by Xen (except SSDT), are loaded after ACPI
> tables built by Xen.
> 
> ACPI namespace devices built by the device model, whose names do not
> conflict with devices built by Xen, are assembled and placed in SSDTs
> after ACPI tables built by Xen.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/firmware/hvmloader/util.c |  12 +++
>  tools/libacpi/acpi2_0.h         |   2 +
>  tools/libacpi/build.c           | 216 ++++++++++++++++++++++++++++++++++++++++
>  tools/libacpi/libacpi.h         |   5 +
>  4 files changed, 235 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index dba954a..e6530cd 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -998,6 +998,18 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>      if ( !strncmp(xenstore_read("platform/acpi_s4", "1"), "1", 1)  )
>          config->table_flags |= ACPI_HAS_SSDT_S4;
>  
> +    s = xenstore_read(HVM_XS_DM_ACPI_ADDRESS, NULL);
> +    if ( s )
> +    {
> +        config->dm.addr = strtoll(s, NULL, 0);
> +
> +        s = xenstore_read(HVM_XS_DM_ACPI_LENGTH, NULL);
> +        if ( s )
> +            config->dm.length = strtoll(s, NULL, 0);
> +        else
> +            config->dm.addr = 0;
> +    }
> +
>      config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET);
>  
>      config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 775eb7a..7414470 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -430,6 +430,7 @@ struct acpi_20_slit {
>  #define ACPI_2_0_WAET_SIGNATURE ASCII32('W','A','E','T')
>  #define ACPI_2_0_SRAT_SIGNATURE ASCII32('S','R','A','T')
>  #define ACPI_2_0_SLIT_SIGNATURE ASCII32('S','L','I','T')
> +#define ACPI_2_0_SSDT_SIGNATURE ASCII32('S','S','D','T')
>  
>  /*
>   * Table revision numbers.
> @@ -445,6 +446,7 @@ struct acpi_20_slit {
>  #define ACPI_1_0_FADT_REVISION 0x01
>  #define ACPI_2_0_SRAT_REVISION 0x01
>  #define ACPI_2_0_SLIT_REVISION 0x01
> +#define ACPI_2_0_SSDT_REVISION 0x02
>  
>  #pragma pack ()
>  
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index 47dae01..829a365 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -20,6 +20,7 @@
>  #include "ssdt_s4.h"
>  #include "ssdt_tpm.h"
>  #include "ssdt_pm.h"
> +#include "aml_build.h"
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/params.h>
> @@ -55,6 +56,34 @@ struct acpi_info {
>      uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
>  };
>  
> +#define DM_ACPI_BLOB_TYPE_TABLE 0 /* ACPI table */
> +#define DM_ACPI_BLOB_TYPE_NSDEV 1 /* AML definition of an ACPI namespace device */
> +
> +/* ACPI tables of following signatures should not appear in DM ACPI */

It would be good to have some form of build check to check against
this list..
> +static const uint64_t dm_acpi_signature_blacklist[] = {
> +    ACPI_2_0_RSDP_SIGNATURE,
> +    ACPI_2_0_FACS_SIGNATURE,
> +    ACPI_2_0_FADT_SIGNATURE,
> +    ACPI_2_0_MADT_SIGNATURE,
> +    ACPI_2_0_RSDT_SIGNATURE,
> +    ACPI_2_0_XSDT_SIGNATURE,
> +    ACPI_2_0_TCPA_SIGNATURE,
> +    ACPI_2_0_HPET_SIGNATURE,
> +    ACPI_2_0_WAET_SIGNATURE,
> +    ACPI_2_0_SRAT_SIGNATURE,
> +    ACPI_2_0_SLIT_SIGNATURE,
> +};
> +
> +/* ACPI namespace devices of following names should not appear in DM ACPI */
> +static const char *dm_acpi_devname_blacklist[] = {
> +    "MEM0",
> +    "PCI0",
> +    "AC",
> +    "BAT0",
> +    "BAT1",
> +    "TPM",

.. and this one.

But I am not even sure how one would do that?

Perhaps add a big warning:

"Make sure to add your table name if you this code (libacpi) is
constructing it. "?

Or maybe have some 'register_acpi_table' function which will expand
this blacklist?

> +};
> +
>  static void set_checksum(
>      void *table, uint32_t checksum_offset, uint32_t length)
>  {
> @@ -339,6 +368,190 @@ static int construct_passthrough_tables(struct acpi_ctxt *ctxt,
>      return nr_added;
>  }
>  
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))

That may want to go libacpi.h ?
> +
> +static int check_signature_collision(uint64_t sig)

bool
> +{
> +    int i;

unsigned int

> +    for ( i = 0; i < ARRAY_SIZE(dm_acpi_signature_blacklist); i++ )
> +    {
> +        if ( sig == dm_acpi_signature_blacklist[i] )
> +            return 1;

return true

> +    }
> +    return 0;
> +}
> +
> +static int check_devname_collision(const char *name)

bool
> +{
> +    int i;

unsigned int

> +    for ( i = 0; i < ARRAY_SIZE(dm_acpi_devname_blacklist); i++ )
> +    {
> +        if ( !strncmp(name, dm_acpi_devname_blacklist[i], 4) )

That 4 could be a #define
> +            return 1;
> +    }
> +    return 0;
> +}
> +
> +static const char *xs_read_dm_acpi_blob_key(struct acpi_ctxt *ctxt,
> +                                            const char *name, const char *key)
> +{
> +#define DM_ACPI_BLOB_PATH_MAX_LENGTH 30
> +    char path[DM_ACPI_BLOB_PATH_MAX_LENGTH];
> +    snprintf(path, DM_ACPI_BLOB_PATH_MAX_LENGTH, HVM_XS_DM_ACPI_ROOT"/%s/%s",
> +             name, key);
> +    return ctxt->xs_ops.read(ctxt, path);

#undef DM_APCI_BLOB... but perhaps that should go in
xen/include/public/hvm/hvm_xs_strings.h ?

> +}
> +
> +static int construct_dm_table(struct acpi_ctxt *ctxt,

bool
> +                              unsigned long *table_ptrs, int nr_tables,

unsigned int nr_tables
> +                              const void *blob, uint32_t length)
> +{
> +    const struct acpi_header *header = blob;
> +    uint8_t *buffer;
> +
> +    if ( check_signature_collision(header->signature) )
> +        return 0;
> +
> +    if ( header->length > length || header->length == 0 )
> +        return 0;
> +
> +    buffer = ctxt->mem_ops.alloc(ctxt, header->length, 16);
> +    if ( !buffer )
> +        return 0;
> +    memcpy(buffer, header, header->length);
> +
> +    /* some device models (e.g. QEMU) does not set checksum */
> +    set_checksum(buffer, offsetof(struct acpi_header, checksum),
> +                 header->length);
> +
> +    table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, buffer);
> +
> +    return 1;
> +}
> +
> +static int construct_dm_nsdev(struct acpi_ctxt *ctxt,

bool
> +                              unsigned long *table_ptrs, int nr_tables,

unsigned int
> +                              const char *dev_name,
> +                              const void *blob, uint32_t blob_length)
> +{
> +    struct acpi_header ssdt, *header;
> +    uint8_t *buffer;
> +
> +    if ( check_devname_collision(dev_name) )
> +        return 0;
> +
> +    /* built ACPI namespace device from [name, blob] */
> +    buffer = aml_build_begin(ctxt);
> +    aml_prepend_blob(buffer, blob, blob_length);
> +    aml_prepend_device(buffer, dev_name);
> +    aml_prepend_scope(buffer, "\\_SB");
> +
> +    /* build SSDT header */
> +    ssdt.signature = ACPI_2_0_SSDT_SIGNATURE;
> +    ssdt.revision = ACPI_2_0_SSDT_REVISION;
> +    fixed_strcpy(ssdt.oem_id, ACPI_OEM_ID);
> +    fixed_strcpy(ssdt.oem_table_id, ACPI_OEM_TABLE_ID);
> +    ssdt.oem_revision = ACPI_OEM_REVISION;
> +    ssdt.creator_id = ACPI_CREATOR_ID;
> +    ssdt.creator_revision = ACPI_CREATOR_REVISION;
> +
> +    /* prepend SSDT header to ACPI namespace device */
> +    aml_prepend_blob(buffer, &ssdt, sizeof(ssdt));
> +    header = (struct acpi_header *) buffer;
> +    header->length = aml_build_end();
> +
> +    /* calculate checksum of SSDT */
> +    set_checksum(header, offsetof(struct acpi_header, checksum),
> +                 header->length);
> +
> +    table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, buffer);
> +
> +    return 1;

return true.
> +}
> +
> +/*
> + * All ACPI stuffs built by the device model are placed in the guest
> + * buffer whose address and size are specified by config->dm.{addr, length},
> + * or XenStore keys HVM_XS_DM_ACPI_{ADDRESS, LENGTH}.

This should be also provided in
xen/include/public/hvm/hvm_xs_strings.h

Especially as you are in effect adding new keys and attributes to it.

> + *
> + * The data layout within the buffer is further specified by XenStore
> + * directories under HVM_XS_DM_ACPI_ROOT. Each directory specifies a

Is each directory the name of the DSDT object? In which case you
want to say a bit about the directory name and the limit (only four
characters long), don't use the ones that are built-in, etc..

But it looks like it can be anything. You extract the name from
the blob.

But we should still say what the directory names ought to be.

> + * data blob and contains following XenStore keys:
> + *
> + * - "type":
> + *   * DM_ACPI_BLOB_TYPE_TABLE
> + *     The data blob specified by this directory is an ACPI table.
> + *   * DM_ACPI_BLOB_TYPE_NSDEV
> + *     The data blob specified by this directory is an ACPI namespace device.
> + *     Its name is specified by the directory name, while the AML code of the
> + *     body of the AML device structure is in the data blob.

Could those be strings on XenStore? Strings are nice. "table" or
"nsdev"?
> + *
> + * - "length": the number of bytes in this data blob.
> + *
> + * - "offset": the offset in bytes of this data blob from the beginning of buffer
> + */
> +static int construct_dm_tables(struct acpi_ctxt *ctxt,

static unsigned int
> +                               unsigned long *table_ptrs,
> +                               int nr_tables,

unsigned int nr_tables
> +                               struct acpi_config *config)
> +{
> +    const char *s;
> +    char **dir;
> +    uint8_t type;
> +    void *blob;
> +    unsigned int num, length, offset, i;
> +    int nr_added = 0;

unsigned int
> +
> +    if ( !config->dm.addr )
> +        return 0;
> +
> +    dir = ctxt->xs_ops.directory(ctxt, HVM_XS_DM_ACPI_ROOT, &num);
> +    if ( !dir || !num )
> +        return 0;
> +
> +    if ( num > ACPI_MAX_SECONDARY_TABLES - nr_tables )
> +        return 0;
> +
> +    for ( i = 0; i < num; i++, dir++ )
> +    {

You probably want to check that *dir is not NULL. Just in case.

> +        s = xs_read_dm_acpi_blob_key(ctxt, *dir, "type");
> +        if ( !s )
> +            continue;
> +        type = (uint8_t)strtoll(s, NULL, 0);
> +
> +        s = xs_read_dm_acpi_blob_key(ctxt, *dir, "length");
> +        if ( !s )
> +            continue;
> +        length = (uint32_t)strtoll(s, NULL, 0);
> +
> +        s = xs_read_dm_acpi_blob_key(ctxt, *dir, "offset");
> +        if ( !s )
> +            continue;
> +        offset = (uint32_t)strtoll(s, NULL, 0);
> +
> +        blob = ctxt->mem_ops.p2v(ctxt, config->dm.addr + offset);
> +
> +        switch ( type )
> +        {
> +        case DM_ACPI_BLOB_TYPE_TABLE:
> +            nr_added += construct_dm_table(ctxt,
> +                                           table_ptrs, nr_tables + nr_added,
> +                                           blob, length);
> +            break;
> +        case DM_ACPI_BLOB_TYPE_NSDEV:
> +            nr_added += construct_dm_nsdev(ctxt,
> +                                           table_ptrs, nr_tables + nr_added,
> +                                           *dir, blob, length);
> +            break;
> +        default:
> +            /* skip blobs of unknown types */
> +            continue;
> +        }
> +    }
> +
> +    return nr_added;
> +}
> +
>  static int construct_secondary_tables(struct acpi_ctxt *ctxt,
>                                        unsigned long *table_ptrs,
>                                        struct acpi_config *config,
> @@ -461,6 +674,9 @@ static int construct_secondary_tables(struct acpi_ctxt *ctxt,
>      nr_tables += construct_passthrough_tables(ctxt, table_ptrs,
>                                                nr_tables, config);
>  
> +    /* Load any additional tables passed from device model (e.g. QEMU) */

Perhaps an period at the end of the sentence?

> +    nr_tables += construct_dm_tables(ctxt, table_ptrs, nr_tables, config);
> +
>      table_ptrs[nr_tables] = 0;
>      return nr_tables;
>  }
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index 12cafd8..684502d 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -82,6 +82,11 @@ struct acpi_config {
>          uint32_t length;
>      } pt;
>  
> +    struct {
> +        uint32_t addr;
> +        uint32_t length;
> +    } dm;
> +
>      struct acpi_numa numa;
>      const struct hvm_info_table *hvminfo;
>  
> -- 
> 2.10.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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