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

Groups > linux.kernel > #1432591 > unrolled thread

[RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

Back to article view | Back to linux.kernel

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

Contents

  [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Tomasz Nowicki <tn@semihalf.com> - 2016-06-28 10:00 +0200
    Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Christopher Covington <cov@codeaurora.org> - 2016-06-28 15:10 +0200
      Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Duc Dang <dhdang@apm.com> - 2016-06-28 18:20 +0200
        Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Tomasz Nowicki <tn@semihalf.com> - 2016-06-29 12:50 +0200
          Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Christopher Covington <cov@codeaurora.org> - 2016-06-29 15:40 +0200
            Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Tomasz Nowicki <tn@semihalf.com> - 2016-06-29 16:00 +0200
              Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-06-29 16:00 +0200
              Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Jeffrey Hugo <jhugo@codeaurora.org> - 2016-06-29 17:40 +0200
            Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-06-29 16:00 +0200

#1432591 — [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

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 (static array),
matches against <oem_id, oem_table_id, rev, domain, bus number range> and
returns pci_config_window structure with fancy PCI config ops.
oem_id, oem_table_id and rev come from MCFG table standard header.

It is possible to define custom init call which is responsible for
setting up PCI config access accordingly to quirk requirements.
If no custom init call defined, use standard pci_acpi_setup_ecam_mapping().

pci_generic_ecam_ops will be used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 drivers/pci/host/Makefile      |  1 +
 drivers/pci/host/mcfg-quirks.c | 88 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/host/mcfg-quirks.h | 20 ++++++++++
 include/linux/pci-acpi.h       |  2 +
 4 files changed, 111 insertions(+)
 create mode 100644 drivers/pci/host/mcfg-quirks.c
 create mode 100644 drivers/pci/host/mcfg-quirks.h

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 5bc0af2..e3d55a0 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
+obj-$(CONFIG_ACPI_MCFG) += mcfg-quirks.o
diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
new file mode 100644
index 0000000..fb2b184
--- /dev/null
+++ b/drivers/pci/host/mcfg-quirks.c
@@ -0,0 +1,88 @@
+/*
+ * Copyright (C) 2016 Semihalf
+ *	Author: Tomasz Nowicki <tn@semihalf.com>
+ *
+ * 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/ioport.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+#include "mcfg-quirks.h"
+
+struct pci_cfg_fixup {
+	char *oem_id;
+	char *oem_table_id;
+	u32 oem_revision;
+	struct resource domain_range;
+	struct resource bus_range;
+	struct pci_ops *ops;
+	struct pci_config_window *(*init)(struct acpi_pci_root *root,
+					  struct pci_ops *ops);
+};
+
+#define MCFG_DOM_RANGE(start, end)	DEFINE_RES_NAMED((start),	\
+						((end) - (start) + 1), NULL, 0)
+#define MCFG_DOM_ANY			MCFG_DOM_RANGE(0x0, 0xffff)
+#define MCFG_BUS_RANGE(start, end)	DEFINE_RES_NAMED((start),	\
+						((end) - (start) + 1),	\
+						NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY			MCFG_BUS_RANGE(0x0, 0xff)
+
+static struct pci_cfg_fixup mcfg_qurks[] __initconst = {
+/*	{ OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */
+};
+
+static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
+				 struct acpi_table_header *mcfg_header)
+{
+	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
+	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
+
+	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
+		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
+		f->oem_revision == mcfg_header->oem_revision);
+}
+
+struct pci_config_window *pci_mcfg_match_quirks(struct acpi_pci_root *root)
+{
+	struct resource dom_res = MCFG_DOM_RANGE(root->segment, root->segment);
+	struct resource *bus_res = &root->secondary;
+	struct pci_cfg_fixup *f = mcfg_qurks;
+	struct acpi_table_header *mcfg_header;
+	acpi_status status;
+	int i;
+
+	status = acpi_get_table(ACPI_SIG_MCFG, 0, &mcfg_header);
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	/*
+	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
+	 * table ID, and OEM revision from MCFG table standard header.
+	 */
+	for (i = 0; i < ARRAY_SIZE(mcfg_qurks); i++, f++) {
+		if (resource_contains(&f->domain_range, &dom_res) &&
+		    resource_contains(&f->bus_range, bus_res) &&
+		    pci_mcfg_fixup_match(f, mcfg_header)) {
+			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
+				f->oem_id, f->oem_table_id, f->oem_revision);
+			return f->init ? f->init(root, f->ops) :
+				pci_acpi_setup_ecam_mapping(root, f->ops);
+		}
+	}
+	return pci_acpi_setup_ecam_mapping(root, &pci_generic_ecam_ops.pci_ops);
+}
diff --git a/drivers/pci/host/mcfg-quirks.h b/drivers/pci/host/mcfg-quirks.h
new file mode 100644
index 0000000..45cbd16
--- /dev/null
+++ b/drivers/pci/host/mcfg-quirks.h
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+
+#ifndef __MCFG_QUIRKS_H__
+#define __MCFG_QUIRKS_H__
+
+/* MCFG quirks initialize call list */
+
+#endif /* __MCFG_QUIRKS_H__ */
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index e9bfe00..28cdce4 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,8 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
 extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+extern struct pci_config_window *
