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

[Mathias Krause <minipli@grsecurity.net>]

[-- 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 --]