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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.

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