2016-03-03 22:51:59

by Bjorn Helgaas

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

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
> From: Jayachandran C <[email protected]>
>
> 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.

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.

> 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.

> 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) { }

Bjorn


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

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas <[email protected]> wrote:
> Hi Tomasz, Jayachandran, et al,
>
> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>> From: Jayachandran C <[email protected]>
>>
>> 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.

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.

>> 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.

Thanks,
JC.

2016-03-04 09:28:20

by Tomasz Nowicki

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

Hi Bjorn,

On 03.03.2016 23:51, Bjorn Helgaas wrote:
> Hi Tomasz, Jayachandran, et al,
>
> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>> From: Jayachandran C <[email protected]>
>>
>> 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.
>
> 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.

Actually I split it as you suggested in the previous patch set. Please
have a look at:
https://lkml.org/lkml/2016/2/4/646

Especially patches [0-6] which handle MMCONFIG refactoring.

Thanks,
Tomasz

2016-03-05 04:14:55

by Bjorn Helgaas

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

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 <[email protected]> wrote:
> > Hi Tomasz, Jayachandran, et al,
> >
> > On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
> >> From: Jayachandran C <[email protected]>
> >>
> >> 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().

- 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.

- 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().

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.

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.

Bjorn

2016-03-09 09:13:26

by Tomasz Nowicki

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

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 <[email protected]> wrote:
>>> Hi Tomasz, Jayachandran, et al,
>>>
>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>> From: Jayachandran C <[email protected]>
>>>>
>>>> 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.

Thanks,
Tomasz

2016-03-09 09:15:08

by Tomasz Nowicki

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

On 09.03.2016 10:13, Tomasz Nowicki wrote:
>> 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.

I meant to say, in my previous patch set (V4), sorry.

Tomasz

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

Hi Tomasz,

On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki <[email protected]> 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 <[email protected]> wrote:
>>>>
>>>> Hi Tomasz, Jayachandran, et al,
>>>>
>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>>
>>>>> From: Jayachandran C <[email protected]>
>>>>>
>>>>> 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.

2016-03-09 10:51:13

by Tomasz Nowicki

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

Hi Jayachandran,

On 09.03.2016 11:10, Jayachandran Chandrashekaran Nair wrote:
> Hi Tomasz,
>
> On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki <[email protected]> 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 <[email protected]> wrote:
>>>>>
>>>>> Hi Tomasz, Jayachandran, et al,
>>>>>
>>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>>>
>>>>>> From: Jayachandran C <[email protected]>
>>>>>>
>>>>>> 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/

Yes it is doable, I implemented it in the same way in one of my initial
patch series, about year ago. I'm questioning raw accessors presence on
per-arch basis. If it is really needed for all archs, then we definitely
should implement it. If ARM64 does not care for it, there is no point to
complicate it. Especially, I mean all kind of PCI config space quirk we
will need to handle right after this patch got merged, see:
[PATCH V5 13/15] pci, acpi: Match PCI config space accessors against
platfrom specific quirks.

and

https://lkml.org/lkml/2016/2/9/627
https://lkml.org/lkml/2016/2/8/967

Giving these quirks, raw accessors are not so easy.

>
> 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 patch set soon,
> unless you want to take this up.
>

Yes, I want to post next version and keep this patch set together, if
you and Bjorn are okay. I am feeling that my previous patch set is close
to what Bjorn suggested, modulo the way we keep MCFG regions. Lets
discuss it here, then I will post it as next version. I am looking
forward to hear Bjorn's comment on my previous patch set.

Tomasz

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

Hi Tomasz,

On Wed, Mar 9, 2016 at 4:20 PM, Tomasz Nowicki <[email protected]> wrote:
> Hi Jayachandran,
>
>
> On 09.03.2016 11:10, Jayachandran Chandrashekaran Nair wrote:
>>
>> Hi Tomasz,
>>
>> On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki <[email protected]> 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 <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Tomasz, Jayachandran, et al,
>>>>>>
>>>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>>>>
>>>>>>>
>>>>>>> From: Jayachandran C <[email protected]>
>>>>>>>
>>>>>>> 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/
>
>
> Yes it is doable, I implemented it in the same way in one of my initial
> patch series, about year ago. I'm questioning raw accessors presence on
> per-arch basis. If it is really needed for all archs, then we definitely
> should implement it. If ARM64 does not care for it, there is no point to
> complicate it. Especially, I mean all kind of PCI config space quirk we will
> need to handle right after this patch got merged, see:
> [PATCH V5 13/15] pci, acpi: Match PCI config space accessors against
> platfrom specific quirks.
>
> and
>
> https://lkml.org/lkml/2016/2/9/627
> https://lkml.org/lkml/2016/2/8/967
>
> Giving these quirks, raw accessors are not so easy.

The whole quirk handling infrastructure seems to be an overkill to me.
I will leave it to maintainers to comment further.

>>
>> 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 patch set soon,
>> unless you want to take this up.
>>
>
> Yes, I want to post next version and keep this patch set together, if you
> and Bjorn are okay. I am feeling that my previous patch set is close to what
> Bjorn suggested, modulo the way we keep MCFG regions. Lets discuss it here,
> then I will post it as next version. I am looking forward to hear Bjorn's
> comment on my previous patch set.

I have been looking thru the code, and I have a reasonable implementation
which updates this one patch. This pulls in common code from pci-host-generic.c
as well. I will post it by next week and you can decide whether to use it
to update your patchset.

Thanks,
JC.

2016-03-17 20:34:02

by Jayachandran C

[permalink] [raw]
Subject: [RFC PATCH 1/4] PCI: Provide generic ECAM mapping functions

Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
provide generic functions to access memory mapped PCI config space.