+pci_mcfg_match_quirks(struct acpi_pci_root *root);
 
 extern struct pci_config_window *
 pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root, struct pci_ops *ops);
-- 
1.9.1

[toc] | [next] | [standalone]

#1432978

Hi Tomasz,

On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:

> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
> new file mode 100644
> index 0000000..fb2b184
> --- /dev/null
> +++ b/drivers/pci/host/mcfg-quirks.c
> @@ -0,0 +1,88 @@

> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *mcfg_header)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
> +		f->oem_revision == mcfg_header->oem_revision);
> +}

Ard's comments on v3 included:

"... exact OEM table/rev id matches ..."
"... substring match ... out of the question ..."

I originally advocated the substring match approach because
space-padding the input strings was unfamiliar. But given that some
vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
former needs quirks and the latter (hopefully) doesn't, I agree with Ard
and think space-padded inputs is the better way to go. Sorry for the
lack of foresight.

(I'm happy to rip it out, test, and communicate the delta however you'd
prefer--just let me know.)

Regards,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

#1433093

On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Tomasz,
>
> On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:
>
>> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
>> new file mode 100644
>> index 0000000..fb2b184
>> --- /dev/null
>> +++ b/drivers/pci/host/mcfg-quirks.c
>> @@ -0,0 +1,88 @@
>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *mcfg_header)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
>> +             f->oem_revision == mcfg_header->oem_revision);
>> +}
>
> Ard's comments on v3 included:
>
> "... exact OEM table/rev id matches ..."
> "... substring match ... out of the question ..."
>
> I originally advocated the substring match approach because
> space-padding the input strings was unfamiliar. But given that some
> vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
> former needs quirks and the latter (hopefully) doesn't, I agree with Ard
> and think space-padded inputs is the better way to go. Sorry for the
> lack of foresight.

I think having OEM Table ID as "PLAT   " and then "PLAT2  " (the the
next version of the SoC)
is common. So yes, matching full string is better as we can use "PLAT2  "
in MCFG table and not worry about the "PLAT" sub-string match causes the quirk
to be applied unintentionally.

>
> (I'm happy to rip it out, test, and communicate the delta however you'd
> prefer--just let me know.)
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Regards,
Duc Dang.

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

#1433666

On 28.06.2016 18:12, Duc Dang wrote:
> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> Hi Tomasz,
>>
>> On 06/28/2016 03:54 AM, Tomasz Nowicki wrote:
>>
>>> diff --git a/drivers/pci/host/mcfg-quirks.c b/drivers/pci/host/mcfg-quirks.c
>>> new file mode 100644
>>> index 0000000..fb2b184
>>> --- /dev/null
>>> +++ b/drivers/pci/host/mcfg-quirks.c
>>> @@ -0,0 +1,88 @@
>>
>>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>>> +                              struct acpi_table_header *mcfg_header)
>>> +{
>>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>>> +
>>> +     return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
>>> +             !strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
>>> +             f->oem_revision == mcfg_header->oem_revision);
>>> +}
>>
>> Ard's comments on v3 included:
>>
>> "... exact OEM table/rev id matches ..."
>> "... substring match ... out of the question ..."
>>
>> I originally advocated the substring match approach because
>> space-padding the input strings was unfamiliar. But given that some
>> vendors have a "PLAT    " then "PLAT2   " naming scheme, where the
>> former needs quirks and the latter (hopefully) doesn't, I agree with Ard
>> and think space-padded inputs is the better way to go. Sorry for the
>> lack of foresight.
>
> I think having OEM Table ID as "PLAT   " and then "PLAT2  " (the the
> next version of the SoC)
> is common. So yes, matching full string is better as we can use "PLAT2  "
> in MCFG table and not worry about the "PLAT" sub-string match causes the quirk
> to be applied unintentionally.
>

