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

[Mathias Krause <minipli@grsecurity.net>]

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