The API defines 'struct pci_config_window' to hold the mappings.
The function pci_generic_map_config() is provided to allocate the
struct, request the memory region, and do the ioremap() calls
needed to setup the mapping. pci_generic_unmap_config() is provided
to delete a mapping.

A helper function pci_generic_map_bus() is also provided to be used
to implement pci_ops map_bus method.

Signed-off-by: Jayachandran C <[email protected]>
---
drivers/pci/Kconfig | 3 ++
drivers/pci/Makefile | 2 +
drivers/pci/ecam.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 10 ++++
4 files changed, 142 insertions(+)
create mode 100644 drivers/pci/ecam.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 209292e..e930d62 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -83,6 +83,9 @@ config HT_IRQ
config PCI_ATS
bool

+config PCI_GENERIC_ECAM
+ bool
+
config PCI_IOV
bool "PCI IOV support"
depends on PCI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 2154092..810aec8 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o

obj-$(CONFIG_PCI_STUB) += pci-stub.o

+obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o
+
obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o

obj-$(CONFIG_OF) += of.o
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
new file mode 100644
index 0000000..6a63901
--- /dev/null
+++ b/drivers/pci/ecam.c
@@ -0,0 +1,127 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+/*
+ * On 64 bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32 bit, do an
+ * ioremap per bus.
+ */
+static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
+
+/*
+ * struct to hold the mappings of a config space window. This
+ * will be allocated with enough entries in win[] to hold all
+ * the mappings for the bus range.
+ */
+struct pci_config_window {
+ phys_addr_t cfgaddr;
+ u8 bus_start;
+ u8 bus_end;
+ u8 bus_shift;
+ u8 devfn_shift;
+ void __iomem *win[0];
+};
+
+/*
+ * helper function provided to implement the pci_ops ->map_bus method
+ */
+void __iomem *pci_generic_map_bus(struct pci_config_window *cfg,
+ unsigned int busn, unsigned int devfn, int where)
+{
+ void __iomem *base;
+
+ if (busn < cfg->bus_start || busn > cfg->bus_end)
+ return NULL;
+
+ busn -= cfg->bus_start;
+ if (per_bus_mapping)
+ base = cfg->win[busn];
+ else
+ base = cfg->win[0] + (busn << cfg->bus_shift);
+ return base + (devfn << cfg->devfn_shift) + where;
+}
+
+/*
+ * Create a PCI config space window
+ * - reserve mem region
+ * - alloc struct pci_config_window with space for all mappings
+ * - ioremap the config space
+ */
+struct pci_config_window *pci_generic_map_config(phys_addr_t addr,
+ u8 bus_start, u8 bus_end, u8 bus_shift, u8 devfn_shift)
+{
+ struct pci_config_window *cfg;
+ unsigned int bus_range, bsz, mapsz;
+ int i, nidx;
+
+ if (bus_end < bus_start)
+ return ERR_PTR(-EINVAL);
+
+ bus_range = bus_end - bus_start + 1;
+ bsz = 1 << bus_shift;
+ nidx = per_bus_mapping ? bus_range : 1;
+ mapsz = per_bus_mapping ? bsz : bus_range * bsz;
+ cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL);
+ if (!cfg)
+ return ERR_PTR(-ENOMEM);
+
+ cfg->bus_start = bus_start;
+ cfg->bus_end = bus_end;
+ cfg->bus_shift = bus_shift;
+ cfg->devfn_shift = devfn_shift;
+
+ if (!request_mem_region(addr, bus_range * bsz, "Configuration Space"))
+ goto err_exit;
+
+ /* cfgaddr has to be set after request_mem_region */
+ cfg->cfgaddr = addr;
+
+ for (i = 0; i < nidx; i++) {
+ cfg->win[i] = ioremap(addr + i * mapsz, mapsz);
+ if (!cfg->win[i])
+ goto err_exit;
+ }
+ return cfg;
+
+err_exit:
+ pci_generic_unmap_config(cfg);
+ return ERR_PTR(-ENOMEM);
+}
+
+/*
+ * Free a config space mapping
+ */
+void pci_generic_unmap_config(struct pci_config_window *cfg)
+{
+ unsigned int bus_range;
+ int i, nidx;
+
+ bus_range = cfg->bus_end - cfg->bus_start + 1;
+ nidx = per_bus_mapping ? bus_range : 1;
+ for (i = 0; i < nidx; i++)
+ if (cfg->win[i])
+ iounmap(cfg->win[i]);
+ if (cfg->cfgaddr)
+ release_mem_region(cfg->cfgaddr, bus_range << cfg->bus_shift);
+ kfree(cfg);
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3df9e37..33c46bc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2013,6 +2013,16 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
return bus->self && bus->self->ari_enabled;
}

+/* Generic ECAM mapping API */
+#ifdef CONFIG_PCI_GENERIC_ECAM
+struct pci_config_window;
+void __iomem *pci_generic_map_bus(struct pci_config_window *cfg,
+ unsigned int busn, unsigned int devfn, int where);
+struct pci_config_window *pci_generic_map_config(phys_addr_t addr,
+ u8 bus_start, u8 bus_end, u8 bus_shift, u8 devfn_shift);
+void pci_generic_unmap_config(struct pci_config_window *cfg);
+#endif
+
/* provide the legacy pci_dma_* API */
#include <linux/pci-dma-compat.h>

--
1.9.1

2016-03-17 20:34:09

by Jayachandran C

[permalink] [raw]
Subject: [RFC PATCH 3/4] ACPI: PCI: Add generic PCI host controller

Add a generic ACPI based PCI host controller, and provide config
option ACPI_PCI_HOST_GENERIC to enable it.

The implementation selects PCI_MMCONFIG and implements function
pci_mmcfg_late_init() to parse MCFG table and save its entries.
PCI_GENERIC_ECAM is also selected and pci/ecam.c functions are
used to map the config space of the PCI controller.