Sorry, I forgot to address Ard's comment.

Note that platforms already shipped where OEM string has no padding will 
have change the firmware or add 0 padding to our quirk array IDs.
So how about:

static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
				 struct acpi_table_header *mcfg_header)
{
	int olen = strnlen(mcfg_header->oem_id, ACPI_OEM_ID_SIZE);
	int tlen = strnlen(mcfg_header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);

	if (olen != strlen(f->oem_id) || tlen != strlen(f->oem_table_id))
		return false;

	return (!strncmp(f->oem_id, mcfg_header->oem_id, olen) &&
		!strncmp(f->oem_table_id, mcfg_header->oem_table_id, tlen) &&
		f->oem_revision == mcfg_header->oem_revision);
}

This should work for all cases:
1. "PLAT"
2. "PLAT    " padding
3. No need to change existing firmware

Tomasz

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

#1433746

Hi Tomasz,

On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
> On 28.06.2016 18:12, Duc Dang wrote:
>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>>> Hi Tomasz,

>>> Ard's comments on v3 included:
>>>
>>> "... exact OEM table/rev id matches ..."
>>> "... substring match ... out of the question ..."

Digging through the archives I see Jon Master commented earlier to "be
careful with substring match".

>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the 
>> next version of the SoC) is common. So yes, matching full string is
>> better as we can use "PLAT2 " in MCFG table and not worry about the
>> "PLAT" sub-string match causes the quirk to be applied
>> unintentionally.

> Note that platforms already shipped where OEM string has no padding will

I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
Table ID as 8 bytes long in the ACPI specification. As far as I can
tell, if your string isn't exactly that long, padding up to that length
is required.

> have change the firmware or add 0 padding to our quirk array IDs.

The fixed 6 or 8 character string compare, as used v2 of this patchset,
will be compatible with existing firmware as best I can tell. Adding
padding to the quirk array IDs is exactly what I'm suggesting, although
all the strings I've seen are space padded rather than null padded.

Matches:
{"APM   ", "XGENE   ", 1}
{"CAVIUM", "THUNDERX", 1}
{"HISI  ", "HISI-D02", 1}
{"HISI  ", "HISI-D03", 1}
{"QCOM  ", "QDF2432 ", 1}

