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

[Alexey G <x1917x@xxxxxxxxx>]

On Tue, 20 Mar 2018 09:03:56 +0000
Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

>On Tue, Mar 20, 2018 at 07:46:04AM +1000, Alexey G wrote:
>> On Mon, 19 Mar 2018 17:33:34 +0000
>> Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxx>
>> >> ---
>> >>  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.  
>
>Note the name change also (acpi_10_mcfg). Also I think you can drop
>the acpi_10_mcfg_entry name and just use an anonymous struct.
>
>> >> +
>> >> +    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.  
>I'm not going to insist, but ACPI doesn't really care about the size,
>it just needs to know the start and end. Seems pointless to write a
>value here that later libacpi needs to convert to the value it
>actually needs. Also start/end buses are uint8_t, size is uint32_t.

As the underlying implementation is limited to just one PCI segment
and we need to pass only one MCFG range entry, I guess it will be ok to
use the mmconfig_addr + mmconfig_num_buses pair (almost same as
end_bus, but more size-descriptive).



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