acpi_pci_root_ops is setup so that init_info looks up the saved
MCFG entries and sets up the config space mapping. release_info
deletes the mapping and frees the memory. Generic PCI functions
are used for accessing config space.

Signed-off-by: Jayachandran C <[email protected]>
---
drivers/acpi/Kconfig | 9 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/pci_gen_host.c | 247 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 drivers/acpi/pci_gen_host.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 82b96ee..f178f2e 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -343,6 +343,15 @@ config ACPI_PCI_SLOT
i.e., segment/bus/device/function tuples, with physical slots in
the system. If you are unsure, say N.

+config ACPI_PCI_HOST_GENERIC
+ bool "Generic ACPI based PCI controller"
+ depends on ARM64
+ select PCI_MMCONFIG
+ select PCI_GENERIC_ECAM
+ help
+ Say Y if you want the generic ACPI based PCI controller
+ implementation.
+
config X86_PM_TIMER
bool "Power Management Timer Support" if EXPERT
depends on X86
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index cb648a4..ceab5b0 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_ACPI_BUTTON) += button.o
obj-$(CONFIG_ACPI_FAN) += fan.o
obj-$(CONFIG_ACPI_VIDEO) += video.o
obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
+obj-$(CONFIG_ACPI_PCI_HOST_GENERIC) += pci_gen_host.o
obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
obj-$(CONFIG_ACPI) += container.o
obj-$(CONFIG_ACPI_THERMAL) += thermal.o
diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
new file mode 100644
index 0000000..a43f2cee
--- /dev/null
+++ b/drivers/acpi/pci_gen_host.c
@@ -0,0 +1,247 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#define PREFIX "ACPI: "
+
+/*
+ * Structure to hold entries from the MCFG table, these need to be
+ * mapped until a pci host bridge claims them for raw_pci_read/write
+ * to work
+ */
+struct mcfg_entry {
+ phys_addr_t addr;
+ u16 segment;
+ u8 bus_start;
+ u8 bus_end;
+};
+
+/*
+ * Global to save mcfg entries
+ */
+static struct {
+ struct mcfg_entry *cfg;
+ int size;
+} mcfgsav;
+
+/* List of all ACPI PCI roots, needed for raw operations */
+static struct list_head gen_acpi_pci_roots;
+
+/* lock for global table AND list above */
+static struct mutex gen_acpi_pci_lock;
+
+/* ACPI info for generic ACPI PCI controller */
+struct acpi_pci_generic_root_info {
+ struct acpi_pci_root_info common;
+ struct pci_config_window *cfg; /* config space mapping */
+ struct list_head node; /* node in acpi_pci_roots */
+};
+
+/* Call generic map_bus after getting cfg pointer */
+static void __iomem *gen_acpi_map_cfg_bus(struct pci_bus *bus,
+ unsigned int devfn, int where)
+{
+ struct acpi_pci_generic_root_info *ri = bus->sysdata;
+
+ return pci_generic_map_bus(ri->cfg, bus->number, devfn, where);
+}
+
+static struct pci_ops acpi_pci_ops = {
+ .map_bus = gen_acpi_map_cfg_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+/* find the entry in mcfgsav.cfg which contains range bus_start..bus_end */
+static int mcfg_lookup(u16 seg, u8 bus_start, u8 bus_end)
+{
+ struct mcfg_entry *e;
+ int i;
+
+ for (i = 0, e = mcfgsav.cfg; i < mcfgsav.size; i++, e++) {
+ if (seg != e->segment)
+ continue;
+ if (bus_start >= e->bus_start && bus_start <= e->bus_end)
+ return (bus_end <= e->bus_end) ? i : -EINVAL;
+ else if (bus_end >= e->bus_start && bus_end <= e->bus_end)
+ return -EINVAL;
+ }
+ return -ENOENT;
+}
+
+/*
+ * init_info - lookup the bus range for the domain in MCFG, and set up
+ * config space mapping.
+ */
+static int pci_acpi_generic_init_info(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_generic_root_info *ri;
+ struct acpi_pci_root *root = ci->root;
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ phys_addr_t addr = root->mcfg_addr;
+ struct mcfg_entry *e;
+ int ret;
+
+ ri = container_of(ci, struct acpi_pci_generic_root_info, common);
+
+ mutex_lock(&gen_acpi_pci_lock);
+ ret = mcfg_lookup(seg, bus_start, bus_end);
+ switch (ret) {
+ case -ENOENT:
+ if (addr != 0) /* use address from _CBA */
+ break;
+ pr_err("%04x:%02x-%02x mcfg lookup failed\n", seg,
+ bus_start, bus_end);
+ goto err_out;
+ case -EINVAL:
+ pr_err("%04x:%02x-%02x bus range error\n", seg, bus_start,
+ bus_end);
+ goto err_out;
+ default:
+ e = &mcfgsav.cfg[ret];
+ if (addr == 0)
+ addr = e->addr;
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ goto err_out;
+ }
+ if (addr != e->addr) {
+ pr_warn("%04x:%02x-%02x addr mismatch, ignoring MCFG\n",
+ seg, bus_start, bus_end);
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ break;
+ }
+
+ ri->cfg = pci_generic_map_config(addr, bus_start, bus_end, 20, 12);
+ if (IS_ERR(ri->cfg)) {
+ ret = PTR_ERR(ri->cfg);
+ pr_err("%04x:%02x-%02x error %d mapping CFG\n", seg, bus_start,
+ bus_end, ret);
+ goto err_out;
+ }
+ list_add_tail(&ri->node, &gen_acpi_pci_roots);
+err_out:
+ mutex_unlock(&gen_acpi_pci_lock);
+ return ret;
+}
+
+/* release_info: free resrouces allocated by init_info */
+static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
+{
+ struct acpi_pci_generic_root_info *ri;
+
+ ri = container_of(ci, struct acpi_pci_generic_root_info, common);
+
+ mutex_lock(&gen_acpi_pci_lock);
+ list_del(&ri->node);
+ pci_generic_unmap_config(ri->cfg);
+ mutex_unlock(&gen_acpi_pci_lock);
+
+ kfree(ri);
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+ .pci_ops = &acpi_pci_ops,
+ .init_info = pci_acpi_generic_init_info,
+ .release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+ int node = acpi_get_node(root->device->handle);
+ struct acpi_pci_generic_root_info *ri;
+ struct pci_bus *bus, *child;
+
+ ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
+ if (!ri)
+ return NULL;
+
+ bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common, ri);
+ if (!bus)
+ return NULL;
+
+ pci_bus_size_bridges(bus);
+ pci_bus_assign_resources(bus);
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+
+ return bus;
+}
+
+/* handle MCFG table entries */
+static __init int handle_mcfg(struct acpi_table_header *header)
+{
+ struct acpi_table_mcfg *mcfg;
+ struct acpi_mcfg_allocation *mptr;
+ struct mcfg_entry *e, *arr;
+ int i, n;
+
+ if (!header)
+ return -EINVAL;
+
+ mcfg = (struct acpi_table_mcfg *)header;
+ mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+ n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
+ if (n <= 0 || n > 255) {
+ pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
+ return -EINVAL;
+ }
+
+ arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
+ if (!arr)
+ return -ENOMEM;
+
+ for (i = 0, e = arr; i < n; i++, mptr++, e++) {
+ e->segment = mptr->pci_segment;
+ e->addr = mptr->address;
+ e->bus_start = mptr->start_bus_number;
+ e->bus_end = mptr->end_bus_number;
+ }
+
+ mcfgsav.cfg = arr;
+ mcfgsav.size = n;
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mmcfg_late_init(void)
+{
+ int err;
+
+ mutex_init(&gen_acpi_pci_lock);
+ INIT_LIST_HEAD(&gen_acpi_pci_roots);
+ err = acpi_sfi_table_parse(ACPI_SIG_MCFG, handle_mcfg);
+ if (err) {
+ pr_err(PREFIX " Failed to parse MCFG (%d)\n", err);
+ mcfgsav.size = 0;
+ } else {
+ pr_info(PREFIX " MCFG table at %p, %d entries.\n",
+ mcfgsav.cfg, mcfgsav.size);
+ }
+}
--
1.9.1

2016-03-17 20:34:25

by Jayachandran C

[permalink] [raw]
Subject: [RFC PATCH 4/4] ACPI: PCI: Add raw_pci_read/write operations

Provide implementations of raw_pci_read and raw_pci_write needed
by ACPI.

We already maintain a gen_acpi_pci_roots list with all the ACPI
PCI controllers, so we walk thru this list to see if there is
an existing mapping. If there is one, we can use the generic
config space access.

If there is no existing mapping, create a temporary mapping with
information available in the saved MCFG table and use it to do raw
config space access.

Signed-off-by: Jayachandran C <[email protected]>
---
drivers/acpi/pci_gen_host.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)

diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
index a43f2cee..b5472f0 100644
--- a/drivers/acpi/pci_gen_host.c
+++ b/drivers/acpi/pci_gen_host.c
@@ -245,3 +245,90 @@ void __init pci_mmcfg_late_init(void)
mcfgsav.cfg, mcfgsav.size);
}
}
+
+/*
+ * First walk thru the root infos of all the known ACPI PCI
+ * controllers to see if there is an existing mapping and
+ * use it if we find one.
+ * Otherwise check the MCFG table and setup a temporary mapping
+ */
+static int raw_pci_op(int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 *val, bool write)
+{
+ void __iomem *m;
+ struct acpi_pci_generic_root_info *ri;
+ struct acpi_pci_root *root;
+ bool tmpmap = false;
+ int i, ret = PCIBIOS_DEVICE_NOT_FOUND;
+
+ mutex_lock(&gen_acpi_pci_lock);
+ list_for_each_entry(ri, &gen_acpi_pci_roots, node) {
+ root = ri->common.root;
+ if (domain == root->segment &&
+ busn >= (u8)root->secondary.start &&
+ busn <= (u8)root->secondary.end) {
+ m = pci_generic_map_bus(ri->cfg, busn, devfn, reg);
+ if (m)
+ goto found;
+ else
+ goto err_out;
+ }
+ }
+
+ /* not found in existing root buses, check in mcfg */
+ i = mcfg_lookup(domain, busn, busn);
+ if (i < 0)
+ goto err_out;
+
+ /* get a temporary mapping */
+ busn -= mcfgsav.cfg[i].bus_start;
+ m = ioremap(mcfgsav.cfg[i].addr + (busn << 20 | devfn << 12), 1 << 12);
+ if (!m)
+ goto err_out;
+ tmpmap = true;
+ m += reg;
+found:
+ if (write) {
+ switch (len) {
+ case 1:
+ writeb(*val, m);
+ break;
+ case 2:
+ writew(*val, m);
+ break;
+ case 4:
+ writel(*val, m);
+ break;
+ }
+ } else {
+ switch (len) {
+ case 1:
+ *val = readb(m);
+ break;
+ case 2:
+ *val = readw(m);
+ break;
+ case 4:
+ *val = readl(m);
+ break;
+ }
+ }
+ if (tmpmap)
+ iounmap(m);
+ ret = 0;
+err_out:
+ mutex_unlock(&gen_acpi_pci_lock);
+ return ret;
+}
+
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 *val)
+{
+ return raw_pci_op(domain, busn, devfn, reg, len, val, false);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 val)
+{
+ return raw_pci_op(domain, busn, devfn, reg, len, &val, true);
+}
--
1.9.1

