[PATCH] scsi: lpfc: Properly set WC for DPP mapping

[PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Mathias Krause]

Using set_memory_wc() to enable write-combining for the DPP portion of
the MMIO mapping is wrong as set_memory_*() is meant to operate on RAM
only, not MMIO mappings. In fact, as used currently triggers a BUG_ON()
with enabled CONFIG_DEBUG_VIRTUAL.

Simply map the DPP region separately and in addition to the already
existing mappings, avoiding any possible negative side effects for
these.

Fixes: 1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
Cc: James Smart <jsmart2021@gmail.com>
---
!!! WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING !!!

I don't have any hardware to test this on. I just got the report from a
customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
have any spec for the hardware either, I assumed a few things, like:
1/ DPP regions are only supported on SIL4 devices.
2/ DPP may be shared with other registers (doorbells?) in the same BAR.

That's why I fixed the bug by creating a dedicated mapping for the DPP
region instead of making the existing one ioremap_wc(). I simply
assumed, the fact that 'dpp_offset' needs to be queried instead of
blindly assumed to be zero allows the DPP region to be shared with other
registers that may be harmed by a WC mapping (slow reads, etc.).

Again, as I don't have any hardware, I can't do any performance
benchmarks either but the dedicated mapping should avoid past[1] latency
issues observed.

[1] https://lore.kernel.org/all/e1da721e-8ec4-9e57-9fb2-9141514e8785@gmail.com/

 drivers/scsi/lpfc/lpfc_init.c |  2 ++
 drivers/scsi/lpfc/lpfc_sli.c  | 52 +++++++++++++++++++++++++++++++----
 drivers/scsi/lpfc/lpfc_sli4.h |  3 ++
 3 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index b1460b16dd91..c6bb45c3d4c4 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -12034,6 +12034,8 @@ lpfc_sli4_pci_mem_unset(struct lpfc_hba *phba)
 		iounmap(phba->sli4_hba.conf_regs_memmap_p);
 		if (phba->sli4_hba.dpp_regs_memmap_p)
 			iounmap(phba->sli4_hba.dpp_regs_memmap_p);
+		if (phba->sli4_hba.dpp_regs_memmap_wc_p)
+			iounmap(phba->sli4_hba.dpp_regs_memmap_wc_p);
 		break;
 	case LPFC_SLI_INTF_IF_TYPE_1:
 		break;
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 73d77cfab5f8..76e32891ee20 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -15981,6 +15981,46 @@ lpfc_dual_chute_pci_bar_map(struct lpfc_hba *phba, uint16_t pci_barset)
 	return NULL;
 }
 
+static phys_addr_t
+lpfc_dual_chute_pci_bar_addr(struct lpfc_hba *phba, uint16_t pci_barset)
+{
+	if (!phba->pcidev)
+		return PHYS_ADDR_MAX;
+
+	switch (pci_barset) {
+	case WQ_PCI_BAR_0_AND_1:
+		return phba->pci_bar0_map;
+	case WQ_PCI_BAR_2_AND_3:
+		return phba->pci_bar1_map;
+	case WQ_PCI_BAR_4_AND_5:
+		return phba->pci_bar2_map;
+	default:
+		break;
+	}
+	return PHYS_ADDR_MAX;
+}
+
+static __maybe_unused void __iomem *
+lpfc_dpp_wc_map(struct lpfc_hba *phba, uint16_t pci_barset, uint32_t dpp_offset,
+		uint32_t size)
+{
+	if (!phba->sli4_hba.dpp_regs_memmap_wc_p) {
+		void __iomem *dpp_map;
+		phys_addr_t dpp_addr;
+
+		dpp_addr = lpfc_dual_chute_pci_bar_addr(phba, pci_barset);
+		if (dpp_addr == PHYS_ADDR_MAX)
+			return NULL;
+
+		dpp_addr += dpp_offset;
+		dpp_map = ioremap_wc(dpp_addr, size);
+		if (dpp_map)
+			phba->sli4_hba.dpp_regs_memmap_wc_p = dpp_map;
+	}
+
+	return phba->sli4_hba.dpp_regs_memmap_wc_p;
+}
+
 /**
  * lpfc_modify_hba_eq_delay - Modify Delay Multiplier on EQs
  * @phba: HBA structure that EQs are on.
@@ -16944,9 +16984,6 @@ lpfc_wq_create(struct lpfc_hba *phba, struct lpfc_queue *wq,
 	uint8_t dpp_barset;
 	uint32_t dpp_offset;
 	uint8_t wq_create_version;
-#ifdef CONFIG_X86
-	unsigned long pg_addr;
-#endif
 
 	/* sanity check on queue memory */
 	if (!wq || !cq)
@@ -17132,14 +17169,17 @@ lpfc_wq_create(struct lpfc_hba *phba, struct lpfc_queue *wq,
 
 #ifdef CONFIG_X86
 			/* Enable combined writes for DPP aperture */
-			pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
-			rc = set_memory_wc(pg_addr, 1);
-			if (rc) {
+			bar_memmap_p = lpfc_dpp_wc_map(phba, pci_barset,
+						       dpp_offset,
+						       wq->entry_size);
+			if (!bar_memmap_p) {
 				lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
 					"3272 Cannot setup Combined "
 					"Write on WQ[%d] - disable DPP\n",
 					wq->queue_id);
 				phba->cfg_enable_dpp = 0;
+			} else {
+				wq->dpp_regaddr = bar_memmap_p;
 			}
 #else
 			phba->cfg_enable_dpp = 0;
diff --git a/drivers/scsi/lpfc/lpfc_sli4.h b/drivers/scsi/lpfc/lpfc_sli4.h
index fd6dab157887..bcd12b59fa92 100644
--- a/drivers/scsi/lpfc/lpfc_sli4.h
+++ b/drivers/scsi/lpfc/lpfc_sli4.h
@@ -785,6 +785,9 @@ struct lpfc_sli4_hba {
 	void __iomem *dpp_regs_memmap_p;  /* Kernel memory mapped address for
 					   * dpp registers
 					   */
+	void __iomem *dpp_regs_memmap_wc_p;/* Kernel memory mapped address for
+					   * dpp registers with write combining
+					   */
 	union {
 		struct {
 			/* IF Type 0, BAR 0 PCI cfg space reg mem map */
-- 
2.47.3

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

Hi Mathias,

> I don't have any hardware to test this on. I just got the report from a
> customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
> have any spec for the hardware either, I assumed a few things, like:
> 1/ DPP regions are only supported on SIL4 devices.
> 2/ DPP may be shared with other registers (doorbells?) in the same BAR.

Sure, we’ll have close look at this patch and test on real hardware.
Will report back on our findings.

Thanks,
Justin

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

Hi Mathias,

> > I don't have any hardware to test this on. I just got the report from a
> > customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
> > have any spec for the hardware either, I assumed a few things, like:
> > 1/ DPP regions are only supported on SIL4 devices.
> > 2/ DPP may be shared with other registers (doorbells?) in the same BAR.
>
> Sure, we’ll have close look at this patch and test on real hardware.
> Will report back on our findings.

This patch has been tested on real hardware and I/O is stalled when
using DPP.  We can look for an alternative solution.

Do we happen to have a dmesg log with the call trace observed?

I plan on attempting to reproduce what the customer is observing by
enabling CONFIG_DEBUG_VIRTUAL, and would be helpful to see context
from a dmesg log.

Regards,
Justin

On Fri, Jan 16, 2026 at 9:46 AM Justin Tee <justintee8345@gmail.com> wrote:
>
> Hi Mathias,
>
> > I don't have any hardware to test this on. I just got the report from a
> > customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
> > have any spec for the hardware either, I assumed a few things, like:
> > 1/ DPP regions are only supported on SIL4 devices.
> > 2/ DPP may be shared with other registers (doorbells?) in the same BAR.
>
> Sure, we’ll have close look at this patch and test on real hardware.
> Will report back on our findings.
>
> Thanks,
> Justin

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Mathias Krause]

Hi Justin,

On 16.01.26 23:33, Justin Tee wrote:
> Hi Mathias,
> 
>>> I don't have any hardware to test this on. I just got the report from a
>>> customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
>>> have any spec for the hardware either, I assumed a few things, like:
>>> 1/ DPP regions are only supported on SIL4 devices.
>>> 2/ DPP may be shared with other registers (doorbells?) in the same BAR.
>>
>> Sure, we’ll have close look at this patch and test on real hardware.
>> Will report back on our findings.
> 
> This patch has been tested on real hardware and I/O is stalled when
> using DPP.  We can look for an alternative solution.

Hmm, that's bad. However, that makes me think, making the mapping
write-combining may had been a bad idea from the very beginning?

The thing is, the call to set_memory_wc() won't really do what commit
1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4") wanted
it to do. It does change the PTE flags of the mapping, but it operates
on invalid physical addresses as __pa($VMALLOC_ADDR) just won't work.

Given the time when it was developed, memcpy_{from,to}io() was really
slow on modern systems as these would simply be memcpy() and that was
ASM-alternative'd to a 'rep movsb' if the CPU featured X86_FEATURE_ERMS.

However, the LPFC driver already tries to do "big writes" by making use
of __raw_writeq() instead of memcpy_toio(). That should have solved most
of the latency issues back then already which makes me think, simply
dropping the call to set_memory_wc() is probably the next best option.

> 
> Do we happen to have a dmesg log with the call trace observed?

Unfortunately, we don't. We just have a truncated screenshot with RIP in
__phys_addr(), code and register dump. The surrounding symbolization of
register values we have in grsecurity makes it clear, it's lpfc and, in
fact, the set_memory_wc() call in lpfc_wq_create().

Specifically, it's the call to __pa(addr), which is __phys_addr() under
CONFIG_DEBUG_VIRTUAL, triggering the VIRTUAL_BUG_ON(... ||
!phys_addr_valid(x)) which boils down to BUG_ON() under
CONFIG_DEBUG_VIRTUAL.

> 
> I plan on attempting to reproduce what the customer is observing by
> enabling CONFIG_DEBUG_VIRTUAL, and would be helpful to see context
> from a dmesg log.

Just loading lpfc.ko with enabled CONFIG_DEBUG_VIRTUAL on a DPP
supported platform should trigger the bug as the set_memory_wc() is
unconditionally attempted in this case.

Thanks,
Mathias

> 
> Regards,
> Justin

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

Hi Mathias,

Thanks, I think I’m able to reproduce the call trace of concern.  I’ll
have a closer look at this patch and will report back.

Regards,
Justin

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

Hi Mathias,

> Thanks, I think I’m able to reproduce the call trace of concern.  I’ll
> have a closer look at this patch and will report back.

I have some slight changes to the original patch, which I've tested on
real hardware.  Please see below.
Is it possible to check if this works for the customer as well?

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index a116a16c4a6f..b5e53c7d33e7 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -12039,6 +12039,8 @@ lpfc_sli4_pci_mem_unset(struct lpfc_hba *phba)
         iounmap(phba->sli4_hba.conf_regs_memmap_p);
         if (phba->sli4_hba.dpp_regs_memmap_p)
             iounmap(phba->sli4_hba.dpp_regs_memmap_p);
+        if (phba->sli4_hba.dpp_regs_memmap_wc_p)
+            iounmap(phba->sli4_hba.dpp_regs_memmap_wc_p);
         break;
     case LPFC_SLI_INTF_IF_TYPE_1:
         break;
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 734af3d039f8..a0b55bd8566e 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -15981,6 +15981,47 @@ lpfc_dual_chute_pci_bar_map(struct lpfc_hba
*phba, uint16_t pci_barset)
     return NULL;
 }

+static phys_addr_t
+lpfc_dual_chute_pci_bar_addr(struct lpfc_hba *phba, uint16_t pci_barset)
+{
+    if (!phba->pcidev)
+        return PHYS_ADDR_MAX;
+
+    switch (pci_barset) {
+    case WQ_PCI_BAR_0_AND_1:
+        return phba->pci_bar0_map;
+    case WQ_PCI_BAR_2_AND_3:
+        return phba->pci_bar1_map;
+    case WQ_PCI_BAR_4_AND_5:
+        return phba->pci_bar2_map;
+    default:
+        break;
+    }
+    return PHYS_ADDR_MAX;
+}
+
+static __maybe_unused void __iomem *
+lpfc_dpp_wc_map(struct lpfc_hba *phba, uint16_t dpp_barset)
+{
+    if (!phba->sli4_hba.dpp_regs_memmap_wc_p) {
+        void __iomem *dpp_map;
+        phys_addr_t dpp_addr;
+
+        dpp_addr = lpfc_dual_chute_pci_bar_addr(phba, dpp_barset);
+        if (dpp_addr == PHYS_ADDR_MAX)
+            return NULL;
+
+        dpp_map = ioremap_wc(dpp_addr,
+                     pci_resource_len(phba->pcidev,
+                              PCI_64BIT_BAR4));
+
+        if (dpp_map)
+            phba->sli4_hba.dpp_regs_memmap_wc_p = dpp_map;
+    }
+
+    return phba->sli4_hba.dpp_regs_memmap_wc_p;
+}
+
 /**
  * lpfc_modify_hba_eq_delay - Modify Delay Multiplier on EQs
  * @phba: HBA structure that EQs are on.
@@ -16944,9 +16985,6 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
lpfc_queue *wq,
     uint8_t dpp_barset;
     uint32_t dpp_offset;
     uint8_t wq_create_version;
-#ifdef CONFIG_X86
-    unsigned long pg_addr;
-#endif

     /* sanity check on queue memory */
     if (!wq || !cq)
@@ -17132,14 +17170,15 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
lpfc_queue *wq,

 #ifdef CONFIG_X86
             /* Enable combined writes for DPP aperture */
-            pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
-            rc = set_memory_wc(pg_addr, 1);
-            if (rc) {
+            bar_memmap_p = lpfc_dpp_wc_map(phba, dpp_barset);
+            if (!bar_memmap_p) {
                 lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
                     "3272 Cannot setup Combined "
                     "Write on WQ[%d] - disable DPP\n",
                     wq->queue_id);
                 phba->cfg_enable_dpp = 0;
+            } else {
+                wq->dpp_regaddr = bar_memmap_p + dpp_offset;
             }
 #else
             phba->cfg_enable_dpp = 0;
diff --git a/drivers/scsi/lpfc/lpfc_sli4.h b/drivers/scsi/lpfc/lpfc_sli4.h
index ee58383492b2..b6d90604bb61 100644
--- a/drivers/scsi/lpfc/lpfc_sli4.h
+++ b/drivers/scsi/lpfc/lpfc_sli4.h
@@ -785,6 +785,9 @@ struct lpfc_sli4_hba {
     void __iomem *dpp_regs_memmap_p;  /* Kernel memory mapped address for
                        * dpp registers
                        */
+    void __iomem *dpp_regs_memmap_wc_p;/* Kernel memory mapped address for
+                        * dpp registers with write combining
+                        */
     union {
         struct {
             /* IF Type 0, BAR 0 PCI cfg space reg mem map */


Regards,
Justin

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Mathias Krause]

On 09.02.26 19:47, Justin Tee wrote:
> Hi Mathias,
> 
>> Thanks, I think I’m able to reproduce the call trace of concern.  I’ll
>> have a closer look at this patch and will report back.
> 
> I have some slight changes to the original patch, which I've tested on
> real hardware.  Please see below.

Thanks for testing! ...and fixing my goof with using pci_barset instead
of dpp_barset. Dunno what I was thinking!

> Is it possible to check if this works for the customer as well?

If your tests on real hardware ran fine, it should be good for our
customer as well. It still gets rid of the problematic set_memory_wc()
call so it's fine from our point of view.

Though, some comments below...

> 
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index a116a16c4a6f..b5e53c7d33e7 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -12039,6 +12039,8 @@ lpfc_sli4_pci_mem_unset(struct lpfc_hba *phba)
>          iounmap(phba->sli4_hba.conf_regs_memmap_p);
>          if (phba->sli4_hba.dpp_regs_memmap_p)
>              iounmap(phba->sli4_hba.dpp_regs_memmap_p);
> +        if (phba->sli4_hba.dpp_regs_memmap_wc_p)
> +            iounmap(phba->sli4_hba.dpp_regs_memmap_wc_p);
>          break;
>      case LPFC_SLI_INTF_IF_TYPE_1:
>          break;
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 734af3d039f8..a0b55bd8566e 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -15981,6 +15981,47 @@ lpfc_dual_chute_pci_bar_map(struct lpfc_hba
> *phba, uint16_t pci_barset)
>      return NULL;
>  }
> 
> +static phys_addr_t
> +lpfc_dual_chute_pci_bar_addr(struct lpfc_hba *phba, uint16_t pci_barset)
> +{
> +    if (!phba->pcidev)
> +        return PHYS_ADDR_MAX;
> +
> +    switch (pci_barset) {
> +    case WQ_PCI_BAR_0_AND_1:
> +        return phba->pci_bar0_map;
> +    case WQ_PCI_BAR_2_AND_3:
> +        return phba->pci_bar1_map;
> +    case WQ_PCI_BAR_4_AND_5:
> +        return phba->pci_bar2_map;
> +    default:
> +        break;
> +    }
> +    return PHYS_ADDR_MAX;
> +}
> +
> +static __maybe_unused void __iomem *
> +lpfc_dpp_wc_map(struct lpfc_hba *phba, uint16_t dpp_barset)
> +{
> +    if (!phba->sli4_hba.dpp_regs_memmap_wc_p) {
> +        void __iomem *dpp_map;
> +        phys_addr_t dpp_addr;
> +
> +        dpp_addr = lpfc_dual_chute_pci_bar_addr(phba, dpp_barset);
> +        if (dpp_addr == PHYS_ADDR_MAX)
> +            return NULL;
> +

> +        dpp_map = ioremap_wc(dpp_addr,
> +                     pci_resource_len(phba->pcidev,
> +                              PCI_64BIT_BAR4));

You're hard-coding PCI_64BIT_BAR4 here which *feels* inappropriate if we
went all the way with wrappers like lpfc_dual_chute_pci_bar_addr() and
making sure to be using the BAR the device told us. In fact, that was
the reason, I did it like this, assuming it might be something else than
WQ_PCI_BAR_4_AND_5, e.g. a BAR shared with the doorbell registers. If
that cannot happen, what's the reason to have 'dpp_offset'?

Also, what's the reason to do the offset calculation in the caller?
ioremap_wc() can handle non-page-aligned / offset addresses just fine.
That's why my version passed dpp_offset to lpfc_dpp_wc_map() and made it
adjust the to-be-mapped address before calling ioremap_wc(); to only
remap what's needed.
Same for the size: I just capped it to what's needed by its only user in
lpfc_sli4_wq_put(). Again, ioremap_wc() will do "The Right Thing(TM)"
and map all required pages.

> +
> +        if (dpp_map)
> +            phba->sli4_hba.dpp_regs_memmap_wc_p = dpp_map;
> +    }
> +
> +    return phba->sli4_hba.dpp_regs_memmap_wc_p;
> +}
> +
>  /**
>   * lpfc_modify_hba_eq_delay - Modify Delay Multiplier on EQs
>   * @phba: HBA structure that EQs are on.
> @@ -16944,9 +16985,6 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
> lpfc_queue *wq,
>      uint8_t dpp_barset;
>      uint32_t dpp_offset;
>      uint8_t wq_create_version;
> -#ifdef CONFIG_X86
> -    unsigned long pg_addr;
> -#endif
> 
>      /* sanity check on queue memory */
>      if (!wq || !cq)
> @@ -17132,14 +17170,15 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
> lpfc_queue *wq,
> 
>  #ifdef CONFIG_X86
>              /* Enable combined writes for DPP aperture */
> -            pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
> -            rc = set_memory_wc(pg_addr, 1);
> -            if (rc) {
> +            bar_memmap_p = lpfc_dpp_wc_map(phba, dpp_barset);
> +            if (!bar_memmap_p) {
>                  lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
>                      "3272 Cannot setup Combined "
>                      "Write on WQ[%d] - disable DPP\n",
>                      wq->queue_id);
>                  phba->cfg_enable_dpp = 0;
> +            } else {
> +                wq->dpp_regaddr = bar_memmap_p + dpp_offset;
>              }
>  #else
>              phba->cfg_enable_dpp = 0;
> diff --git a/drivers/scsi/lpfc/lpfc_sli4.h b/drivers/scsi/lpfc/lpfc_sli4.h
> index ee58383492b2..b6d90604bb61 100644
> --- a/drivers/scsi/lpfc/lpfc_sli4.h
> +++ b/drivers/scsi/lpfc/lpfc_sli4.h
> @@ -785,6 +785,9 @@ struct lpfc_sli4_hba {
>      void __iomem *dpp_regs_memmap_p;  /* Kernel memory mapped address for
>                         * dpp registers
>                         */
> +    void __iomem *dpp_regs_memmap_wc_p;/* Kernel memory mapped address for
> +                        * dpp registers with write combining
> +                        */
>      union {
>          struct {
>              /* IF Type 0, BAR 0 PCI cfg space reg mem map */
> 
> 
> Regards,
> Justin

Out of curiosity, the I/O stalls are no longer happening with this version?

Thanks,
Mathias

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

> You're hard-coding PCI_64BIT_BAR4 here which *feels* inappropriate if we
> went all the way with wrappers like lpfc_dual_chute_pci_bar_addr() and
> making sure to be using the BAR the device told us. In fact, that was
> the reason, I did it like this, assuming it might be something else than
> WQ_PCI_BAR_4_AND_5, e.g. a BAR shared with the doorbell registers. If
> that cannot happen, what's the reason to have 'dpp_offset'?

Totally right, we actually only use WQ_PCI_BAR_4_AND_5 for dpp.
Hence, the hardcode to PCI_64BIT_BAR4.  Please see revised patch
below.

The reason for dpp_offset is to be able to store the dpp register
address for each individual WQ created.  We can just add the
dpp_offset from the response of our mailbox command on top of the base
dpp register address.

> Also, what's the reason to do the offset calculation in the caller?
> ioremap_wc() can handle non-page-aligned / offset addresses just fine.
> That's why my version passed dpp_offset to lpfc_dpp_wc_map() and made it
> adjust the to-be-mapped address before calling ioremap_wc(); to only
> remap what's needed.
> Same for the size: I just capped it to what's needed by its only user in
> lpfc_sli4_wq_put(). Again, ioremap_wc() will do "The Right Thing(TM)"
> and map all required pages.

Sure, but the DPP apertures are contiguous anyways so what’s the harm
in a single ioremap_wc call and then have each wq->dpp_regaddr point
to its corresponding dpp register address, it seemed simpler?

> Out of curiosity, the I/O stalls are no longer happening with this version?
Yes, I/O stalls are no longer happening with that version and the
revised patch below.

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index a116a16c4a6f..b5e53c7d33e7 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -12039,6 +12039,8 @@ lpfc_sli4_pci_mem_unset(struct lpfc_hba *phba)
                iounmap(phba->sli4_hba.conf_regs_memmap_p);
                if (phba->sli4_hba.dpp_regs_memmap_p)
                        iounmap(phba->sli4_hba.dpp_regs_memmap_p);
+               if (phba->sli4_hba.dpp_regs_memmap_wc_p)
+                       iounmap(phba->sli4_hba.dpp_regs_memmap_wc_p);
                break;
        case LPFC_SLI_INTF_IF_TYPE_1:
                break;
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 734af3d039f8..f4bff6ee3a0b 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -15981,6 +15981,23 @@ lpfc_dual_chute_pci_bar_map(struct lpfc_hba
*phba, uint16_t pci_barset)
        return NULL;
 }

+static __maybe_unused void __iomem *
+lpfc_dpp_wc_map(struct lpfc_hba *phba)
+{
+       if (!phba->sli4_hba.dpp_regs_memmap_wc_p) {
+               void __iomem *dpp_map;
+
+               dpp_map = ioremap_wc(phba->pci_bar2_map,
+                                    pci_resource_len(phba->pcidev,
+                                                     PCI_64BIT_BAR4));
+
+               if (dpp_map)
+                       phba->sli4_hba.dpp_regs_memmap_wc_p = dpp_map;
+       }
+
+       return phba->sli4_hba.dpp_regs_memmap_wc_p;
+}
+
 /**
  * lpfc_modify_hba_eq_delay - Modify Delay Multiplier on EQs
  * @phba: HBA structure that EQs are on.
@@ -16944,9 +16961,6 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
lpfc_queue *wq,
        uint8_t dpp_barset;
        uint32_t dpp_offset;
        uint8_t wq_create_version;
-#ifdef CONFIG_X86
-       unsigned long pg_addr;
-#endif

        /* sanity check on queue memory */
        if (!wq || !cq)
@@ -17132,14 +17146,15 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
lpfc_queue *wq,

 #ifdef CONFIG_X86
                        /* Enable combined writes for DPP aperture */
-                       pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
-                       rc = set_memory_wc(pg_addr, 1);
-                       if (rc) {
+                       bar_memmap_p = lpfc_dpp_wc_map(phba);
+                       if (!bar_memmap_p) {
                                lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
                                        "3272 Cannot setup Combined "
                                        "Write on WQ[%d] - disable DPP\n",
                                        wq->queue_id);
                                phba->cfg_enable_dpp = 0;
+                       } else {
+                               wq->dpp_regaddr = bar_memmap_p + dpp_offset;
                        }
 #else
                        phba->cfg_enable_dpp = 0;
diff --git a/drivers/scsi/lpfc/lpfc_sli4.h b/drivers/scsi/lpfc/lpfc_sli4.h
index ee58383492b2..b6d90604bb61 100644
--- a/drivers/scsi/lpfc/lpfc_sli4.h
+++ b/drivers/scsi/lpfc/lpfc_sli4.h
@@ -785,6 +785,9 @@ struct lpfc_sli4_hba {
        void __iomem *dpp_regs_memmap_p;  /* Kernel memory mapped address for
                                           * dpp registers
                                           */
+       void __iomem *dpp_regs_memmap_wc_p;/* Kernel memory mapped address for
+                                           * dpp registers with write combining
+                                           */
        union {
                struct {
                        /* IF Type 0, BAR 0 PCI cfg space reg mem map */

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Mathias Krause]

[-- Attachment #1.1: Type: text/plain, Size: 6516 bytes --]

On 12.02.26 01:01, Justin Tee wrote:
>> You're hard-coding PCI_64BIT_BAR4 here which *feels* inappropriate if we
>> went all the way with wrappers like lpfc_dual_chute_pci_bar_addr() and
>> making sure to be using the BAR the device told us. In fact, that was
>> the reason, I did it like this, assuming it might be something else than
>> WQ_PCI_BAR_4_AND_5, e.g. a BAR shared with the doorbell registers. If
>> that cannot happen, what's the reason to have 'dpp_offset'?
> 
> Totally right, we actually only use WQ_PCI_BAR_4_AND_5 for dpp.
> Hence, the hardcode to PCI_64BIT_BAR4.  Please see revised patch
> below.

But this would still be inconsistent with the previous mapping of the
DPP region (wq->dpp_regaddr) which uses 'dpp_barset' as gathered from
the hardware. This, at least, opens the window for inconsistencies
if/when a future devices returns something else than WQ_PCI_BAR_4_AND_5.

> 
> The reason for dpp_offset is to be able to store the dpp register
> address for each individual WQ created.  We can just add the
> dpp_offset from the response of our mailbox command on top of the base
> dpp register address.

Ahh, that makes totally sense. Thanks for the explanation.

> 
>> Also, what's the reason to do the offset calculation in the caller?
>> ioremap_wc() can handle non-page-aligned / offset addresses just fine.
>> That's why my version passed dpp_offset to lpfc_dpp_wc_map() and made it
>> adjust the to-be-mapped address before calling ioremap_wc(); to only
>> remap what's needed.
>> Same for the size: I just capped it to what's needed by its only user in
>> lpfc_sli4_wq_put(). Again, ioremap_wc() will do "The Right Thing(TM)"
>> and map all required pages.
> 
> Sure, but the DPP apertures are contiguous anyways so what’s the harm
> in a single ioremap_wc call and then have each wq->dpp_regaddr point
> to its corresponding dpp register address, it seemed simpler?

Yeah, you're right. I missed the point that there are multiple queues,
each with a different offest into the DPP region.

> 
>> Out of curiosity, the I/O stalls are no longer happening with this version?
> Yes, I/O stalls are no longer happening with that version and the
> revised patch below.

Perfect!

> 
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index a116a16c4a6f..b5e53c7d33e7 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -12039,6 +12039,8 @@ lpfc_sli4_pci_mem_unset(struct lpfc_hba *phba)
>                 iounmap(phba->sli4_hba.conf_regs_memmap_p);
>                 if (phba->sli4_hba.dpp_regs_memmap_p)
>                         iounmap(phba->sli4_hba.dpp_regs_memmap_p);
> +               if (phba->sli4_hba.dpp_regs_memmap_wc_p)
> +                       iounmap(phba->sli4_hba.dpp_regs_memmap_wc_p);
>                 break;
>         case LPFC_SLI_INTF_IF_TYPE_1:
>                 break;
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 734af3d039f8..f4bff6ee3a0b 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -15981,6 +15981,23 @@ lpfc_dual_chute_pci_bar_map(struct lpfc_hba
> *phba, uint16_t pci_barset)
>         return NULL;
>  }
> 

> +static __maybe_unused void __iomem *
> +lpfc_dpp_wc_map(struct lpfc_hba *phba)
> +{
> +       if (!phba->sli4_hba.dpp_regs_memmap_wc_p) {
> +               void __iomem *dpp_map;
> +
> +               dpp_map = ioremap_wc(phba->pci_bar2_map,
> +                                    pci_resource_len(phba->pcidev,
> +                                                     PCI_64BIT_BAR4));
> +
> +               if (dpp_map)
> +                       phba->sli4_hba.dpp_regs_memmap_wc_p = dpp_map;
> +       }
> +
> +       return phba->sli4_hba.dpp_regs_memmap_wc_p;
> +}

To address my propably just paranoia concerns, can you pass 'dpp_barset'
as an argument to lpfc_dpp_wc_map() and add the following at the begin
of the function?:

	/* DPP region is supposed to cover 64-bit BAR2 */
	if (WARN_ON(dpp_barset != WQ_PCI_BAR_4_AND_5))
		return NULL;

That would make me more comfortable with hardcoding BAR.

> +
>  /**
>   * lpfc_modify_hba_eq_delay - Modify Delay Multiplier on EQs
>   * @phba: HBA structure that EQs are on.
> @@ -16944,9 +16961,6 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
> lpfc_queue *wq,
>         uint8_t dpp_barset;
>         uint32_t dpp_offset;
>         uint8_t wq_create_version;
> -#ifdef CONFIG_X86
> -       unsigned long pg_addr;
> -#endif
> 
>         /* sanity check on queue memory */
>         if (!wq || !cq)
> @@ -17132,14 +17146,15 @@ lpfc_wq_create(struct lpfc_hba *phba, struct
> lpfc_queue *wq,
> 
>  #ifdef CONFIG_X86
>                         /* Enable combined writes for DPP aperture */
> -                       pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
> -                       rc = set_memory_wc(pg_addr, 1);
> -                       if (rc) {
> +                       bar_memmap_p = lpfc_dpp_wc_map(phba);
> +                       if (!bar_memmap_p) {
>                                 lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
>                                         "3272 Cannot setup Combined "
>                                         "Write on WQ[%d] - disable DPP\n",
>                                         wq->queue_id);
>                                 phba->cfg_enable_dpp = 0;
> +                       } else {
> +                               wq->dpp_regaddr = bar_memmap_p + dpp_offset;
>                         }
>  #else
>                         phba->cfg_enable_dpp = 0;
> diff --git a/drivers/scsi/lpfc/lpfc_sli4.h b/drivers/scsi/lpfc/lpfc_sli4.h
> index ee58383492b2..b6d90604bb61 100644
> --- a/drivers/scsi/lpfc/lpfc_sli4.h
> +++ b/drivers/scsi/lpfc/lpfc_sli4.h
> @@ -785,6 +785,9 @@ struct lpfc_sli4_hba {
>         void __iomem *dpp_regs_memmap_p;  /* Kernel memory mapped address for
>                                            * dpp registers
>                                            */
> +       void __iomem *dpp_regs_memmap_wc_p;/* Kernel memory mapped address for
> +                                           * dpp registers with write combining
> +                                           */
>         union {
>                 struct {
>                         /* IF Type 0, BAR 0 PCI cfg space reg mem map */

Thanks,
Mathias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Justin Tee]

> To address my propably just paranoia concerns, can you pass 'dpp_barset'
> as an argument to lpfc_dpp_wc_map() and add the following at the begin
> of the function?:
>
>         /* DPP region is supposed to cover 64-bit BAR2 */
>         if (WARN_ON(dpp_barset != WQ_PCI_BAR_4_AND_5))
>                 return NULL;
>
> That would make me more comfortable with hardcoding BAR.

Sure thing, I’ll post a v2 of this patch with my Signed-off-by and you
still as the original author.

Thanks,
Justin