[PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Ranjan Kumar]

Extend DMA direction override to handle additional SCSI commands
(SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
This avoids issues on platforms that perform strict DMA checks.

Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index bd3efa5b46c7..8aec475fc7a4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2686,8 +2686,22 @@ static inline int _base_scsi_dma_map(struct scsi_cmnd *cmd)
 	 * (e.g. AMD hosts). Avoid such issue by making the report zones buffer
 	 * mapping bi-directional.
 	 */
-	if (cmd->cmnd[0] == ZBC_IN && cmd->cmnd[1] == ZI_REPORT_ZONES)
-		cmd->sc_data_direction = DMA_BIDIRECTIONAL;
+
+		switch (cmd->cmnd[0]) {
+		case SECURITY_PROTOCOL_IN:
+			cmd->sc_data_direction = DMA_BIDIRECTIONAL;
+			break;
+		case ZBC_IN:
+			if  (cmd->cmnd[1] == ZI_REPORT_ZONES)
+				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
+			break;
+		case SERVICE_ACTION_IN_16:
+			if (cmd->cmnd[1] == 0x17)
+				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
+			break;
+		default:
+			break;
+	}
 
 	return scsi_dma_map(cmd);
 }
-- 
2.31.1

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Johannes Thumshirn]

On 21.07.25 13:11, Ranjan Kumar wrote:
> Extend DMA direction override to handle additional SCSI commands
> (SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
> require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
> This avoids issues on platforms that perform strict DMA checks.
> 
> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> ---
>   drivers/scsi/mpt3sas/mpt3sas_base.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index bd3efa5b46c7..8aec475fc7a4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2686,8 +2686,22 @@ static inline int _base_scsi_dma_map(struct scsi_cmnd *cmd)
>   	 * (e.g. AMD hosts). Avoid such issue by making the report zones buffer
>   	 * mapping bi-directional.
>   	 */
> -	if (cmd->cmnd[0] == ZBC_IN && cmd->cmnd[1] == ZI_REPORT_ZONES)
> -		cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +
> +		switch (cmd->cmnd[0]) {
> +		case SECURITY_PROTOCOL_IN:
> +			cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		case ZBC_IN:
> +			if  (cmd->cmnd[1] == ZI_REPORT_ZONES)
> +				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		case SERVICE_ACTION_IN_16:
> +			if (cmd->cmnd[1] == 0x17)
> +				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		default:
> +			break;
> +	}

Might be my mail client, but indentation looks broken here.

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Christoph Hellwig]

On Mon, Jul 21, 2025 at 04:35:46PM +0530, Ranjan Kumar wrote:
> Extend DMA direction override to handle additional SCSI commands
> (SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
> require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
> This avoids issues on platforms that perform strict DMA checks.

I think you are totally misstating the problem here.

The Broadcom MPT3SAS implementation, probablt the SATL is implemented
gravely incorrectly, and rewrites data structure in place against
the protocol requirements, which is cought by all but the dumbest
IOMMUs, but Broadcome until now never bothered to actually rest the
junk they ship out to customers fully.

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Mon, Jul 21, 2025 at 11:57 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jul 21, 2025 at 04:35:46PM +0530, Ranjan Kumar wrote:
> > Extend DMA direction override to handle additional SCSI commands
> > (SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
> > require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
> > This avoids issues on platforms that perform strict DMA checks.
>
> I think you are totally misstating the problem here.
>
> The Broadcom MPT3SAS implementation, probablt the SATL is implemented
> gravely incorrectly, and rewrites data structure in place against
> the protocol requirements, which is cought by all but the dumbest
> IOMMUs, but Broadcome until now never bothered to actually rest the
> junk they ship out to customers fully.

There is a resource issue which prevents SATL from doing double
buffering and RMW of the contents to map the data from SATA drives to
the host buffers for the specific commands.  Our firmware changes
leads to performance and functional issues, So we are left with either
failing those commands in the driver or changing the mapping, We
decided to go with changing the DMA mapping as we know those commands
require both read/write access to the buffers.
Any functional issue of changing the DMA mapping?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4214 bytes --]

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Christoph Hellwig]