2016-03-17 20:34:44

by Jayachandran C

[permalink] [raw]
Subject: [RFC PATCH 2/4] PCI: generic,thunder: Use generic config functions

Use functions provided by pci/ecam.c for mapping config space. The
gen_pci structure is updated to store a pointer to the mapping.

'struct gen_pci_cfg_windows' is no longer needed since the mapping
is handled by generic code and 'struct gen_pci_cfg_bus_ops' can be
removed since bus shift is handled by generic code. The rest of the
data related to generic host controller are moved into 'struct gen_pci'
itself.

The generic code handles the bus and devfn shifts, so a common
function gen_pci_map_cfg_bus() has been added which can be used
as ->map_bus by all users.

With the updated interface, the users have to allocate a gen_pci
instance and setup the bus_shift and pci_ops fields in the structure
before calling pci_host_common_probe().

Update pci-host-common.c, pci-host-generic.c, pci-thunder-ecam.c
and pci-thunder-pem.c to use the new interface.

Also add dependency of PCI_GENERIC_ECAM for PCI_HOST_GENERIC
in Kconfig

Signed-off-by: Jayachandran C <[email protected]>
---
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 68 +++++++++++++++++++------------------
drivers/pci/host/pci-host-common.h | 25 +++++---------
drivers/pci/host/pci-host-generic.c | 51 +++++-----------------------
drivers/pci/host/pci-thunder-ecam.c | 33 +++++-------------
drivers/pci/host/pci-thunder-pem.c | 41 +++++++---------------
6 files changed, 74 insertions(+), 145 deletions(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index da61fa77..d27b989 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -82,6 +82,7 @@ config PCI_HOST_GENERIC
bool "Generic PCI host controller"
depends on (ARM || ARM64) && OF
select PCI_HOST_COMMON
+ select PCI_GENERIC_ECAM
help
Say Y here if you want to support a simple generic PCI host
controller, such as the one emulated by kvmtool.
diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
index e9f850f..d3b0664 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -24,6 +24,14 @@

#include "pci-host-common.h"

+void __iomem *gen_pci_map_cfg_bus(struct pci_bus *bus,
+ unsigned int devfn, int where)
+{
+ struct gen_pci *pci = bus->sysdata;
+
+ return pci_generic_map_bus(pci->cfg, bus->number, devfn, where);
+}
+
static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
{
pci_free_resource_list(&pci->resources);
@@ -60,7 +68,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
res_valid |= !(res->flags & IORESOURCE_PREFETCH);
break;
case IORESOURCE_BUS:
- pci->cfg.bus_range = res;
+ pci->bus_range = res;
default:
continue;
}
@@ -83,50 +91,44 @@ out_release_res:
return err;
}

+static void gen_pci_generic_unmap_cfg(void *ptr)
+{
+ pci_generic_unmap_config((struct pci_config_window *)ptr);
+}
+
static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
{
int err;
- u8 bus_max;
- resource_size_t busn;
- struct resource *bus_range;
+ struct resource *cfgres = &pci->cfgres;
+ struct resource *bus_range = pci->bus_range;
+ unsigned int bus_shift = pci->bus_shift;
struct device *dev = pci->host.dev.parent;
struct device_node *np = dev->of_node;
- u32 sz = 1 << pci->cfg.ops->bus_shift;
+ struct pci_config_window *cfg;

- err = of_address_to_resource(np, 0, &pci->cfg.res);
+ err = of_address_to_resource(np, 0, cfgres);
if (err) {
dev_err(dev, "missing \"reg\" property\n");
return err;
}

/* Limit the bus-range to fit within reg */
- bus_max = pci->cfg.bus_range->start +
- (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
- pci->cfg.bus_range->end = min_t(resource_size_t,
- pci->cfg.bus_range->end, bus_max);
-
- pci->cfg.win = devm_kcalloc(dev, resource_size(pci->cfg.bus_range),
- sizeof(*pci->cfg.win), GFP_KERNEL);
- if (!pci->cfg.win)
- return -ENOMEM;
-
- /* Map our Configuration Space windows */
- if (!devm_request_mem_region(dev, pci->cfg.res.start,
- resource_size(&pci->cfg.res),
- "Configuration Space"))
- return -ENOMEM;
-
- bus_range = pci->cfg.bus_range;
- for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
- u32 idx = busn - bus_range->start;
-
- pci->cfg.win[idx] = devm_ioremap(dev,
- pci->cfg.res.start + idx * sz,
- sz);
- if (!pci->cfg.win[idx])
- return -ENOMEM;
+ bus_range->end = min(bus_range->end,
+ bus_range->start + (resource_size(cfgres) >> bus_shift) - 1);
+
+ cfg = pci_generic_map_config(cfgres->start, bus_range->start,
+ bus_range->end, bus_shift, bus_shift - 8);
+
+ if (IS_ERR(cfg))
+ return PTR_ERR(cfg);
+
+ err = devm_add_action(dev, gen_pci_generic_unmap_cfg, cfg);
+ if (err) {
+ gen_pci_generic_unmap_cfg(cfg);
+ return err;
}

+ pci->cfg = cfg;
return 0;
}

@@ -168,8 +170,8 @@ int pci_host_common_probe(struct platform_device *pdev,
pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);


- bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
- &pci->cfg.ops->ops, pci, &pci->resources);
+ bus = pci_scan_root_bus(dev, pci->bus_range->start,
+ pci->ops, pci, &pci->resources);
if (!bus) {
dev_err(dev, "Scanning rootbus failed");
return -ENODEV;
diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h
index 09f3fa0..4eb2ff0 100644
--- a/drivers/pci/host/pci-host-common.h
+++ b/drivers/pci/host/pci-host-common.h
@@ -22,26 +22,19 @@
#include <linux/kernel.h>
#include <linux/platform_device.h>

-struct gen_pci_cfg_bus_ops {
- u32 bus_shift;
- struct pci_ops ops;
-};
-
-struct gen_pci_cfg_windows {
- struct resource res;
- struct resource *bus_range;
- void __iomem **win;
-
- struct gen_pci_cfg_bus_ops *ops;
-};
-
struct gen_pci {
- struct pci_host_bridge host;
- struct gen_pci_cfg_windows cfg;
- struct list_head resources;
+ struct pci_host_bridge host;
+ struct resource *bus_range;
+ unsigned int bus_shift;
+ struct resource cfgres;
+ struct pci_config_window *cfg;
+ struct pci_ops *ops;
+ struct list_head resources;
};

int pci_host_common_probe(struct platform_device *pdev,
struct gen_pci *pci);
+void __iomem *gen_pci_map_cfg_bus(struct pci_bus *bus,
+ unsigned int devfn, int where);

#endif /* _PCI_HOST_COMMON_H */
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index e8aa78f..7b6cee1 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -27,51 +27,15 @@

#include "pci-host-common.h"

-static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 8) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
- .bus_shift = 16,
- .ops = {
- .map_bus = gen_pci_map_cfg_bus_cam,
- .read = pci_generic_config_read,
- .write = pci_generic_config_write,
- }
-};
-
-static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
- .bus_shift = 20,
- .ops = {
- .map_bus = gen_pci_map_cfg_bus_ecam,
- .read = pci_generic_config_read,
- .write = pci_generic_config_write,
- }
+static struct pci_ops gen_pci_ops = {
+ .map_bus = gen_pci_map_cfg_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
};