Given the above tuples, won't accidentally match:
(guessing at possible future ids)
{"APM   ", "XGENEi  ", 1}
{"CAVIUM", "THUNDERX", i} i != 1
{"CAVIUM", "THUNDERi", 1}
{"CAVIUM", "THUNDRXi", 1}
{"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
{"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2

References for APM, HiSilicon IDs:
https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

#1433758

On 29.06.2016 15:34, Christopher Covington wrote:
> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
> Table ID as 8 bytes long in the ACPI specification. As far as I can
> tell, if your string isn't exactly that long, padding up to that length
> is required.

Well, I cannot find that requirement in ACPI spec. but I might missed 
something.

I dumped my x86 machine ACPI tables and here is an example of MCFG:

$ cat mcfg.dsl
/*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20160108-64
  * Copyright (c) 2000 - 2016 Intel Corporation
  *
  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
  *
  * ACPI Data Table [MCFG]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

[000h 0000   4]                    Signature : "MCFG"    [Memory Mapped 
Configuration table]
[004h 0004   4]                 Table Length : 0000003C
[008h 0008   1]                     Revision : 01
[009h 0009   1]                     Checksum : A9
[00Ah 0010   6]                       Oem ID : "ALASKA"
[010h 0016   8]                 Oem Table ID : "A M I"
[018h 0024   4]                 Oem Revision : 01072009
[01Ch 0028   4]              Asl Compiler ID : "MSFT"
[020h 0032   4]        Asl Compiler Revision : 00000097

[024h 0036   8]                     Reserved : 0000000000000000

[02Ch 0044   8]                 Base Address : 00000000F8000000
[034h 0052   2]         Segment Group Number : 0000
[036h 0054   1]             Start Bus Number : 00
[037h 0055   1]               End Bus Number : 3F
[038h 0056   4]                     Reserved : 00000000

Raw Table Data: Length 60 (0x3C)

00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41 
|MCFG<.....ALASKA|
00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I.... 
..MSFT|
00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8 
|................|
00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|

So in this example I have OEM table ID "A M I" 6 character long and 0 
padding.

Tomasz

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

#1433760

On 29 June 2016 at 15:52, Tomasz Nowicki <tn@semihalf.com> wrote:
> On 29.06.2016 15:34, Christopher Covington wrote:
>>
>> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> Table ID as 8 bytes long in the ACPI specification. As far as I can
>> tell, if your string isn't exactly that long, padding up to that length
>> is required.
>
>
> Well, I cannot find that requirement in ACPI spec. but I might missed
> something.
>

This has *nothing* to do with the ACPI spec.

There exist a couple of machines that have known quirks. Those
machines have certain 6 or 8 byte long strings in their MCFG tables
that we can match against. If any of those fields contains spaces,
whether you call that padding or not is totally irrelevant. The same
applies to NUL bytes.

So for a 6 byte field, our table contains the six characters that we
know is in the quirky hardware's table.
The same for an 8 byte field.

That means there is no need for strnlen, strncmp() with variable size
etc. Each match can be implemented using memcmp() [since the table
fields are not NUL terminated] with a constant argument for the length
parameter.

Regards,
Ard.


> I dumped my x86 machine ACPI tables and here is an example of MCFG:
>
> $ cat mcfg.dsl
> /*
>  * Intel ACPI Component Architecture
>  * AML/ASL+ Disassembler version 20160108-64
>  * Copyright (c) 2000 - 2016 Intel Corporation
>  *
>  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
>  *
>  * ACPI Data Table [MCFG]
>  *
>  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
>  */
>
> [000h 0000   4]                    Signature : "MCFG"    [Memory Mapped
> Configuration table]
> [004h 0004   4]                 Table Length : 0000003C
> [008h 0008   1]                     Revision : 01
> [009h 0009   1]                     Checksum : A9
> [00Ah 0010   6]                       Oem ID : "ALASKA"
> [010h 0016   8]                 Oem Table ID : "A M I"
> [018h 0024   4]                 Oem Revision : 01072009
> [01Ch 0028   4]              Asl Compiler ID : "MSFT"
> [020h 0032   4]        Asl Compiler Revision : 00000097
>
> [024h 0036   8]                     Reserved : 0000000000000000
>
> [02Ch 0044   8]                 Base Address : 00000000F8000000
> [034h 0052   2]         Segment Group Number : 0000
> [036h 0054   1]             Start Bus Number : 00
> [037h 0055   1]               End Bus Number : 3F
> [038h 0056   4]                     Reserved : 00000000
>
> Raw Table Data: Length 60 (0x3C)
>
> 00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41
> |MCFG<.....ALASKA|
> 00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I....
> ..MSFT|
> 00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8
> |................|
> 00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|
>
> So in this example I have OEM table ID "A M I" 6 character long and 0
> padding.
>
> Tomasz
>

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

#1433860

On 6/29/2016 7:52 AM, Tomasz Nowicki wrote:
> On 29.06.2016 15:34, Christopher Covington wrote:
>> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> Table ID as 8 bytes long in the ACPI specification. As far as I can
>> tell, if your string isn't exactly that long, padding up to that length
>> is required.
>
> Well, I cannot find that requirement in ACPI spec. but I might missed
> something.

Have a look at table 5-29 in Section 5.2.6 which starts on page 114 of 
ACPI 6.1

>
> I dumped my x86 machine ACPI tables and here is an example of MCFG:
>
> $ cat mcfg.dsl
> /*
>  * Intel ACPI Component Architecture
>  * AML/ASL+ Disassembler version 20160108-64
>  * Copyright (c) 2000 - 2016 Intel Corporation
>  *
>  * Disassembly of mcfg.dat, Wed Jun 29 15:48:16 2016
>  *
>  * ACPI Data Table [MCFG]
>  *
>  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
>  */
>
> [000h 0000   4]                    Signature : "MCFG"    [Memory Mapped
> Configuration table]
> [004h 0004   4]                 Table Length : 0000003C
> [008h 0008   1]                     Revision : 01
> [009h 0009   1]                     Checksum : A9
> [00Ah 0010   6]                       Oem ID : "ALASKA"
> [010h 0016   8]                 Oem Table ID : "A M I"
> [018h 0024   4]                 Oem Revision : 01072009
> [01Ch 0028   4]              Asl Compiler ID : "MSFT"
> [020h 0032   4]        Asl Compiler Revision : 00000097
>
> [024h 0036   8]                     Reserved : 0000000000000000
>
> [02Ch 0044   8]                 Base Address : 00000000F8000000
> [034h 0052   2]         Segment Group Number : 0000
> [036h 0054   1]             Start Bus Number : 00
> [037h 0055   1]               End Bus Number : 3F
> [038h 0056   4]                     Reserved : 00000000
>
> Raw Table Data: Length 60 (0x3C)
>
> 00000000  4d 43 46 47 3c 00 00 00  01 a9 41 4c 41 53 4b 41
> |MCFG<.....ALASKA|
> 00000010  41 20 4d 20 49 00 00 00  09 20 07 01 4d 53 46 54  |A M I....
> ..MSFT|
> 00000020  97 00 00 00 00 00 00 00  00 00 00 00 00 00 00 f8
> |................|
> 00000030  00 00 00 00 00 00 00 3f  00 00 00 00              |.......?....|
>
> So in this example I have OEM table ID "A M I" 6 character long and 0
> padding.
>
> Tomasz
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

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

#1433762

On 29 June 2016 at 15:34, Christopher Covington <cov@codeaurora.org> wrote:
> Hi Tomasz,
>
> On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
>> On 28.06.2016 18:12, Duc Dang wrote:
>>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>>> <cov@codeaurora.org> wrote:
>>>> Hi Tomasz,
>
>>>> Ard's comments on v3 included:
>>>>
>>>> "... exact OEM table/rev id matches ..."
>>>> "... substring match ... out of the question ..."
>
> Digging through the archives I see Jon Master commented earlier to "be
> careful with substring match".
>
>>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the
>>> next version of the SoC) is common. So yes, matching full string is
>>> better as we can use "PLAT2 " in MCFG table and not worry about the
>>> "PLAT" sub-string match causes the quirk to be applied
>>> unintentionally.
>
>> Note that platforms already shipped where OEM string has no padding will
>
> I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
> Table ID as 8 bytes long in the ACPI specification. As far as I can
> tell, if your string isn't exactly that long, padding up to that length
> is required.
>
>> have change the firmware or add 0 padding to our quirk array IDs.
>
> The fixed 6 or 8 character string compare, as used v2 of this patchset,
> will be compatible with existing firmware as best I can tell. Adding
> padding to the quirk array IDs is exactly what I'm suggesting, although
> all the strings I've seen are space padded rather than null padded.
>

I don't think any interpretation of the 6 or 8 byte wide OEM fields is
necessary to be able to match it against a list of known values as
used by the quirky platforms. We need an exact match against whatever
we know is in the table of an affected system, and whether a space
qualifies as padding or as a character is irrelevant.

> Matches:
> {"APM   ", "XGENE   ", 1}
> {"CAVIUM", "THUNDERX", 1}
> {"HISI  ", "HISI-D02", 1}
> {"HISI  ", "HISI-D03", 1}
> {"QCOM  ", "QDF2432 ", 1}
>

I would not mind listing these as

{ { 'A','P','M',' ',' ',' ',' '}, {'X','G','E','N','E',' ',' ',' '}, 1}
...

just to stress that we are not dealing with C strings (and to avoid
having to deal with the implicit NUL terminator).
That also means memcmp() with a fixed length is the most appropriate
to perform the comparison

> Given the above tuples, won't accidentally match:
> (guessing at possible future ids)
> {"APM   ", "XGENEi  ", 1}
> {"CAVIUM", "THUNDERX", i} i != 1
> {"CAVIUM", "THUNDERi", 1}
> {"CAVIUM", "THUNDRXi", 1}
> {"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
> {"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2
>
> References for APM, HiSilicon IDs:
> https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
> https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html
>
> Thanks,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web