Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371665 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V5 01/15] ACPI: MCFG: Move mmcfg_list management to  drivers/acpi Tomasz Nowicki <tn@semihalf.com> - 2016-04-05 16:20 +0200
    Re: [PATCH V5 01/15] ACPI: MCFG: Move mmcfg_list management to  drivers/acpi Bjorn Helgaas <helgaas@kernel.org> - 2016-04-05 18:50 +0200
      Re: [PATCH V5 01/15] ACPI: MCFG: Move mmcfg_list management to  drivers/acpi Tomasz Nowicki <tn@semihalf.com> - 2016-04-05 20:10 +0200
      Re: [PATCH V5 01/15] ACPI: MCFG: Move mmcfg_list management to drivers/acpi Jayachandran C <jchandra@broadcom.com> - 2016-04-05 21:00 +0200

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

On 09.03.2016 10:13, Tomasz Nowicki 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@kernel.org>
>>> wrote:
>>>> Hi Tomasz, Jayachandran, et al,
>>>>
>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>> From: Jayachandran C <jchandra@broadcom.com>
>>>>>
>>>>> 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.
>
> Any comments appreciated.
>

Hi Bjorn,

Kindly reminder. I would like to move on with this patch set. Can you 
please comments on it so that we could decide which way to go.

Regards,
Tomasz

[toc] | [next] | [standalone]

#1371782

Hi Tomasz,

On Tue, Apr 05, 2016 at 04:11:55PM +0200, Tomasz Nowicki wrote:
> On 09.03.2016 10:13, Tomasz Nowicki 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@kernel.org>
> >>>wrote:
> >>>>Hi Tomasz, Jayachandran, et al,
> >>>>
> >>>>On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
> >>>>>From: Jayachandran C <jchandra@broadcom.com>
> >>>>>
> >>>>>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.
> >
> >Any comments appreciated.
> 
> Kindly reminder. I would like to move on with this patch set. Can
> you please comments on it so that we could decide which way to go.

Can you repost your current proposal with a version number higher than
any previous ones?  It's OK if the content is the same as v4; I just
think it's confusing if we resurrect v4 and have to follow discussion
from v3 to v4 to v5 and back to v4.  The archives would be a bit of a
muddle.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1371847

Hi Bjorn,

On 05.04.2016 18:41, Bjorn Helgaas wrote:
> Hi Tomasz,
>

[...]

>>>>
>>>
>>> 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.
>>>
>>> Any comments appreciated.
>>
>> Kindly reminder. I would like to move on with this patch set. Can
>> you please comments on it so that we could decide which way to go.
>
> Can you repost your current proposal with a version number higher than
> any previous ones?  It's OK if the content is the same as v4; I just
> think it's confusing if we resurrect v4 and have to follow discussion
> from v3 to v4 to v5 and back to v4.  The archives would be a bit of a
> muddle.
>

Sure I will repost ASAP.

Thanks!
Tomasz

[toc] | [prev] | [next] | [standalone]

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

Hi Bjorn,

On Tue, Apr 5, 2016 at 10:11 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Hi Tomasz,
>
> On Tue, Apr 05, 2016 at 04:11:55PM +0200, Tomasz Nowicki wrote:
> > On 09.03.2016 10:13, Tomasz Nowicki 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:
> [...]
> > >
> > >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.
> > >
> > >Any comments appreciated.
>
> > Kindly reminder. I would like to move on with this patch set. Can
> > you please comments on it so that we could decide which way to go.
>
> Can you repost your current proposal with a version number higher than
> any previous ones?  It's OK if the content is the same as v4; I just
> think it's confusing if we resurrect v4 and have to follow discussion
> from v3 to v4 to v5 and back to v4.  The archives would be a bit of a
> muddle.

I had posted a patchset based on your suggestions in this thread
https://lkml.org/lkml/2016/3/17/621

Would appreciate any comments on that. Like I said in the earlier
mail, if this is a reasonable approach, I can combine this with
Tomasz patchset to provide the full patchset for ACPI support.

Thanks,
JC.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web