Re: [PATCH V5 01/15] ACPI: MCFG: Move mmcfg_list management to drivers/acpi

[Jayachandran Chandrashekaran Nair]

Hi Tomasz,

On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki <tn@xxxxxxxxxxxx> wrote:
> Hi Bjorn,
>
> Thanks for your pointers! See my comments inline. Aslo, can you please have
> a look at my previous patch set v4 and check how many of your comments are
> already addressed there. We may want to back to it then.
>
> https://lkml.org/lkml/2016/2/4/646
> Especially patches [0-6] which handle MMCONFIG refactoring.
>
>
> On 05.03.2016 05:14, Bjorn Helgaas wrote:
>>
>> On Fri, Mar 04, 2016 at 02:05:56PM +0530, Jayachandran Chandrashekaran
>> Nair wrote:
>>>
>>> On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>>>>
>>>> Hi Tomasz, Jayachandran, et al,
>>>>
>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>>
>>>>> From: Jayachandran C <jchandra@xxxxxxxxxxxx>
>>>>>
>>>>> Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is
>>>>> to share the API and code with ARM64 later. The corresponding
>>>>> declarations are moved from asm/pci_x86.h to linux/pci-acpi.h
>>>>>
>>>>> As a part of this we introduce three functions that can be
>>>>> implemented by the arch code: pci_mmconfig_map_resource() to map a
>>>>> mcfg entry, pci_mmconfig_unmap_resource to do the corresponding
>>>>> unmap and pci_mmconfig_enabled to see if the arch setup of
>>>>> mcfg entries was successful. We also provide weak implementations
>>>>> of these, which will be used from ARM64. On x86, we retain the
>>>>> old logic by providing platform specific implementation.
>>>>>
>>>>> This patch is purely rearranging code, it should not have any
>>>>> impact on the logic of MCFG parsing or list handling.
>>>>
>>>>
>>>> I definitely want to figure out how to make this work well on ARM64.
>>>> I need to ponder this some more, so these are just some initial
>>>> thoughts.
>>>>
>>>> My first impression is that (a) the x86 MCFG code is an unmitigated
>>>> disaster, and (b) we're trying a little too hard to make that mess
>>>> generic.  I think we might be better served if we came up with some
>>>> cleaner, more generic code that we can use for ARM64 today, and
>>>> migrate x86 toward that over time.
>>>>
>>>> My concern is that if we elevate the current x86 code to be
>>>> "arch-independent", we will be perpetuating some interfaces and
>>>> designs that shouldn't be allowed to escape arch/x86.
>>>
>>>
>>> I think the major decision is whether to maintain the pci_mmcfg_list
>>> for all architectures or not. My initial plan was not to do this because
>>> of the mess (basically the ECAM region info should be attached to
>>> the pci root and not maintained in a separate list that needs locking),
>>> The patch I posted initially https://patchwork.ozlabs.org/patch/553464/
>>> had a much simpler way of handling the MCFG table without using
>>> the list.
>>
>>
>> I agree that ECAM info should be attached to the PCI host controller.
>> That should simplify locking and hot-add and hot-removal of host
>> controllers.
>>
>> I think pci_mmcfg_list is an implementation detail that may not need
>> to be generic.  I certainly don't think it needs to be part of the
>> interface.
>>
>>> In x86 case it is not feasible to remove using the pci_mmcfg_list.
>>> The only use of it outside is in xen that can be fixed up.
>>>
>>>> Some of the code that moved to drivers/acpi/pci_mcfg.c is not really
>>>> ACPI-specific, and could potentially be used for non-ACPI bridges that
>>>> support ECAM.  I'd like to see that sort of code moved to a new file
>>>> like drivers/pci/ecam.c.
>>>>
>>>> There's a little bit of overlap here with the ECAM code in
>>>> pci-host-generic.c.  I'm not sure whether or how to include that, but
>>>> it's a very good example of how simple this *should* be: probe the
>>>> host bridge, discover the ECAM region, request the region, ioremap it,
>>>> done.
>>>
>>>
>>> I had a similar approach in my initial patchset, please see the patch
>>> above. The resource for ECAM is mapped similar to the the way
>>> pci-host-generic.c handled it. An additional step I could do was to
>>> move the common code (ioremap and mapbus) into a common
>>> file and share the code with pci-host-generic.c
>>>
>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>>> new file mode 100644
>>>>> index 0000000..ea84365
>>>>> --- /dev/null
>>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>>> ...
>>>>> +int __weak pci_mmconfig_map_resource(struct device *dev,
>>>>> +     struct pci_mmcfg_region *mcfg)
>>>>> +{
>>>>> +     struct resource *tmp;
>>>>> +     void __iomem *vaddr;
>>>>> +
>>>>> +     tmp = insert_resource_conflict(&iomem_resource, &mcfg->res);
>>>>> +     if (tmp) {
>>>>> +             dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
>>>>> +                     &mcfg->res, tmp->name, tmp);
>>>>> +             return -EBUSY;
>>>>> +     }
>>>>
>>>>
>>>> I think this is a mistake in the x86 MCFG support that we should not
>>>> carry over to a generic implementation.  We should not use the MCFG
>>>> table for resource reservation because MCFG is not defined by the ACPI
>>>> spec and an OS need not include support for it.  The platform must
>>>> indicate in some other, more generic way, that ECAM space is reserved.
>>>> This probably means ECAM space should be declared in a PNP0C02 _CRS
>>>> method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).
>>>>
>>>> We might need some kind of x86-specific quirk that does this, or a
>>>> pcibios hook or something here; I just don't think it should be
>>>> generic.
>>>>
>>>>> +int __init pci_mmconfig_parse_table(void)
>>>>> +{
>>>>> +     return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
>>>>> +}
>>>>
>>>>
>>>> I don't like the fact that we parse the entire MCFG table at once
>>>> here.  I think we should look for the information we need when we are
>>>> claiming a PCI host bridge, e.g., in acpi_pci_root_add().  This might
>>>> not be a great fit for the way ACPI table management works, but I
>>>> think it's better to do things on-demand rather than just-in-case.
>>>
>>>
>>> There is an overhead of looking up this table, and the information
>>> available there is very limited (i.e, segment, start_bus, end_bus
>>> and address). My approach in the above patch is to save this info
>>> into an array at boot time and avoid multiple lookups.
>>
>>
>> We need to look up MCFG info once per host bridge, so I don't think
>> there's any performance issue here.  But we do use acpi_table_parse(),
>> which is __init, and *that* is a reason why we might need to parse the
>> entire MCFG at boot-time.  But this is the least of our worries in any
>> case.
>>
>>>>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>>>>> index 89ab057..e9450ef 100644
>>>>> --- a/include/linux/pci-acpi.h
>>>>> +++ b/include/linux/pci-acpi.h
>>>>> @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[];
>>>>>   #define RESET_DELAY_DSM              0x08
>>>>>   #define FUNCTION_DELAY_DSM   0x09
>>>>>
>>>>> +/* common API to maintain list of MCFG regions */
>>>>> +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */
>>>>> +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)
>>>>> +
>>>>> +struct pci_mmcfg_region {
>>>>> +     struct list_head list;
>>>>> +     struct resource res;
>>>>> +     u64 address;
>>>>> +     char __iomem *virt;
>>>>> +     u16 segment;
>>>>> +     u8 start_bus;
>>>>> +     u8 end_bus;
>>>>> +     char name[PCI_MMCFG_RESOURCE_NAME_LEN];
>>>>> +};
>>>>> +
>>>>> +extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start,
>>>>> u8 end,
>>>>> +                            phys_addr_t addr);
>>>>> +extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);
>>>>> +
>>>>> +extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int
>>>>> bus);
>>>>> +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int
>>>>> start,
>>>>> +                                                     int end, u64
>>>>> addr);
>>>>> +extern int pci_mmconfig_map_resource(struct device *dev,
>>>>> +     struct pci_mmcfg_region *mcfg);
>>>>> +extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region
>>>>> *mcfg);
>>>>> +extern int pci_mmconfig_enabled(void);
>>>>> +extern int __init pci_mmconfig_parse_table(void);
>>>>> +
>>>>> +extern struct list_head pci_mmcfg_list;
>>>>> +
>>>>> +#define PCI_MMCFG_BUS_OFFSET(bus)      ((bus) << 20)
>>>>> +#define PCI_MMCFG_OFFSET(bus, devfn)   ((bus) << 20 | (devfn) << 12)
>>>>> +
>>>>
>>>>
>>>> With the exception of pci_mmconfig_parse_table(), nothing here is
>>>> ACPI-specific.  I'd like to see the PCI ECAM-related interfaces
>>>> (hopefully not these exact ones, but a more rational set) put in
>>>> something like include/linux/pci-ecam.h.
>>>>
>>>>>   #else        /* CONFIG_ACPI */
>>>>>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>>>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>>
>>>
>>> I can update this patch to
>>>   -  drop the pci_mmcfg_list handling from generic case
>>>   -  move common ECAM code so that it can be shared with
>>>      pci-host-generic.c
>>> if that is what you are looking for. The code will end up looking much
>>> simpler.
>>
>>
>> I think we should ignore x86 mmconfig for now.  It is absurdly
>> complicated and I'm not sure it's fixable.  I *do* want to keep
>> drivers/acpi/pci_root.c for all ACPI host bridges, including x86,
>> ia64, and arm64.
>>
>> So I think we should write generic MCFG and ECAM support from scratch
>> for arm64.  Something like this:
>>
>>    - Add an acpi_mcfg_init(), maybe in drivers/acpi/pci_mcfg.c, to be
>>      called from acpi_init() to copy MCFG info to something we can
>>      access after __init.  This would not reserve resources, but
>>      probably does have to ioremap() the regions to support
>>      raw_pci_read().
>
>
> As said, ECAM and ACPI specific code was isolated in previous patch. There,
> I tried to leave x86 complication in arch/x86/ and extract generic
> functionalities to driver/pci/ecam.c as the library.
>
>>
>>    - Implement raw_pci_read(), which is "special" because ACPI needs it
>>      for PCI config access from AML.  It's supposed to be "always
>>      accessible" and we don't have a struct pci_bus *, so this probably
>>      has to use the MCFG copy and the ioremap done above.  Maybe it
>>      should go in the same file.  This is completely independent of
>>      the PCI core and PCI data structures.
>
> We were looking for the answer which would justify RAW PCI config accessors
> being for ARM64 world. Unfortunately, nobody was able to show real use case
> for ARM64. Do you see the reason we need this? Our conclusion was to leave
> it empty for ARM64 which in turn makes code simpler. I am not ASWG member
> while that was under discussion so I will ask Lorenzo to elaborate more on
> this.
>
>>
>>    - Implement arm64 pci_acpi_scan_root() that calls
>>      acpi_pci_root_create() with an .init_info() function that calls
>>      acpi_pci_root_get_mcfg_addr() to read _CBA, and if that fails,
>>      looks up the bus range in the MCFG copy from above.  It should
>>      call request_mem_region().  For a region from _CBA, it should call
>>      ioremap().  For regions from MCFG it can probably use the ioremap
>>      done by acpi_mcfg_init().
>
> Yes, Expanding .init_info() to check for _CBA is good point.
>
>>
>>      I know acpi_pci_root_add() calls acpi_pci_root_get_mcfg_addr()
>>      before calling pci_acpi_scan_root(), but I think that's wrong
>>      because (a) some arches, e.g., ia64, don't use ECAM and (b) _CBA
>>      and MCFG should be handled in the same place.
>>
>>      I know calling request_mem_region() here will probably be an
>>      ordering problem because the PNP0C02 driver hasn't reserved
>>      resources yet.  But the host bridge driver is using the region and
>>      it should reserve it.
>>
>>    - If we store the ECAM mapped base address in the sysdata or struct
>>      pci_host_bridge, the normal config accessors can use
>>      pci_generic_config_read() with a new generic .map_bus() function.
>
>
> pci_generic_config_{read|write}() is what we want to use, actually we do
> now, but ECAM region and sysdata association will remove ECAM region lookup
> step (see patch 09/15 of this series).
>
>>
>>      On x86, the normal config access path is:
>>
>>        pci_read(struct pci_bus *, ...)
>>          raw_pci_read(seg, bus#, ...)
>>           raw_pci_ext_ops->read(seg, bus#, ...)
>>             pci_mmcfg_read(seg, bus#, ...)
>>               pci_dev_base
>>                 pci_mmconfig_lookup(seg, bus#)
>>
>>      I think this is somewhat backwards because we start with a pci_bus
>>      pointer, so we *could* have a nice simple bus-specific accessor,
>>      but we throw that pointer away, so pci_mmcfg_read() has to start
>>      over and look up the ECAM offset from scratch, which makes it all
>>      unnecessarily complicated.
>>
>
> As you pointed out raw_pci_{read|write} make things complicated, so IMO we
> should either say they are absolutely necessary (and then think how to
> simplify it) or just use simple bus-specific accessor (patch 02/15) e.g. for
> ARM64.

Both raw_pci_read/write and a host controller with pci_generic_read/write can
be done without much trouble, please see the patch I had at:
https://patchwork.ozlabs.org/patch/575526/

I have been looking at Bjorn's suggestions and trying to see if I can update
my MCFG patch taking care of them. I will post an updated patchset soon,
unless you want to take this up.

JC.