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

[Alexey G <x1917x@gmail.com>]

On Mon, 19 Mar 2018 17:33:34 +0000
Roger Pau Monné <roger.pau@citrix.com> wrote:

>On Tue, Mar 13, 2018 at 04:33:55AM +1000, Alexey Gerasimenko wrote:
>> This adds construct_mcfg() function to libacpi which allows to build
>> MCFG table for a given mmconfig_addr/mmconfig_len pair if the
>> ACPI_HAS_MCFG flag was specified in acpi_config struct.
>> 
>> The maximum bus number is calculated from mmconfig_len using
>> MCFG_SIZE_TO_NUM_BUSES macro (1MByte of MMIO space per bus).
>> 
>> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
>> ---
>>  tools/libacpi/acpi2_0.h | 21 +++++++++++++++++++++
>>  tools/libacpi/build.c   | 42
>> ++++++++++++++++++++++++++++++++++++++++++ tools/libacpi/libacpi.h
>> |  4 ++++ 3 files changed, 67 insertions(+)
>> 
>> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
>> index 2619ba32db..209ad1acd3 100644
>> --- 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];
>> +};  
>
>I would define this as:
>
>struct acpi_10_mcfg {
>    struct acpi_header header;
>    uint8_t reserved[8];
>    struct acpi_10_mcfg_entry {
>        uint64_t base_address;
>        uint16_t pci_segment;
>        uint8_t  start_pci_bus;
>        uint8_t  end_pci_bus;
>        uint32_t reserved;
>    } entries[1];
>};

Hmm, a choice of preference, but OK, will move it inside.

>> +
>> +#define MCFG_SIZE_TO_NUM_BUSES(size)  ((size) >> 20)  
>
>I'm not sure the following macro belongs here. This is not directly
>related to ACPI.

Yeah, pci_regs.h might be better I think.

>> +
>> +/*
>>   * Table Signatures.
>>   */
>>  #define ACPI_2_0_RSDP_SIGNATURE ASCII64('R','S','D','
>> ','P','T','R',' ') @@ -435,6 +454,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_MCFG_SIGNATURE     ASCII32('M','C','F','G')
>>  
>>  /*
>>   * Table revision numbers.
>> @@ -449,6 +469,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_1_0_MCFG_REVISION 0x01
>>  
>>  #pragma pack ()
>>  
>> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
>> index f9881c9604..5daf1fc5b8 100644
>> --- 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)  
>
>Coding style.

OK

>> +        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);  
>
>As said before, if you want to align things, please do it for the
>whole block.

Agree, will reorder lines.

>> +
>> +    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;  
>
>Why not pass the start_bus and end_bus values in acpi_config at least?

start_pci_bus_num will be always 0.

It will be kinda ugly to pass config->mmconfig_addr along with
config->end_pci_bus_num, baseaddr+size combo looks nicer I think.

>> +
>> +    set_checksum(mcfg, offsetof(struct acpi_header, checksum),
>> sizeof(*mcfg)); +
>> +    return mcfg;;  
>
>Double ;;

Oops, missed this one.

>> +}
>> +
>>  static int construct_passthrough_tables(struct acpi_ctxt *ctxt,
>>                                          unsigned long *table_ptrs,
>>                                          int nr_tables,
>> @@ -350,6 +381,7 @@ static int construct_secondary_tables(struct
>> acpi_ctxt *ctxt, struct acpi_20_hpet *hpet;
>>      struct acpi_20_waet *waet;
>>      struct acpi_20_tcpa *tcpa;
>> +    struct acpi_mcfg *mcfg;
>>      unsigned char *ssdt;
>>      static const uint16_t tis_signature[] = {0x0001, 0x0001,
>> 0x0001}; void *lasa;
>> @@ -417,6 +449,16 @@ static int construct_secondary_tables(struct
>> acpi_ctxt *ctxt, printf("CONV disabled\n");
>>      }
>>  
>> +    /* MCFG */
>> +    if ( config->table_flags & ACPI_HAS_MCFG )
>> +    {
>> +        mcfg = construct_mcfg(ctxt, config);
>> +        if (!mcfg)  
>
>Coding style.

Will fix.

>> +            return -1;
>> +
>> +        table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, mcfg);
>> +    }
>> +
>>      /* TPM TCPA and SSDT. */
>>      if ( (config->table_flags & ACPI_HAS_TCPA) &&
>>           (config->tis_hdr[0] == tis_signature[0]) &&
>> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
>> index a2efd23b0b..dd85b928e9 100644
>> --- a/tools/libacpi/libacpi.h
>> +++ b/tools/libacpi/libacpi.h
>> @@ -36,6 +36,7 @@
>>  #define ACPI_HAS_8042              (1<<13)
>>  #define ACPI_HAS_CMOS_RTC          (1<<14)
>>  #define ACPI_HAS_SSDT_LAPTOP_SLATE (1<<15)
>> +#define ACPI_HAS_MCFG              (1<<16)
>>  
>>  struct xen_vmemrange;
>>  struct acpi_numa {
>> @@ -96,6 +97,9 @@ struct acpi_config {
>>      uint32_t ioapic_base_address;
>>      uint16_t pci_isa_irq_mask;
>>      uint8_t ioapic_id;
>> +
>> +    uint64_t mmconfig_addr;
>> +    uint32_t mmconfig_len;  
>
>This interface is quite limited because it only allows us to create a
>single MCFG entry, but since this is not a public interface I guess it
>doesn't matter that much, it can always be expanded when required.

We will be limited to a single MMCONFIG area for a long time I'm
afraid, it will good to move away from the bus 0 limitation first.

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