Re: CAM Shingled Disk support patches available

> On Mar 2, 2016, at 2:25 PM, Kenneth D. Merry <ken_at_FreeBSD.ORG> wrote:
> 
>> 
>> 
>> void scsi_ata_pass(struct ccb_scsiio *csio, u_int32_t retries,
>>                      void (*cbfcnp)(struct cam_periph *, union ccb *),
>>                      u_int32_t flags, u_int8_t tag_action,
>>                      struct ata_cmd *cmd, struct ata_res *res,
>>                      u_int8_t *data_ptr, u_int32_t dxfer_len,
>>                      u_int8_t *data_ptr, u_int16_t dxfer_len,
> 
> I assume you only intended one line there, not two. :)
> 

Indeed =-)

>>                      u_int8_t sense_len, u_int32_t timeout);
>> 
>> To differentiate between the 12 and 16 byte variants, you???d look at the
>> AP_EXTEND flag in the protocol field.  Btw, the handling of that flag is
>> inconsistent in the implementation of the existing scsi_ata_pass_16().  If
>> the caller providse an ata_res pointer then it gets filled on completion,
>> otherwise the caller does its best to look at 12.2.2.6 and extract what it
>> can from the sense descriptor.
>> 
>> So my proposal is to create a new scsi_ata_pass and deprecate but not remove
>> scsi_ata_pass_16.  Tell people that if they need to use it, dxfer_len is going to
>> have lower priority than sector_count/sector_count_exp if the latter multiply to
>> more than 65535.
> 
> In general I think that's a reasonable idea, but we should probably go
> further.
> 
> While we're at it, we should figure out what we need to do to add the
> Auxiliary register to struct ata_cmd.  We'll need that to do the NCQ
> versions of the various SMR commands, as well as TRIM.
> 

Warner and I talked about this, and I thought that at one point we had a patch
that defined a ‘struct ata_cmd_aux’.  As with function signatures, I’m very
much against redefining structures, and I think it’s reasonable to create a
new structure for what’s basically a late addition to the specs.  I need to read
more of the draft ACS and SATA specs to see if there’s anything else on the
horizon that should also be included.  However, and I talk a little bit about
this below, I think the situation is a bit more complicated than just adding
a field to the structure.  The ata_cmd structure mostly models what’s in an
ATA taskfile, and the taskfile definition, whether from ACS or APT, has never
been expanded to include the aux (and aux_exp) registers.  They exist only
in SATA as part of the Device-to-Host (D2H) FIS.  However, I’m kinda back
and forth on this; maybe my interpretation of ata_cmd is too strict, and we
can stick in whatever is needed and let the SIM deal with it.

At one point I had some patches that defined the
various FIS packets and allowed the periphs to send in an XPT_SATA_FIS
instead of an XPT_ATA_IO, but it seemed to not provide much value since
most drivers (and hardware) still operate in terms of legacy ATA taskfiles.
It also wasn’t clear in the scheme of driving I/O via a FIS where the
responsibility of the periph stopped and the SIM started; If the periph
sends a H2D FIS to initiate an I/O, does it need to then drive the PIO
and/or DMA FIS’s as well?  The FIS is really in the realm of the transport,
not the protocol, and it’s a huge shame that ATA is starting to blur the lines
by having the FIS Aux registers be part of the protocol.

> The obvious challenge is that probably means changing the existing struct
> ccb_ataio CCB and bumping the CAM version.  At least that will be source
> compatible, but will require ifdefs if people want to compile on older
> versions of FreeBSD.  But in that case, they'll also be faced with no
> support for sending the NCQ versions of the commands, anyway.  No way
> around that, though, since we have to follow the changing specs.
> 

Again, not a fan of redefining the structures.

A couple of other thoughts here.  Steve Hartland was right about not abusing
the AP_EXTEND flag.  What are your thoughts on having a new flag
in the function to signal 12 vs 16 byte variants?  Also do you have any thoughts
on the existing arguments?  I’m not sure that tag_action has ever made any
sense for the ATA transport, there’s no such a thing as ordered tags in ATA
and we don’t expose the NCQ tag number outside of the SIM.
MSG_SIMPLE_Q_TAG definitely has no meaning in ATA.  I think the
argument could go away.

Second, I’m not sure that cam_ata_pass (or cam_ata_pass_16, for that matter)
is the right place to include the aux register.  My reading is that it’s implementing
the 12.2.2.x chapter of SAT-3, and that doesn’t have any allowance for the
aux register.  SAT-4 r.04a doesn’t seem to mention this register either.  The
register seems to only be exposed when you have access to creating a H2D FIS,
and SAT is pretty much silent on that.  IIRC, when Warner implemented
NCQ TRIM, which required the Aux register, it could only be used on devices
attached via AHCI; LSI controllers had no way of expressing the register.  Still,
we could add this ad-hoc to cam_ata_pass().  Maybe instead of it taking an
ata_cmd struct, it takes a union of ata_cmd and ata_cmd_aux, and we add an
argument to the function that tells it what is in the union.  Maybe the union
could also include a H2D FIS?

Anyways, here’s a possibility:

union ata_cmds {
	struct ata_cmd		cmd;
	struct ata_cmd_aux		cmd_aux; 	/* Don’t forget that this includes both aux registers! */
	struct ata_fis_h2d		fis;
};

#define ATA_FORMAT_CMD		0x01
#define ATA_FORMAT_CMD_AUX	0x02
#define ATA_FORMAT_FIS_H2D	0x03

void scsi_ata_pass(struct ccb_scsiio *csio, u_int32_t retries,
                     void (*cbfcnp)(struct cam_periph *, union ccb *),
                     u_int32_t flags, u_int format,
                     union ata_cmds *cmd, struct ata_res *res,
                     u_int8_t *data_ptr, u_int32_t dxfer_len,
                     u_int8_t *data_ptr, u_int16_t dxfer_len,
                     u_int8_t sense_len, u_int32_t timeout);

Scott


> Ken
> -- 
> Kenneth Merry
> ken_at_FreeBSD.ORG