| Message ID | 1467100442-28078-4-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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
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
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 >
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
On 29.06.16 15:56:50, Ard Biesheuvel wrote: > 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 I still would go with memcmp but have the char arrays null terminated in addition. This first makes string handling easier, and fixes some unterminated %s printfs bugs in the code. Thus, I would prefer to go with: struct pci_cfg_fixup { char oem_id[ACPI_OEM_ID_SIZE + 1]; char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; ... static struct pci_cfg_quirks mcfg_qurks[] __initconst = { /* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */ #ifdef CONFIG_PCI_HOST_THUNDER_PEM /* Pass2.0 */ { "CAVIUM", "THUNDERX", 1, ... This is also no "pain in the eyes". :) If there are zero bytes in then just use \0, e.g.: { "foo\0\0\0", "foobar\0\0", ... } For comparisation still use memcmp accordingly: memcmp(..., ACPI_OEM_ID_SIZE); memcmp(..., ACPI_OEM_TABLE_ID_SIZE); The following would be fixed too as strings are now null terminated: pr_info("Handling %s %s r%d PCI MCFG quirks\n", f->oem_id, f->oem_table_id, f->oem_revision); Btw, use dev_info(&root->device->dev, ...) here for pr_info() and modify message text, e.g.: acpi PNP0A08:04: Applying PCI MCFG quirks for CAVIUM THUNDERX rev: 1 And, we should support some sort of MCFG_OEM_REVISION_ANY to move the rev handling optional to pci_cfg_fixup::init(). Plus one spelling fix: mcfg_qurks -> mcfg_quirks Thanks, -Robert > > > 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
On 22 July 2016 at 13:38, Robert Richter <rric@kernel.org> wrote: > On 29.06.16 15:56:50, Ard Biesheuvel wrote: >> 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 > > I still would go with memcmp but have the char arrays null terminated > in addition. This first makes string handling easier, and fixes some > unterminated %s printfs bugs in the code. > > Thus, I would prefer to go with: > > struct pci_cfg_fixup { > char oem_id[ACPI_OEM_ID_SIZE + 1]; > char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; > ... > > static struct pci_cfg_quirks mcfg_qurks[] __initconst = { > /* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */ > #ifdef CONFIG_PCI_HOST_THUNDER_PEM > /* Pass2.0 */ > { "CAVIUM", "THUNDERX", 1, ... > > This is also no "pain in the eyes". :) > > If there are zero bytes in then just use \0, e.g.: > > { "foo\0\0\0", "foobar\0\0", ... } > > For comparisation still use memcmp accordingly: > > memcmp(..., ACPI_OEM_ID_SIZE); > memcmp(..., ACPI_OEM_TABLE_ID_SIZE); > > The following would be fixed too as strings are now null terminated: > > pr_info("Handling %s %s r%d PCI MCFG quirks\n", > f->oem_id, f->oem_table_id, f->oem_revision); > This looks like a clear improvement to me. > Btw, use dev_info(&root->device->dev, ...) here for pr_info() and > modify message text, e.g.: > > acpi PNP0A08:04: Applying PCI MCFG quirks for CAVIUM THUNDERX rev: 1 > > And, we should support some sort of MCFG_OEM_REVISION_ANY to move the > rev handling optional to pci_cfg_fixup::init(). > xxx_ANY implies 'wildcard', which we don't want in this code. The set of quirky hardware we intend to support is known, and wildcard matching makes it easier to circumvent our policy that from here on, i.e., that all ACPI/arm64 supported hardware shall adhere to the spec.
On 22.07.16 14:00:42, Ard Biesheuvel wrote: > On 22 July 2016 at 13:38, Robert Richter <rric@kernel.org> wrote: > > And, we should support some sort of MCFG_OEM_REVISION_ANY to move the > > rev handling optional to pci_cfg_fixup::init(). > > > > xxx_ANY implies 'wildcard', which we don't want in this code. The set > of quirky hardware we intend to support is known, and wildcard > matching makes it easier to circumvent our policy that from here on, > i.e., that all ACPI/arm64 supported hardware shall adhere to the spec. Fine with me, there shouln't be to many revs around to have a quirk per rev. -Robert
On Tue, 2016-06-28 at 09:54 +0200, 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 (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 = { ^^^^^^^^^^ mcfg_quirks > +/* { 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);
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);