Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

Hanjun Guo
2016-06-06T07:54:29Z

On 2016/6/6 15:27, Gabriele Paoloni wrote:
> Hi Jeffrey
>> On 6/3/2016 9:32 AM, Gabriele Paoloni wrote:
>>> Hi Cov
>>>
>>>> Hi Tomasz,
>>>>
>>>> Thanks for your work on this.
>>>>
>>>> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
>>>>> Some platforms may not be fully compliant with generic set of PCI
>>>> config
>>>>> accessors. For these cases we implement the way to overwrite
>>>> accessors
>>>>> set. Algorithm traverses available quirk list, matches against
>>>>> <oem_id, oem_rev, domain, bus number> tuple and returns
>> corresponding
>>>>> PCI config ops. oem_id and oem_rev come from MCFG table standard
>>>> header.
>>>>> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
>>>>> kept self contained. Example:
>>>>>
>>>>> /* Custom PCI config ops */
>>>>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>>>> .bus_shift = 24,
>>>>> .pci_ops = {
>>>>> .map_bus = pci_ecam_map_bus,
>>>>> .read = foo_ecam_config_read,
>>>>> .write = foo_ecam_config_write,
>>>>> }
>>>>> };
>>>>>
>>>>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>,
>>>> <domain_nr>, <bus_nr>);
>>>>>
>>>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>>>> ---
>>>>> drivers/acpi/pci_mcfg.c | 32
>>>> ++++++++++++++++++++++++++++++++
>>>>> include/asm-generic/vmlinux.lds.h | 7 +++++++
>>>>> include/linux/pci-acpi.h | 19 +++++++++++++++++++
>>>>> 3 files changed, 58 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>>> index 1847f74..f3d4570 100644
>>>>> --- a/drivers/acpi/pci_mcfg.c
>>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>>> @@ -22,11 +22,43 @@
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/pci.h>
>>>>> #include <linux/pci-acpi.h>
>>>>> +#include <linux/pci-ecam.h>
>>>>>
>>>>> /* Root pointer to the mapped MCFG table */
>>>>> static struct acpi_table_mcfg *mcfg_table;
>>>>> static int mcfg_entries;
>>>>>
>>>>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>>>>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>>>>> +
>>>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>>>> +{
>>>>> + int bus_num = root->secondary.start;
>>>>> + int domain = root->segment;
>>>>> + struct pci_cfg_fixup *f;
>>>>> +
>>>>> + if (!mcfg_table)
>>>>> + return &pci_generic_ecam_ops;
>>>>> +
>>>>> + /*
>>>>> + * Match against platform specific quirks and return
>>>> corresponding
>>>>> + * CAM ops.
>>>>> + *
>>>>> + * First match against PCI topology <domain:bus> then use OEM ID
>>>> and
>>>>> + * OEM revision from MCFG table standard header.
>>>>> + */
>>>>> + for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
>>>> f++) {
>>>>> + if ((f->domain == domain || f->domain ==
>>>> PCI_MCFG_DOMAIN_ANY) &&
>>>>> + (f->bus_num == bus_num || f->bus_num ==
>>>> PCI_MCFG_BUS_ANY) &&
>>>>> + (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>>>> + ACPI_OEM_ID_SIZE)) &&
>>>>> + (f->oem_revision == mcfg_table->header.oem_revision))
>>>>
>>>> Is this more likely to be updated between quirky and fixed platforms
>>>> than oem_table_id? What do folks think about using oem_table_id
>> instead
>>>> of, or in addition to, oem_revision?
>>>
>>> From my understanding we need to stick to this mechanism as
>> (otherwise)
>>> there are platforms out in the field that would need a FW update.
>>>
>>> So I don't think that using oem_table_id "instead" is possible; about
>>> "in addition" I think it is doable, but I do not see the advantage
>> much.
>>> I mean that if a platform gets fixed the oem revision should change
>> too,
>>> Right?
>>
>> Cov and I had a discussion about this, so hopefully I can bring a
>> slightly different perspective that will make sense.
>>
>> We forsee a situation where we have platform A that needs a quirk, and
>> platform B that does not. The OEM id is the same for both platforms as
>> they are different platforms from the same OEM. Using the OEM revision
>> field does not seem to be appropriate since these are different
>> platforms and the revision field appears to be for the purpose of
>> tracking differences within a single platform. Therefore, Cov is
>> proposing using the OEM table id as a mechanism to distinguish platform
>> A (needs quirk applied) vs platform B (no quirks) from the same OEM.
>
> Ah yes I see now...
>
> Probably it should be ok to have a check on all three OEM fields.

Just for reference, x86 and IA64 use oem_id and oem_table_id to make a
difference between different platforms, see
acpi_madt_oem_check(char *oem_id, char *oem_table_id) for x86 and ia64,
that can apply to ARM64 on MCFG too.

Thanks
Hanjun