From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web11.22787.1585343163651701870 for ; Fri, 27 Mar 2020 14:06:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LMuagliI; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585343162; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rnrvSb+imaNxSeTzOtkVXbauv8C6WiXShjBYbWqCHZQ=; b=LMuagliIB0AFH1hFvrto4M63ZPt6/6wLG9YaLU6xKhfvB+qaV+DkDtlP719Wsyqgya1FOp PlYKxLwPPhSEREAKu9U9HYSyf3czqikHGzQdiWCVEhfFMcddUbi3s5ow0opGYVgIx8ho+b LWlKJBbWEHEzb3LNzdsqGcX+hayL4Jk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-92-G3wXVr5qO8qZr6rY_Kk69A-1; Fri, 27 Mar 2020 17:06:00 -0400 X-MC-Unique: G3wXVr5qO8qZr6rY_Kk69A-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D76CF1005516; Fri, 27 Mar 2020 21:05:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-36.ams2.redhat.com [10.36.114.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2DE097E303; Fri, 27 Mar 2020 21:05:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response To: Liran Alon , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-16-liran.alon@oracle.com> <6f32838a-d136-050e-7a05-f817da7954a8@oracle.com> <50ebcafb-b0c2-f948-6adb-6e533d3ed3ba@oracle.com> From: "Laszlo Ersek" Message-ID: <32b3a064-3074-eeda-22fe-1d01310a71fa@redhat.com> Date: Fri, 27 Mar 2020 22:05:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <50ebcafb-b0c2-f948-6adb-6e533d3ed3ba@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 03/27/20 14:20, Liran Alon wrote: >=20 > On 27/03/2020 16:04, Liran Alon wrote: >> >> On 27/03/2020 14:26, Laszlo Ersek wrote: >>> On 03/25/20 17:10, Liran Alon wrote: >>> >>>> + >>>> +=A0 // >>>> +=A0 // Report target status >>>> +=A0 // >>>> +=A0 Packet->TargetStatus =3D Response->ScsiStatus; >>>> + >>>> +=A0 // >>>> +=A0 // Host adapter status and function return value depend on >>>> +=A0 // device response's host status >>>> +=A0 // >>>> +=A0 switch (Response->HostStatus) { >>>> +=A0=A0=A0 case PvScsiBtStatSuccess: >>>> +=A0=A0=A0 case PvScsiBtStatLinkedCommandCompleted: >>>> +=A0=A0=A0 case PvScsiBtStatLinkedCommandCompletedWithFlag: >>>> +=A0=A0=A0=A0=A0 Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOS= T_ADAPTER_OK; >>>> +=A0=A0=A0=A0=A0 return EFI_SUCCESS; >>>> + >>>> +=A0=A0=A0 case PvScsiBtStatSelTimeout: >>>> +=A0=A0=A0=A0=A0 Packet->HostAdapterStatus =3D >>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT; >>>> +=A0=A0=A0=A0=A0 return EFI_TIMEOUT; >>>> + >>>> +=A0=A0=A0 case PvScsiBtStatDatarun: >>>> +=A0=A0=A0 case : >>>> +=A0=A0=A0=A0=A0 // >>>> +=A0=A0=A0=A0=A0 // Report residual data in overrun/underrun >>>> +=A0=A0=A0=A0=A0 // >>>> +=A0=A0=A0=A0=A0 if (Packet->DataDirection =3D=3D EFI_EXT_SCSI_DATA_DI= RECTION_READ) { >>>> +=A0=A0=A0=A0=A0=A0=A0 Packet->InTransferLength =3D Response->DataLen; >>>> +=A0=A0=A0=A0=A0 } else { >>>> +=A0=A0=A0=A0=A0=A0=A0 Packet->OutTransferLength =3D Response->DataLen= ; >>>> +=A0=A0=A0=A0=A0 } >>> OK, if we are sure that (a) the device will always report short >>> reads/writes like this, and that (b) the above assignments will never >>> cause InTransferLength / OutTransferLength to *grow*, then the >>> InTransferLength / OutTransferLength adjustments are sufficiently >>> covered. >> I believe both of these are indeed true. >> Even though that current QEMU VMware PVSCSI device emulation code have >> a bug that it never sets this in pvscsi_command_complete() when it >> does set BTSTAT_DATARUN... >>> Still: >>> >>> (8) The CopyMem() call above should not copy garbage (at the tail). >> I don't think it matters. We don't guarantee anything on the content >> in Packet->InDataBuffer beyond Packet->InTransferLength. >> I think the code is simpler how it is currently written. >>> >>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen", >> I don't think this is the case. >>> then I would prefer if: >>> >>> - we always updated InTransferLength / OutTransferLength (regardless of >>> "Response->HostStatus"), >>> >>> - and we only used these case labels (PvScsiBtStatDatarun / >>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus". >=20 > Regarding all the above: > You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c) > reads the "DataLen" field only in case the "HostStatus" is > BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN. > As I have done in my driver. In the lack of more detailed device > specification (As PVSCSI is a proprietary VMware PV device), I prefer to > remain with my implementation which seems > to be guaranteed to be safe and working. Please tell me if you think > otherwise. No, I'm fine with your reasoning. Thanks, Laszlo