On Tue, Jul 22, 2025 at 09:06:10AM -0600, Sathya Prakash Veerichetty wrote:
> There is a resource issue which prevents SATL from doing double
> buffering and RMW of the contents to map the data from SATA drives to
> the host buffers for the specific commands.  Our firmware changes
> leads to performance and functional issues, So we are left with either
> failing those commands in the driver or changing the mapping, We
> decided to go with changing the DMA mapping as we know those commands
> require both read/write access to the buffers.
> Any functional issue of changing the DMA mapping?

Well, we have to change the mapping to make your broken hardware work.
But these kinds of hacks in firmware are really problematic.  When an
application does a passthrough of a SCSI command it can very much
expect that the payload is not changed by a call to SG_IO, and this
breaks it.

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Wed, Jul 23, 2025 at 12:26 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jul 22, 2025 at 09:06:10AM -0600, Sathya Prakash Veerichetty wrote:
> > There is a resource issue which prevents SATL from doing double
> > buffering and RMW of the contents to map the data from SATA drives to
> > the host buffers for the specific commands.  Our firmware changes
> > leads to performance and functional issues, So we are left with either
> > failing those commands in the driver or changing the mapping, We
> > decided to go with changing the DMA mapping as we know those commands
> > require both read/write access to the buffers.
> > Any functional issue of changing the DMA mapping?
>
> Well, we have to change the mapping to make your broken hardware work.
> But these kinds of hacks in firmware are really problematic.  When an
> application does a passthrough of a SCSI command it can very much
> expect that the payload is not changed by a call to SG_IO, and this
> breaks it.
>
Agree, that is the reason, we don't want to change the mapping outside
the driver.  In this case, the problem is with reading the payload
(the write access was already available in the default mapping).  We
tried a couple of long term solutions in the firmware and with the
current hardware capabilities they were not successful.  The other
option we have other than modifying the mapping is to fail the
commands but that will result in functional failure. So we took the
least impactful approach.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4214 bytes --]

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Damien Le Moal]

On 7/21/25 20:05, Ranjan Kumar wrote:
> Extend DMA direction override to handle additional SCSI commands
> (SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
> require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
> This avoids issues on platforms that perform strict DMA checks.
> 
> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index bd3efa5b46c7..8aec475fc7a4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2686,8 +2686,22 @@ static inline int _base_scsi_dma_map(struct scsi_cmnd *cmd)
>  	 * (e.g. AMD hosts). Avoid such issue by making the report zones buffer
>  	 * mapping bi-directional.
>  	 */
> -	if (cmd->cmnd[0] == ZBC_IN && cmd->cmnd[1] == ZI_REPORT_ZONES)
> -		cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +
> +		switch (cmd->cmnd[0]) {
> +		case SECURITY_PROTOCOL_IN:
> +			cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		case ZBC_IN:
> +			if  (cmd->cmnd[1] == ZI_REPORT_ZONES)
> +				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		case SERVICE_ACTION_IN_16:
> +			if (cmd->cmnd[1] == 0x17)
> +				cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> +			break;
> +		default:
> +			break;
> +	}

Very broken indentation. And the comment above this hunk would need to be
updated too.

This really has to stop. The need for modifying a data buffer content when
translating between SCSI and ATA is nothing new. HBA hardware designs that
cannot handle that internally are simply broken and should be fixed. Or better:
make your HBA use libsas and rely on libata-scsi SAT.

The commands above are not "performance" commands as they are generally not
issued in the hot path under heavy read/write workload. So even if they are a
little slower because of the HBA double buffering, that's fine. Relying on
unauthorized accesses to host memory to replace the HBA internal buffering is
probably not faster at all anyway.

I was already very sad/disapointed to stumble on the report zones issue. Adding
more commands to this is really not desired at all. Where does it stop ? After
all, you are not doing anything like this for translating ATA log pages to
mode/vpd pages, right ? So the HBA *is* capable of double buffering, no ?

Your commit message says nothing of the use case that this fixes. Is there an
actual problem in the field without this modification ? Without a better
justification, I am not in favor of this and would prefer the fixes to go into
the HBA firmware.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1] mpt3sas: Set DMA_BIDIRECTIONAL for additional SCSI commands

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 4070 bytes --]