static const struct of_device_id gen_pci_of_match[] = {
- { .compatible = "pci-host-cam-generic",
- .data = &gen_pci_cfg_cam_bus_ops },
-
- { .compatible = "pci-host-ecam-generic",
- .data = &gen_pci_cfg_ecam_bus_ops },
-
+ { .compatible = "pci-host-cam-generic", .data = (void *)16},
+ { .compatible = "pci-host-ecam-generic", .data = (void *)20},
{ },
};
MODULE_DEVICE_TABLE(of, gen_pci_of_match);
@@ -86,7 +50,8 @@ static int gen_pci_probe(struct platform_device *pdev)
return -ENOMEM;

of_id = of_match_node(gen_pci_of_match, dev->of_node);
- pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+ pci->bus_shift = (unsigned long)of_id->data;
+ pci->ops = &gen_pci_ops;

return pci_host_common_probe(pdev, pci);
}
diff --git a/drivers/pci/host/pci-thunder-ecam.c b/drivers/pci/host/pci-thunder-ecam.c
index d71935cb..34714df 100644
--- a/drivers/pci/host/pci-thunder-ecam.c
+++ b/drivers/pci/host/pci-thunder-ecam.c
@@ -15,17 +15,6 @@

#include "pci-host-common.h"

-/* Mapping is standard ECAM */
-static void __iomem *thunder_ecam_map_bus(struct pci_bus *bus,
- unsigned int devfn,
- int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
-
static void set_val(u32 v, int where, int size, u32 *val)
{
int shift = (where & 3) * 8;
@@ -129,7 +118,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
* the config space access window. Since we are working with
* the high-order 32 bits, shift everything down by 32 bits.
*/
- node_bits = (pci->cfg.res.start >> 32) & (1 << 12);
+ node_bits = (pci->cfgres.start >> 32) & (1 << 12);

v |= node_bits;
set_val(v, where, size, val);
@@ -358,19 +347,14 @@ static int thunder_ecam_config_write(struct pci_bus *bus, unsigned int devfn,
return pci_generic_config_write(bus, devfn, where, size, val);
}

-static struct gen_pci_cfg_bus_ops thunder_ecam_bus_ops = {
- .bus_shift = 20,
- .ops = {
- .map_bus = thunder_ecam_map_bus,
- .read = thunder_ecam_config_read,
- .write = thunder_ecam_config_write,
- }
+static struct pci_ops thunder_ecam_pci_ops = {
+ .map_bus = gen_pci_map_cfg_bus,
+ .read = thunder_ecam_config_read,
+ .write = thunder_ecam_config_write,
};

static const struct of_device_id thunder_ecam_of_match[] = {
- { .compatible = "cavium,pci-host-thunder-ecam",
- .data = &thunder_ecam_bus_ops },
-
+ { .compatible = "cavium,pci-host-thunder-ecam" },
{ },
};
MODULE_DEVICE_TABLE(of, thunder_ecam_of_match);
@@ -378,14 +362,13 @@ MODULE_DEVICE_TABLE(of, thunder_ecam_of_match);
static int thunder_ecam_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- const struct of_device_id *of_id;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);

if (!pci)
return -ENOMEM;

- of_id = of_match_node(thunder_ecam_of_match, dev->of_node);
- pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+ pci->ops = &thunder_ecam_pci_ops;
+ pci->bus_shift = 20;

return pci_host_common_probe(pdev, pci);
}
diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index cabb92a..eb248c1 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -31,15 +31,6 @@ struct thunder_pem_pci {
void __iomem *pem_reg_base;
};

-static void __iomem *thunder_pem_map_bus(struct pci_bus *bus,
- unsigned int devfn, int where)
-{
- struct gen_pci *pci = bus->sysdata;
- resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
- return pci->cfg.win[idx] + ((devfn << 16) | where);
-}
-
static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
@@ -134,15 +125,15 @@ static int thunder_pem_config_read(struct pci_bus *bus, unsigned int devfn,
{
struct gen_pci *pci = bus->sysdata;

- if (bus->number < pci->cfg.bus_range->start ||
- bus->number > pci->cfg.bus_range->end)
+ if (bus->number < pci->bus_range->start ||
+ bus->number > pci->bus_range->end)
return PCIBIOS_DEVICE_NOT_FOUND;

/*
* The first device on the bus is the PEM PCIe bridge.
* Special case its config access.
*/
- if (bus->number == pci->cfg.bus_range->start)
+ if (bus->number == pci->bus_range->start)
return thunder_pem_bridge_read(bus, devfn, where, size, val);

return pci_generic_config_read(bus, devfn, where, size, val);
@@ -258,33 +249,28 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
{
struct gen_pci *pci = bus->sysdata;

- if (bus->number < pci->cfg.bus_range->start ||
- bus->number > pci->cfg.bus_range->end)
+ if (bus->number < pci->bus_range->start ||
+ bus->number > pci->bus_range->end)
return PCIBIOS_DEVICE_NOT_FOUND;
/*
* The first device on the bus is the PEM PCIe bridge.
* Special case its config access.
*/
- if (bus->number == pci->cfg.bus_range->start)
+ if (bus->number == pci->bus_range->start)
return thunder_pem_bridge_write(bus, devfn, where, size, val);


return pci_generic_config_write(bus, devfn, where, size, val);
}

-static struct gen_pci_cfg_bus_ops thunder_pem_bus_ops = {
- .bus_shift = 24,
- .ops = {
- .map_bus = thunder_pem_map_bus,
- .read = thunder_pem_config_read,
- .write = thunder_pem_config_write,
- }
+static struct pci_ops thunder_pem_pci_ops = {
+ .map_bus = gen_pci_map_cfg_bus,
+ .read = thunder_pem_config_read,
+ .write = thunder_pem_config_write,
};

static const struct of_device_id thunder_pem_of_match[] = {
- { .compatible = "cavium,pci-host-thunder-pem",
- .data = &thunder_pem_bus_ops },
-
+ { .compatible = "cavium,pci-host-thunder-pem" },
{ },
};
MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
@@ -292,7 +278,6 @@ MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
static int thunder_pem_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- const struct of_device_id *of_id;
resource_size_t bar4_start;
struct resource *res_pem;
struct thunder_pem_pci *pem_pci;
@@ -301,8 +286,8 @@ static int thunder_pem_probe(struct platform_device *pdev)
if (!pem_pci)
return -ENOMEM;

- of_id = of_match_node(thunder_pem_of_match, dev->of_node);
- pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+ pem_pci->gen_pci.ops = &thunder_pem_pci_ops;
+ pem_pci->gen_pci.bus_shift = 24;

/*
* The second register range is the PEM bridge to the PCIe
--
1.9.1

2016-03-17 20:34:50

by Jayachandran C

[permalink] [raw]
Subject: [RFC PATCH 0/4] ACPI based PCI host driver with generic ECAM

Hi Bjorn,

Here is a new patchset for the ACPI PCI controller driver based on the
earlier discussion[1].

The first two patches in the patchset implements pci/ecam.c for generic
config space access and uses it in pci-host-generic.c and related files.

The third patch implements the ACPI PCI host driver using the same ecam
access functions. The fourth patch adds the implementation of raw
operations.

I have not used the pci_mmcfg_list or the region definitions from x86,
but have used a much simpler approach here.

This should apply cleanly on top of the current pci next tree, and
can be reviewed as a patchset. To use it on ARM64, we need to pull
in about 7 patches more from Tomasz patchset that fixes various
issues (like stub code in arm64 pci.c, ACPI companion setup,
domain number assignment, IO resources fixup etc.).

If you are okay with this approach, I will work with Tomasz and
post the full patchset.

This has been tested on qemu with OVMF for the ACPI part and with
device tree for pci-host-generic code.

Thanks,
JC.

[1] https://lkml.org/lkml/2016/3/3/921

Jayachandran C (4):
PCI: Provide generic ECAM mapping functions
PCI: generic,thunder: Use generic config functions
ACPI: PCI: Add generic PCI host controller
ACPI: PCI: Add raw_pci_read/write operations

drivers/acpi/Kconfig | 9 +
drivers/acpi/Makefile | 1 +
drivers/acpi/pci_gen_host.c | 334 ++++++++++++++++++++++++++++++++++++
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 2 +
drivers/pci/ecam.c | 127 ++++++++++++++
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 68 ++++----
drivers/pci/host/pci-host-common.h | 25 +--
drivers/pci/host/pci-host-generic.c | 51 +-----
drivers/pci/host/pci-thunder-ecam.c | 33 +---
drivers/pci/host/pci-thunder-pem.c | 41 ++---
include/linux/pci.h | 10 ++
13 files changed, 560 insertions(+), 145 deletions(-)
create mode 100644 drivers/acpi/pci_gen_host.c
create mode 100644 drivers/pci/ecam.c

--
1.9.1

2016-03-18 17:48:23

by Jayachandran C

[permalink] [raw]
Subject: Re: [RFC PATCH 0/4] ACPI based PCI host driver with generic ECAM

On Fri, Mar 18, 2016 at 1:48 AM, Jayachandran C <[email protected]> wrote:
> Hi Bjorn,
>
> Here is a new patchset for the ACPI PCI controller driver based on the
> earlier discussion[1].
>
> The first two patches in the patchset implements pci/ecam.c for generic
> config space access and uses it in pci-host-generic.c and related files.
>
> The third patch implements the ACPI PCI host driver using the same ecam
> access functions. The fourth patch adds the implementation of raw
> operations.
>
> I have not used the pci_mmcfg_list or the region definitions from x86,
> but have used a much simpler approach here.
>
> This should apply cleanly on top of the current pci next tree, and
> can be reviewed as a patchset. To use it on ARM64, we need to pull
> in about 7 patches more from Tomasz patchset that fixes various
> issues (like stub code in arm64 pci.c, ACPI companion setup,
> domain number assignment, IO resources fixup etc.).
>
> If you are okay with this approach, I will work with Tomasz and
> post the full patchset.
>
> This has been tested on qemu with OVMF for the ACPI part and with
> device tree for pci-host-generic code.

The full patchset is available at https://github.com/jchandra-brcm/linux.git on
branch arm64-acpi-pci, if anyone wants to try it.

Comments, suggestions and testing would be welcome.

Thanks,
JC.

2016-03-23 10:24:05

by Gabriele Paoloni

[permalink] [raw]
Subject: RE: [RFC PATCH 0/4] ACPI based PCI host driver with generic ECAM

Hi Jayachandran

> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Jayachandran C
> Sent: 18 March 2016 17:48
> To: Bjorn Helgaas; Tomasz Nowicki; [email protected]
> Cc: Jayachandran C; Arnd Bergmann; Will Deacon; Catalin Marinas; Hanjun
> Guo; Lorenzo Pieralisi; [email protected];
> [email protected]; Stefano Stabellini;
> [email protected]; Marcin Wojtas; [email protected];
> David Daney; Wangyijing; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Jon Masters
> Subject: Re: [RFC PATCH 0/4] ACPI based PCI host driver with generic
> ECAM
>
> On Fri, Mar 18, 2016 at 1:48 AM, Jayachandran C <[email protected]>
> wrote:
> > Hi Bjorn,
> >
> > Here is a new patchset for the ACPI PCI controller driver based on
> the
> > earlier discussion[1].
> >
> > The first two patches in the patchset implements pci/ecam.c for
> generic
> > config space access and uses it in pci-host-generic.c and related
> files.
> >
> > The third patch implements the ACPI PCI host driver using the same
> ecam
> > access functions. The fourth patch adds the implementation of raw
> > operations.
> >
> > I have not used the pci_mmcfg_list or the region definitions from
> x86,
> > but have used a much simpler approach here.
> >
> > This should apply cleanly on top of the current pci next tree, and
> > can be reviewed as a patchset. To use it on ARM64, we need to pull
> > in about 7 patches more from Tomasz patchset that fixes various
> > issues (like stub code in arm64 pci.c, ACPI companion setup,
> > domain number assignment, IO resources fixup etc.).
> >
> > If you are okay with this approach, I will work with Tomasz and
> > post the full patchset.
> >
> > This has been tested on qemu with OVMF for the ACPI part and with
> > device tree for pci-host-generic code.
>
> The full patchset is available at https://github.com/jchandra-
> brcm/linux.git on
> branch arm64-acpi-pci, if anyone wants to try it.

I had a look at your patchset and also in your git repo at the other
patches that you ported over from Tomasz; it seems that now we miss
a quirk mechanism to enable controller that are not fully ECAM.

This was provided before by Tomasz in:
https://lkml.org/lkml/2016/2/16/410

I think we should put something like that back in...

Thanks

Gab

>
> Comments, suggestions and testing would be welcome.
>
> Thanks,
> JC.

2016-03-28 13:42:17

by Sinan Kaya

[permalink] [raw]
Subject: Re: [RFC PATCH 0/4] ACPI based PCI host driver with generic ECAM

Hi,

On 3/23/2016 6:22 AM, Gabriele Paoloni wrote:
> I had a look at your patchset and also in your git repo at the other
> patches that you ported over from Tomasz; it seems that now we miss
> a quirk mechanism to enable controller that are not fully ECAM.
>
> This was provided before by Tomasz in:
> https://lkml.org/lkml/2016/2/16/410
>
> I think we should put something like that back in...
>
> Thanks
>
> Gab

I was requested to test your patchset. I'll need this mechanism before
I can start.

Sinan

--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

2016-03-28 18:02:20

by Jayachandran C

[permalink] [raw]
Subject: Re: [RFC PATCH 0/4] ACPI based PCI host driver with generic ECAM

On Mon, Mar 28, 2016 at 7:12 PM, Sinan Kaya <[email protected]> wrote:
> Hi,
>
> On 3/23/2016 6:22 AM, Gabriele Paoloni wrote:
>> I had a look at your patchset and also in your git repo at the other
>> patches that you ported over from Tomasz; it seems that now we miss
>> a quirk mechanism to enable controller that are not fully ECAM.
>>
>> This was provided before by Tomasz in:
>> https://lkml.org/lkml/2016/2/16/410
>>
>> I think we should put something like that back in...

Like Tomasz mentioned in his mail, his approach does not work
for raw operations. I have added raw operations in may patchset,
so we have to come up with a new approach or decide that raw
operations can be dropped.

I am waiting for the overall acceptance of the patch set before
going further along this path

>>
>> Thanks
>>
>> Gab
>
> I was requested to test your patchset. I'll need this mechanism before
> I can start.

Please see above, we will need to look at the quirks again.

> Sinan

JC.

2016-04-05 14:12:18

by Tomasz Nowicki

[permalink] [raw]
Subject: 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 <[email protected]>
>>> wrote:
>>>> Hi Tomasz, Jayachandran, et al,
>>>>
>>>> On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
>>>>> From: Jayachandran C <[email protected]>
>>>>>
>>>>> 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

2016-04-05 16:41:37

by Bjorn Helgaas

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

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 <[email protected]>
> >>>wrote:
> >>>>Hi Tomasz, Jayachandran, et al,
> >>>>
> >>>>On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:
> >>>>>From: Jayachandran C <[email protected]>
> >>>>>
> >>>>>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

2016-04-05 18:08:09

by Tomasz Nowicki

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

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

2016-04-05 18:51:16

by Jayachandran C

[permalink] [raw]
Subject: 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 <[email protected]> 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.