Re: [PATCH v4 3/4] ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation

Re: [PATCH v4 3/4] ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation

[Bjorn Helgaas]

[+cc linux-pci, Konstantin, since you're thinking about this area, too]

The 0-day bot seems to remove mailing lists from the recipient list,
which means reports like the one below don't become part of the public
email thread and don't get connected to the patch in patchwork.  I
think this is a problem.

For example here are Xuesong's original patch, the patch in patchwork,
and the 0-day bot report:

  https://lore.kernel.org/all/20211027081312.53682-1-xuesong.chen@linux.alibaba.com/
  https://patchwork.kernel.org/project/linux-pci/patch/20211027081312.53682-1-xuesong.chen@linux.alibaba.com/
  https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/QISVC5TJIHTEMAPX7T7YXOZEDS5CBURY/

Neither the lore link nor the patchwork have any indication that the
0-day bot found a problem.

It looks like the 0-day bot sent the report to all the individual
recipients from the original patch, but it ADDED these recipients:

  llvm@lists.linux.dev
  kbuild-all@lists.01.org

and REMOVED these:

  linux-pci@vger.kernel.org
  linux-acpi@vger.kernel.org
  linux-arm-kernel@lists.infradead.org
  linux-kernel@vger.kernel.org

Gmail filed Xuesong's original patch and the 0-day report as spam.
That's a problem on my end, but the bigger problem is that I work from
the patchwork queue, which has no indication of the problem.

I'm sure you have a reason for removing the mailing lists.  Should we
revisit that?  Should I be approaching this a different way?

If a *person* responds to a patch on the list, I expect a reply-all so
the response becomes part of the thread.  Why should the 0-day bot be
different?

I added linux-pci to cc, but since the 0-day report didn't go to the
list, I don't think this message will be threaded correctly on lore.
It should be part of
https://lore.kernel.org/all/20211027081035.53370-1-xuesong.chen@linux.alibaba.com/

On Thu, Oct 28, 2021 at 06:46:16AM +0800, kernel test robot wrote:
> Hi Xuesong,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on helgaas-pci/next]
> [also build test WARNING on rafael-pm/linux-next tip/x86/core arm64/for-next/core v5.15-rc7 next-20211027]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Xuesong-Chen/PCI-MCFG-Consolidate-the-separate-PCI-MCFG-table-entry-list/20211027-171229
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> config: x86_64-randconfig-s022-20211027 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.4-dirty
>         # https://github.com/0day-ci/linux/commit/8fce66d9da6f8e55c5cf0dda4a13dba6bd51492d
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Xuesong-Chen/PCI-MCFG-Consolidate-the-separate-PCI-MCFG-table-entry-list/20211027-171229
>         git checkout 8fce66d9da6f8e55c5cf0dda4a13dba6bd51492d
>         # save the attached .config to linux build tree
>         make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/apei/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> sparse warnings: (new ones prefixed by >>)
> >> drivers/acpi/apei/apei-base.c:453:5: sparse: sparse: symbol 'remove_quirk_mcfg_res' was not declared. Should it be static?
>    drivers/acpi/apei/apei-base.c:804:12: sparse: sparse: symbol 'arch_apei_enable_cmcff' was not declared. Should it be static?
>    drivers/acpi/apei/apei-base.c:811:13: sparse: sparse: symbol 'arch_apei_report_mem_error' was not declared. Should it be static?
> 
> Please review and possibly fold the followup patch.
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [kbuild-all] Re: [PATCH v4 3/4] ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation

[Rong Chen]

On 11/2/21 1:15 AM, Bjorn Helgaas wrote:
> [+cc linux-pci, Konstantin, since you're thinking about this area, too]
>
> The 0-day bot seems to remove mailing lists from the recipient list,
> which means reports like the one below don't become part of the public
> email thread and don't get connected to the patch in patchwork.  I
> think this is a problem.
>
> For example here are Xuesong's original patch, the patch in patchwork,
> and the 0-day bot report:
>
>    https://lore.kernel.org/all/20211027081312.53682-1-xuesong.chen@linux.alibaba.com/
>    https://patchwork.kernel.org/project/linux-pci/patch/20211027081312.53682-1-xuesong.chen@linux.alibaba.com/
>    https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/QISVC5TJIHTEMAPX7T7YXOZEDS5CBURY/
>
> Neither the lore link nor the patchwork have any indication that the
> 0-day bot found a problem.
>
> It looks like the 0-day bot sent the report to all the individual
> recipients from the original patch, but it ADDED these recipients:
>
>    llvm@lists.linux.dev
>    kbuild-all@lists.01.org
>
> and REMOVED these:
>
>    linux-pci@vger.kernel.org
>    linux-acpi@vger.kernel.org
>    linux-arm-kernel@lists.infradead.org
>    linux-kernel@vger.kernel.org
>
> Gmail filed Xuesong's original patch and the 0-day report as spam.
> That's a problem on my end, but the bigger problem is that I work from
> the patchwork queue, which has no indication of the problem.
>
> I'm sure you have a reason for removing the mailing lists.  Should we
> revisit that?  Should I be approaching this a different way?
>
> If a *person* responds to a patch on the list, I expect a reply-all so
> the response becomes part of the thread.  Why should the 0-day bot be
> different?

Hi Bjorn,

Sorry for the inconvenience, we expect replying to the mailing lists too,
it should be a problem in 0-day side.

Best Regards,
Rong Chen

>
> I added linux-pci to cc, but since the 0-day report didn't go to the
> list, I don't think this message will be threaded correctly on lore.
> It should be part of
> https://lore.kernel.org/all/20211027081035.53370-1-xuesong.chen@linux.alibaba.com/
>
> On Thu, Oct 28, 2021 at 06:46:16AM +0800, kernel test robot wrote:
>> Hi Xuesong,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on helgaas-pci/next]
>> [also build test WARNING on rafael-pm/linux-next tip/x86/core arm64/for-next/core v5.15-rc7 next-20211027]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    https://github.com/0day-ci/linux/commits/Xuesong-Chen/PCI-MCFG-Consolidate-the-separate-PCI-MCFG-table-entry-list/20211027-171229
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
>> config: x86_64-randconfig-s022-20211027 (attached as .config)
>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>> reproduce:
>>          # apt-get install sparse
>>          # sparse version: v0.6.4-dirty
>>          # https://github.com/0day-ci/linux/commit/8fce66d9da6f8e55c5cf0dda4a13dba6bd51492d
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review Xuesong-Chen/PCI-MCFG-Consolidate-the-separate-PCI-MCFG-table-entry-list/20211027-171229
>>          git checkout 8fce66d9da6f8e55c5cf0dda4a13dba6bd51492d
>>          # save the attached .config to linux build tree
>>          make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/apei/
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>>
>> sparse warnings: (new ones prefixed by >>)
>>>> drivers/acpi/apei/apei-base.c:453:5: sparse: sparse: symbol 'remove_quirk_mcfg_res' was not declared. Should it be static?
>>     drivers/acpi/apei/apei-base.c:804:12: sparse: sparse: symbol 'arch_apei_enable_cmcff' was not declared. Should it be static?
>>     drivers/acpi/apei/apei-base.c:811:13: sparse: sparse: symbol 'arch_apei_report_mem_error' was not declared. Should it be static?
>>
>> Please review and possibly fold the followup patch.
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org

[PATCH v4 0/4] PCI MCFG consolidation and APEI resource filtering

[Xuesong Chen]

The issue of commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection tolerance
level") on x86 is also happened on our own ARM64 platform. We sent a patch[1]
trying to fix this issue in an arch-specific way as x86 does at first, but
according to the suggestion from Lorenzo Pieralisi and Catalin Marinas, we can
consolidate the PCI MCFG part then fix it in a more common way, that's why this
patch series comes.

[1] https://marc.info/?l=linux-arm-kernel&m=163108478627166&w=2