On Thu, Jul 31, 2025 at 7:28 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 7/21/25 20:05, Ranjan Kumar wrote:
> > Extend DMA direction override to handle additional SCSI commands
> > (SECURITY_PROTOCOL_IN, SERVICE_ACTION_IN_16 with RELEASE) that
> > require bidirectional DMA mapping, in addition to ZBC REPORT_ZONES.
> > This avoids issues on platforms that perform strict DMA checks.
> >
> > Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> > index bd3efa5b46c7..8aec475fc7a4 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> > @@ -2686,8 +2686,22 @@ static inline int _base_scsi_dma_map(struct scsi_cmnd *cmd)
> >        * (e.g. AMD hosts). Avoid such issue by making the report zones buffer
> >        * mapping bi-directional.
> >        */
> > -     if (cmd->cmnd[0] == ZBC_IN && cmd->cmnd[1] == ZI_REPORT_ZONES)
> > -             cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> > +
> > +             switch (cmd->cmnd[0]) {
> > +             case SECURITY_PROTOCOL_IN:
> > +                     cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> > +                     break;
> > +             case ZBC_IN:
> > +                     if  (cmd->cmnd[1] == ZI_REPORT_ZONES)
> > +                             cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> > +                     break;
> > +             case SERVICE_ACTION_IN_16:
> > +                     if (cmd->cmnd[1] == 0x17)
> > +                             cmd->sc_data_direction = DMA_BIDIRECTIONAL;
> > +                     break;
> > +             default:
> > +                     break;
> > +     }
>
> Very broken indentation. And the comment above this hunk would need to be
> updated too.
Will fix it in the next version of the patch.

>
> This really has to stop. The need for modifying a data buffer content when
> translating between SCSI and ATA is nothing new. HBA hardware designs that
> cannot handle that internally are simply broken and should be fixed. Or better:
> make your HBA use libsas and rely on libata-scsi SAT.
>
> The commands above are not "performance" commands as they are generally not
> issued in the hot path under heavy read/write workload. So even if they are a
> little slower because of the HBA double buffering, that's fine. Relying on
> unauthorized accesses to host memory to replace the HBA internal buffering is
> probably not faster at all anyway.
>
There were multiple attempts to work this around in our firmware in
the existing generation of the products  for the last few months and
with the limited memory resources available in the controller,
performing double buffering of the commands for 1024 drives were
resulting in resource unavailability and commands getting timed out
and resets. So no controller firmware level solution is viable at this
moment.
> I was already very sad/disapointed to stumble on the report zones issue. Adding
> more commands to this is really not desired at all. Where does it stop ? After
> all, you are not doing anything like this for translating ATA log pages to
> mode/vpd pages, right ? So the HBA *is* capable of double buffering, no ?
>
> Your commit message says nothing of the use case that this fixes. Is there an
> actual problem in the field without this modification ? Without a better
> justification, I am not in favor of this and would prefer the fixes to go into
> the HBA firmware.
There are no real use cases we have encountered. What we want to
protect is if one such command is issued we shouldn't end up in IOMMU
fault, with that said, we have two options

1. Change the dma mapping as mentioned in this patch
2. Modify the drivers to fail the specific commands as unsupported.


>
>
> --
> Damien Le Moal
> Western Digital Research

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5489 bytes --]