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

[Mathias Krause <minipli@grsecurity.net>]

On 12.02.26 20:23, Justin Tee wrote:
> From: Mathias Krause <minipli@grsecurity.net>
> 
> 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>
> Signed-off-by: Justin Tee <justintee8345@gmail.com>
> ---
>  drivers/scsi/lpfc/lpfc_init.c |  2 ++
>  drivers/scsi/lpfc/lpfc_sli.c  | 36 +++++++++++++++++++++++++++++------
>  drivers/scsi/lpfc/lpfc_sli4.h |  3 +++
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
> 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..690763e0aa55 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -15981,6 +15981,32 @@ 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, uint8_t dpp_barset)
> +{
> +
> +	/* DPP region is supposed to cover 64-bit BAR2 */
> +	if (dpp_barset != WQ_PCI_BAR_4_AND_5) {
> +		lpfc_log_msg(phba, KERN_WARNING, LOG_INIT,
> +			     "3273 dpp_barset x%x != WQ_PCI_BAR_4_AND_5\n",
> +			     dpp_barset);
> +		return NULL;
> +	}
> +
> +	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 +16970,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 +17155,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 */

Looks good to me, therefore:

Reviewed-by: Mathias Krause <minipli@grsecurity.net>

Thanks,
Mathias