Re: [Xen-devel] [PATCH v3 4/5] tools/libacpi: update FADT layout to support version 5

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, Dec 07, 2016 at 09:04:14AM -0700, Jan Beulich wrote:
> >>> On 05.12.16 at 16:04, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/util.c
> > +++ b/tools/firmware/hvmloader/util.c
> > @@ -949,7 +949,7 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> > *config,
> >          config->table_flags |= ACPI_HAS_SSDT_S4;
> >  
> >      config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | 
> > ACPI_HAS_WAET |
> > -                            ACPI_HAS_VGA | ACPI_HAS_8042);
> > +                            ACPI_HAS_VGA | ACPI_HAS_8042 | ACPI_FADT_4);
> 
> It shouldn't be an explicit FADT version that gets requested, but
> a specific ACPI version (and I don't think a boolean flag is suitable
> for this).

Hm, I'm not opposed to this, but maybe then there are other tables to be bumped 
if we really want to request a specific ACPI version?
 
> > --- a/tools/libacpi/acpi2_0.h
> > +++ b/tools/libacpi/acpi2_0.h
> > @@ -169,7 +169,7 @@ struct acpi_10_fadt {
> >  /*
> >   * Fixed ACPI Description Table Structure (FADT).
> >   */
> > -struct acpi_20_fadt {
> > +struct acpi_50_fadt {
> 
> I think it would be better to drop the version number from this
> name and ...
> 
> > @@ -222,6 +222,8 @@ struct acpi_20_fadt {
> >      struct acpi_20_generic_address x_pm_tmr_blk;
> >      struct acpi_20_generic_address x_gpe0_blk;
> >      struct acpi_20_generic_address x_gpe1_blk;
> > +    struct acpi_20_generic_address sleep_control;
> > +    struct acpi_20_generic_address sleep_status;
> >  };
> 
> ... add version comments to the new fields.

Ack.

> > --- a/tools/libacpi/build.c
> > +++ b/tools/libacpi/build.c
> > @@ -503,12 +503,13 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct 
> > acpi_config *config)
> >      struct acpi_20_rsdp *rsdp;
> >      struct acpi_20_rsdt *rsdt;
> >      struct acpi_20_xsdt *xsdt;
> > -    struct acpi_20_fadt *fadt;
> > +    struct acpi_50_fadt *fadt;
> >      struct acpi_10_fadt *fadt_10;
> >      struct acpi_20_facs *facs;
> >      unsigned char       *dsdt;
> >      unsigned long        secondary_tables[ACPI_MAX_SECONDARY_TABLES];
> >      int                  nr_secondaries, i;
> > +    unsigned long        fadt_size;
> 
> unsigned int would suffice here, wouldn't it? Otherwise it should be
> size_t.

Yes, unsigned int should be fine.

> > --- a/tools/libacpi/static_tables.c
> > +++ b/tools/libacpi/static_tables.c
> > @@ -38,11 +38,11 @@ struct acpi_20_facs Facs = {
> >  #define ACPI_PM_TMR_BLK_BIT_WIDTH           0x20
> >  #define ACPI_PM_TMR_BLK_BIT_OFFSET          0x00
> >  
> > -struct acpi_20_fadt Fadt = {
> > +struct acpi_50_fadt Fadt = {
> >      .header = {
> > -        .signature    = ACPI_2_0_FADT_SIGNATURE,
> > -        .length       = sizeof(struct acpi_20_fadt),
> > -        .revision     = ACPI_2_0_FADT_REVISION,
> > +        .signature    = ACPI_FADT_SIGNATURE,
> > +        .length       = sizeof(struct acpi_50_fadt),
> > +        .revision     = ACPI_5_0_FADT_REVISION,
> 
> Let's please not pre-initialize either value, but set both fields uniformly
> at runtime.

Ack.

Roger.

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