Re: [Xen-devel] [RFC PATCH 10/12] libacpi: build ACPI MCFG table if requested

[Alexey G <x1917x@xxxxxxxxx>]

On Tue, 29 May 2018 08:36:49 -0600
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

>>>> On 12.03.18 at 19:33, <x1917x@xxxxxxxxx> wrote:  
>> --- a/tools/libacpi/acpi2_0.h
>> +++ b/tools/libacpi/acpi2_0.h
>> @@ -422,6 +422,25 @@ struct acpi_20_slit {
>>  };
>>  
>>  /*
>> + * PCI Express Memory Mapped Configuration Description Table
>> + */
>> +struct mcfg_range_entry {
>> +    uint64_t base_address;
>> +    uint16_t pci_segment;
>> +    uint8_t  start_pci_bus_num;
>> +    uint8_t  end_pci_bus_num;
>> +    uint32_t reserved;
>> +};
>> +
>> +struct acpi_mcfg {
>> +    struct acpi_header header;
>> +    uint8_t reserved[8];
>> +    struct mcfg_range_entry entries[1];
>> +};
>> +
>> +#define MCFG_SIZE_TO_NUM_BUSES(size)  ((size) >> 20)  
>
>In a response to a comment from Roger you suggested to move this to pci_regs.h.
>I don't see why it would belong there. I think if ACPI spells out such a 
>formula
>somewhere, it's fine to liver here. Otherwise, since you need it in a single 
>file only,
>please put it into the .c file.

Agree, it is currently used in one place only, no need for .h

>> --- a/tools/libacpi/build.c
>> +++ b/tools/libacpi/build.c
>> @@ -303,6 +303,37 @@ static struct acpi_20_slit *construct_slit(struct 
>> acpi_ctxt *ctxt,
>>      return slit;
>>  }
>>  
>> +static struct acpi_mcfg *construct_mcfg(struct acpi_ctxt *ctxt,
>> +                                        const struct acpi_config *config)
>> +{
>> +    struct acpi_mcfg *mcfg;
>> +
>> +    /* Warning: this code expects that we have only one PCI segment */
>> +    mcfg = ctxt->mem_ops.alloc(ctxt, sizeof(*mcfg), 16);
>> +    if (!mcfg)
>> +        return NULL;
>> +
>> +    memset(mcfg, 0, sizeof(*mcfg));
>> +    mcfg->header.signature    = ACPI_MCFG_SIGNATURE;
>> +    mcfg->header.revision     = ACPI_1_0_MCFG_REVISION;
>> +    fixed_strcpy(mcfg->header.oem_id, ACPI_OEM_ID);
>> +    fixed_strcpy(mcfg->header.oem_table_id, ACPI_OEM_TABLE_ID);
>> +    mcfg->header.oem_revision = ACPI_OEM_REVISION;
>> +    mcfg->header.creator_id   = ACPI_CREATOR_ID;
>> +    mcfg->header.creator_revision = ACPI_CREATOR_REVISION;
>> +    mcfg->header.length = sizeof(*mcfg);
>> +
>> +    mcfg->entries[0].base_address = config->mmconfig_addr;
>> +    mcfg->entries[0].pci_segment = 0;
>> +    mcfg->entries[0].start_pci_bus_num = 0;
>> +    mcfg->entries[0].end_pci_bus_num =
>> +        MCFG_SIZE_TO_NUM_BUSES(config->mmconfig_len) - 1;
>> +
>> +    set_checksum(mcfg, offsetof(struct acpi_header, checksum), 
>> sizeof(*mcfg));  
>
>Despite the numerous pre-existing examples this isn't really correct.
>What you mean is something like
>
>    set_checksum(mcfg, offsetof(typeof(*mcfg), header.checksum), 
> sizeof(*mcfg));

Yes, all those set_checksum calls rely on the fact the acpi_header
structure will always be the first field. It will be, but the code is
technically wrong anyway.

I'll update all such set_checksum(...checksum)...) instances in the
file for the next version, this is a trivial change.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel