Add tri-mode support to mpr

[slm]

Summary

Add tri-mode support (SAS/SATA/PCIe), also known as Ventura Gen3.5 support, to the mpr driver. This includes support for NVMe devices.

[mav]

I've added some comments inline.

sys/dev/mpr/mpr.c

1405

Is this 1MB limit is based on something or at least reported outside? Now this driver reports maximal IO size via cpi->maxio in XPT_PATH_INQ, and it looks much more based on some real limitations. FreeBSD limits IOs to MAXPHYS constant, which is 128KB by default, but it can be overridden in kernel config.

sys/dev/mpr/mpr_sas.c

1936

Is there something wrong with firmware doing it? It looks dirty to me when parts of command set are handled in different places.

2023

It just should not happen if initiator periph driver is sane.

2024

I think any way IOCDBLEN is more appropriate here then 16.

2026

I've never seen such error code to be used before, and I don't see it handled anyhow special. CAM now handles CAM_REQ_INVALID better when some CDB is not supported.

2913

Instead of csio.cdb_io.cdb_bytes there is scsiio_cdb_ptr() macro now to handle CAM_CDB_POINTER case.

[slm]

sys/dev/mpr/mpr.c

1405

I did this change before I added the cpi->maxio change and I forgot about it. I'll change this to use cpi->maxio instead.

sys/dev/mpr/mpr_sas.c

1936

I did not add the UNMAP change and I'm not sure of the background. I'll check with the person who did it to answer your questions in this area.

2023

It probably makes sense to use a KASSERT here, then we can remove this code all together.

2913

OK. We'll probably need to check the FreeBSD version number then, if we MFC this code. And I think there are other places that should change as well.

[ken]

In general this looks good. I agree with mav's comments.

[ken]

sys/dev/mpr/mpr_sas.c

1936

Another question about UNMAP -- do the NVME passthrough commands get single-stepped through the firmware like ATA passthrough commands or are they executed in parallel with other outstanding NVME commands?

[slm]

sys/dev/mpr/mpr_sas.c

1936

FW guy says NVMe commands are typically executed in parallel.

[scottl]

sys/dev/mpr/mpr.c

1005

Something to be done in the future would be to have separate atomic/non-atomic handlers, and eliminate references to the softc. Not a big deal now, but will be important with multi-queue to get to 1Mpps

[scottl]

sys/dev/mpr/mpr_sas.c

2023

It looks like CDB length of 32 is required for EEDP, and I think that this change is going to break that. I'm a little confused why this chunk of code needed to change as part of Tri-mode support.

[slm]

sys/dev/mpr/mpr_sas.c

2023

This part has nothing to do with Tri-Mode. There was a merge conflict with the Current driver and our driver. We fixed this part in one way and Alan fixed it another I think is what happened. We can change it back to the KASSERT and it should be OK.

[slm]

sys/dev/mpr/mpr_sas.c

1936

This is the answer to why the driver issues UNMAP commands instead of FW:

This translation is necessary because in some I/O workloads SCSI Unmap commands can comprise up to 20% of the I/O commands. Issuing these using the MPI SCSI IO request will require F/W to translate the command into an NVMe native command. To maximize performance, OS drivers should translate SCSI Unmap commands to NVMe Encapsulated commands to be handled directly by H/W acceleration.

[mav]

I have no objections aside of ones you already promised to fix.

sys/dev/mpr/mpr_sas.c

1936

Thanks for the answer. Though I can't say that I like it. It creates another rhetorical question: why they should we bother to make firmware to translate other commands and translate this command ourselves?

[slm]

sys/dev/mpr/mpr_sas.c

1936

I don't know for sure, but I assume that we did some testing and determined that, in some cases, if the driver handles the UNMAP command it frees up FW to handle other I/Os. My guess is that this is just a special case where we can predict that a little help from the driver will improve performance, whereas it wouldn't make sense in most other cases. If the driver handled all commands, I think that would hurt performance quite a bit.

[slm]

I'm not sure there is a better way to update my changes without checking them in first, which I didn't do. So, the changes I made from the original review are:

mpr.c:

Line 1404 - add comment
Line 1408 - use sc->maxio

mpr_sas.c:

Line 1031 - set sc->maxio
Line 1716 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 1856 - add scsi_opcode declaration
Line 1948 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 2040 - revert to using KASSERT
Line 2562 - add scsi_cdb pointer declaration
Line 2661 - use scsiio_cdb_ptr macro or check for CAM_CDB_POINTER
Line 2681, 2723, 2818, 2832, 2847, 2939 - use scsi_cdb
Line 2727 - use csio

mprvar.h:

Line 39 - remove MPR_MAX_IO_SIZE
Line 290 - define maxio

[ken]

Looks good to me, thanks!