nvme: Tidy up transfer rate settings in XPT_GET_TRAN_SETTINGS.

[jhb]

Summary

• Replace a magic number with CTS_NVME_VALID_SPEC.

• Set the transport and protocol versions the same as for XPT_PATH_INQ.

Probably we shouldn't bother with setting the version in the 'spec'
member of ccb_trans_settings_nvme at all and use the transport
and/or protocol version field instead.

[imp]

Yea, we're setting the spec in the nvme protocol or transports in the nvme specific part of the cts... But I agree. I'm not sure how that would be different than the protocol spec at least (we set the two to be the same anyway. It's likely redundant since I wasn't sure cts->*version was the same as the spec when I first did this, but now I kinda think they are the same.

We could set nvmex->max_xfer, but don't. We se tit in the PATH_INQ, but not there, we likely should. I'm not sure if it's needed for the protocol or not, but it likely is so should likely also be set in nvmep as well. Though we should define a new bit for it to say it's valid. We'd likely want to enhance the camcontrol rate/negotiate command to print it too.

if we stop setting nvmep->spec and nvmex->spec, we can stop setting VALID_SPEC, retire that bit and then re-use that field. But camcontrol uses it today to know if it should print it in rate/negotiate commands.

camcontrol donesn't look at protocol_version or transport_version at all, weirdly. It prints the ata spec version, but we don't encode that in protocol or transport version (ahci only sets it to UNSPECIFIED). scsi does encode it in what looks to be all the sims. ata will set it on the device to be what's in the ident buffer or inq buffer (depending on if the device talks ATA or ATAPI/SCSI). both the scsi and ata xpt layers pull it out like this. nvme just encodes the device as what the sim says, which makes sense because there's currently no way for it to differ like in scsi/ata. I didn't look at mmc.

Which is a long way of saying 'lgtm' but more refinement might be needed as this appears to be documented only really well in code.

sys/dev/nvme/nvme_sim.c

245

are things correct enough to ditch this comment?

[jhb]

In D40616#924832, @imp wrote:

Yea, we're setting the spec in the nvme protocol or transports in the nvme specific part of the cts... But I agree. I'm not sure how that would be different than the protocol spec at least (we set the two to be the same anyway. It's likely redundant since I wasn't sure cts->*version was the same as the spec when I first did this, but now I kinda think they are the same.

I think for base NVMe they should be the same. In theory Fabrics could use a different transport version, but Fabrics also just uses the same version number, so I think they are the same for both there as well.

We could set nvmex->max_xfer, but don't. We se tit in the PATH_INQ, but not there, we likely should. I'm not sure if it's needed for the protocol or not, but it likely is so should likely also be set in nvmep as well. Though we should define a new bit for it to say it's valid. We'd likely want to enhance the camcontrol rate/negotiate command to print it too.

Do we need another copy of the same value if it's already returned from PATH_INQ? It wasn't clear to me what the value of returning duplicate info is here.

if we stop setting nvmep->spec and nvmex->spec, we can stop setting VALID_SPEC, retire that bit and then re-use that field. But camcontrol uses it today to know if it should print it in rate/negotiate commands.

For camcontrol I think we could check if the protocol_version != 0 and if so use it. We also have plenty of bits available so could just leave VALID_SPEC retired and use other bits for new fields.

camcontrol donesn't look at protocol_version or transport_version at all, weirdly. It prints the ata spec version, but we don't encode that in protocol or transport version (ahci only sets it to UNSPECIFIED). scsi does encode it in what looks to be all the sims. ata will set it on the device to be what's in the ident buffer or inq buffer (depending on if the device talks ATA or ATAPI/SCSI). both the scsi and ata xpt layers pull it out like this. nvme just encodes the device as what the sim says, which makes sense because there's currently no way for it to differ like in scsi/ata. I didn't look at mmc.

Which is a long way of saying 'lgtm' but more refinement might be needed as this appears to be documented only really well in code.

I think I'd like to actually split ccb_trans_settings_nvme into two structures so that the protocol and xport can have different structures. The xport one would just be for things specific to NVMe over PCIe to include things like the link details. The protocol one would include things common to PCIe and Fabrics which currently would be the version (but I think we don't need that and should just use protocol_version going forward) and maybe max_xfer if we really do need that here as well as in PATH_INQ.

For Fabrics I've added a new ccb_transs_settings_nvmf as part of the Fabrics-specific XPORT_NVMF. Currently the only thing the fabrics SIM sets for the protocol side of ccb_trans_settings_nvme is SPEC_VERSION.