linux-kernel - Re: [PATCH v4 4/5] scsi: hisi_sas: Add support for DIF feature for v3 hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4752be71-f39c-ac55-0e51-36c146108a38@huawei.com>
Date:   Thu, 13 Dec 2018 13:35:56 +0000
From:   John Garry <john.garry@...wei.com>
To:     "Martin K. Petersen" <martin.petersen@...cle.com>
CC:     <jejb@...ux.vnet.ibm.com>, <linuxarm@...wei.com>,
        <linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        Xiang Chen <chenxiang66@...ilicon.com>
Subject: Re: [PATCH v4 4/5] scsi: hisi_sas: Add support for DIF feature for v3
 hw

On 13/12/2018 02:20, Martin K. Petersen wrote:
>
> John,

Hi Martin,

>
>> +static void fill_prot_v3_hw(struct scsi_cmnd *scsi_cmnd,
>> +			    struct hisi_sas_protect_iu_v3_hw *prot)
>> +{
>> +	u8 prot_type = scsi_get_prot_type(scsi_cmnd);
>> +	u8 prot_op = scsi_get_prot_op(scsi_cmnd);
>> +	unsigned int interval = scsi_prot_interval(scsi_cmnd);
>> +	u32 lbrt_chk_val;
>> +
>> +	if (interval == 4096)
>> +		lbrt_chk_val = (u32)(scsi_get_lba(scsi_cmnd) >> 3);
>> +	else
>> +		lbrt_chk_val = (u32)scsi_get_lba(scsi_cmnd);
>
> lbrt_chk_val = t10_pi_ref_tag(scmd->request);
>

ok

> +
>> +	switch (prot_op) {
>> +	case SCSI_PROT_READ_STRIP:
>> +		prot->dw0 |= (T10_RMV_EN_MSK | T10_CHK_EN_MSK);
>> +		prot->lbrtcv = lbrt_chk_val;
>> +		if (prot_type == SCSI_PROT_DIF_TYPE1)
>> +			prot->dw4 |= (0xc << 16);
>> +		else if (prot_type == SCSI_PROT_DIF_TYPE3)
>> +			prot->dw4 |= (0xfc << 16);
>
> We're moving away from prot_type. You should use:
>
> enum scsi_prot_flags {
>         SCSI_PROT_TRANSFER_PI           = 1 << 0,
>         SCSI_PROT_GUARD_CHECK           = 1 << 1,
>         SCSI_PROT_REF_CHECK             = 1 << 2,
>         SCSI_PROT_REF_INCREMENT         = 1 << 3,
>         SCSI_PROT_IP_CHECKSUM           = 1 << 4,
> };

ok

>
> to set your controller flags.
>
> +		if (prot_op == SCSI_PROT_WRITE_INSERT) {
> +			unsigned int interval = scsi_prot_interval(scsi_cmnd);
> +			unsigned int ilog2_interval = ilog2(interval);
> +
> +			len = (task->total_xfer_len >> ilog2_interval) * 8;
>
> scsi_transfer_length(struct scsi_cmnd *scmd)

ok

>
>> +	if (hisi_hba->enable_dif) {
>> +		dev_info(dev, "Registering for DIF type 1/2/3 protection.\n");
>> +		prot |=	SHOST_DIF_TYPE1_PROTECTION |
>> +			SHOST_DIF_TYPE2_PROTECTION |
>> +			SHOST_DIF_TYPE3_PROTECTION;
>> +	}
>> +
>> +	scsi_host_set_prot(hisi_hba->shost, prot);
>
> I'm not so keen on this enable_dif/enable_dix business in module
> parameters. I suggest you just allow the user to specify the host
> protection mask instead of having a layer of indirection.
>

Fine, we can let the user select the mask. However, for now, I would 
like to keep default at off. When soaks a bit, we can make default on @ 0x7.

Cheers,
John