---
Change from v3 to v4:
  - Add a new patch (patch #3) to address the quirk ECAM access issue. Because
    the normal ECAM config space can be accessed in a lockless way, so we don't
    need the mutual exclusion with the EINJ action. But those quirks maybe break
    this rule and corrupt the configuration access, reserve its MCFG address
    regions in this case to avoid that happens. 
  
  - Add another patch (patch #4) to log the PCI MCFG entry parse message per
    the suggestion from Bjorn Helgaas. The output on ARM64 as:
    ACPI: MCFG entry for domain 0000 [bus 00-0f] at [mem 0x50000000-0x50ffffff] (base 0x50000000)
  
  - Commit message updated with more details of patch #2

Change from v2 to v3:
  - Address the comments of Lorenzo Pieralisi about the CONFIG_PCI
    dependence issue in APEI module (patch #2)

Change from v1 to v2:
  - Fix the "undefined reference to `pci_mmcfg_list'" build error in case
    of PCI_CONFIG=n, reported by the kernel test robot

Xuesong Chen (4):
  PCI: MCFG: Consolidate the separate PCI MCFG table entry list
  ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method
  ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation
  PCI: MCFG: Add the MCFG entry parse log message

 arch/x86/include/asm/pci_x86.h | 17 +----------
 arch/x86/pci/mmconfig-shared.c | 30 -------------------
 drivers/acpi/apei/apei-base.c  | 68 ++++++++++++++++++++++++++++++++----------
 drivers/acpi/pci_mcfg.c        | 46 +++++++++++++++-------------
 drivers/pci/pci.c              |  2 ++
 drivers/pci/quirks.c           |  2 ++
 include/linux/pci.h            | 18 +++++++++++
 7 files changed, 101 insertions(+), 82 deletions(-)

-- 
1.8.3.1

[PATCH v4 3/4] ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation

[Xuesong Chen]

On some platforms, the hardware ECAM implementiation is not generic
as expected, which will make the PCI configuration access atomic
primitive lost. In this case, we need to reserve those quirk MCFG
address regions when filtering the normal MCFG resource to make sure
the mutual exclusion still works between the MCFG configuration
access and EINJ's operation.

Signed-off-by: Xuesong Chen <xuesong.chen@linux.alibaba.com>
---
 drivers/acpi/apei/apei-base.c | 25 ++++++++++++++++++++++++-
 drivers/acpi/pci_mcfg.c       |  8 ++++++++
 drivers/pci/quirks.c          |  2 ++
 include/linux/pci.h           |  1 +
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index daae75a..4f7311a 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -450,6 +450,23 @@ static int apei_get_nvs_resources(struct apei_resources *resources)
 }
 
 #ifdef CONFIG_PCI
+int remove_quirk_mcfg_res(struct apei_resources *mcfg_res)
+{
+#ifdef CONFIG_PCI_QUIRKS
+	int rc = 0;
+	struct apei_resources quirk_res;
+
+	apei_resources_init(&quirk_res);
+	rc = apei_res_add(&quirk_res.iomem, pci_quirk_mcfg_res.start,
+		resource_size(&pci_quirk_mcfg_res));
+	if (rc)
+		return rc;
+
+	return apei_resources_sub(mcfg_res, &quirk_res);
+#else
+	return 0;
+#endif
+}
 extern struct list_head pci_mmcfg_list;
 static int apei_filter_mcfg_addr(struct apei_resources *res,
 			struct apei_resources *mcfg_res)
@@ -462,11 +479,17 @@ static int apei_filter_mcfg_addr(struct apei_resources *res,
 
 	apei_resources_init(mcfg_res);
 	list_for_each_entry(cfg, &pci_mmcfg_list, list) {
-		rc = apei_res_add(&mcfg_res->iomem, cfg->res.start, resource_size(&cfg->res));
+		rc = apei_res_add(&mcfg_res->iomem, cfg->res.start,
+				resource_size(&cfg->res));
 		if (rc)
 			return rc;
 	}
 
+	/* remove the pci quirk mcfg resource if any from the mcfg_res */
+	rc = remove_quirk_mcfg_res(mcfg_res);
+	if (rc)
+		return rc;
+
 	/* filter the mcfg resource from current APEI's */
 	return apei_resources_sub(res, mcfg_res);
 }
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 6ce467f..b5ab866 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -26,6 +26,8 @@ struct mcfg_fixup {
 	struct resource cfgres;
 };
 
+static bool pci_quirk_matched;
+
 #define MCFG_BUS_RANGE(start, end)	DEFINE_RES_NAMED((start),	\
 						((end) - (start) + 1),	\
 						NULL, IORESOURCE_BUS)
@@ -195,6 +197,7 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
 
 	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
 		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
+			pci_quirk_matched = true;
 			if (f->cfgres.start)
 				*cfgres = f->cfgres;
 			if (f->ops)
@@ -251,6 +254,11 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
 
 	*cfgres = res;
 	*ecam_ops = ops;
+#ifdef CONFIG_PCI_QUIRKS
+	if (pci_quirk_matched)
+		pci_quirk_mcfg_res = res;
+#endif
+
 	return 0;
 }
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f284ab4..bf64232 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -32,6 +32,8 @@
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
+struct resource pci_quirk_mcfg_res;
+
 static ktime_t fixup_debug_start(struct pci_dev *dev,
 				 void (*fn)(struct pci_dev *dev))
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 34b0cbb..10d2c17 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2103,6 +2103,7 @@ enum pci_fixup_pass {
 		suspend_late##hook, vendor, device, PCI_ANY_ID, 0, hook)
 
 #ifdef CONFIG_PCI_QUIRKS
+extern struct resource pci_quirk_mcfg_res;
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
 #else
 static inline void pci_fixup_device(enum pci_fixup_pass pass,
-- 
1.8.3.1