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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 01/27/17 16:40 -0500, Konrad Rzeszutek Wilk wrote:
> 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@xxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  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?
I think the later is better.

> > +};
> > +
> >  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 ?
will move to libacpi.h, so others can use it in future.

> > +
> > +static int check_signature_collision(uint64_t sig)
> bool
will change here and below

> > +{
> > +    int i;
> unsigned int
ditto

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

> > +    }
> > +    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
yes

> > +            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 ?
will move to the header file

> > +}
> > +
> > +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.
yes, will move to the header file.

> > + *
> > + * 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..
If the type is DM_ACPI_BLOB_TYPE_TABLE, the directory name makes
little sense as long as it's different than other names.

If the type is DM_ACPI_BLOB_TYPE_NSDEV, the directory name is the name
of namespace device (see the comment in the patch about
DM_ACPI_BLOB_TYPE_NSDEV below), such as "NVDR" below
   Scope (\_SB) {
       Device (NVDR) {
          ...
       }
   }
I'll add comments about the limitation of the name.

> 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"?
I'm not object to use a string, but isn't an integer easier for
programs to parse? Or you are considering it should also be human
readable?

> > + *
> > + * - "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
OK. As long as ACPI_MAX_SECONDARY_TABLES (i.e. the max value of
nr_tables) is smaller than INT_MAX, the signed -> unsigned type will
not cause trouble to its caller.

> > +                               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.
will do

> > +        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?
Yes.

Thanks,
Haozhong

> > +    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@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel