From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web12.665.1583365338004177934 for ; Wed, 04 Mar 2020 15:42:18 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@oracle.com header.s=corp-2020-01-29 header.b=fnoqfGa+; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 024NcFCe017551; Wed, 4 Mar 2020 23:42:17 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=m5sM9YOeYlqgHybCWm/QiaxkTuxLOcvxHgP0Ru39kkY=; b=fnoqfGa+Dv7ymWbSras83IZtFjKQ1exSg48VZpfBcOnQCLJCnqDMb+/x1aWCaUrJP1lQ EvLzGr1dYHJ2tsjDswiJR7K6ykFnOoWtCQ9s5IP9BpWm2tdL58dLXzOaSUzwvfdeDSET DTZwxTA3O5c9gN/CBNm9t4mTuYQ2r7KbPpwMNuS+Za4CCBBsJHG08P7R6pAqsCiD7KeS rAV1iY+xogbpj/pLRqqjzGz/i1iO9emx0nhlwh93ORYk1i8mLvwb9sNS74jZ0s1QRMPX lxZdC0QM/6koY6Ukn7iOCYciIkEDTIo/z28twmmOWAtwgCqrwVKB0ae7qXCyRjdceW0L eQ== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 2yffwr1knp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 04 Mar 2020 23:42:17 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 024Nagl3158374; Wed, 4 Mar 2020 23:42:16 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3030.oracle.com with ESMTP id 2yg1er3ms5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 04 Mar 2020 23:42:16 +0000 Received: from abhmp0019.oracle.com (abhmp0019.oracle.com [141.146.116.25]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 024NgDfi031166; Wed, 4 Mar 2020 23:42:15 GMT Received: from [192.168.14.112] (/79.177.218.220) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 04 Mar 2020 15:42:13 -0800 Subject: Re: [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU To: Nikita Leshenko , devel@edk2.groups.io Cc: aaron.young@oracle.com, jordan.l.justen@intel.com, lersek@redhat.com, ard.biesheuvel@linaro.org References: <20200304192257.96736-1-nikita.leshchenko@oracle.com> <20200304192257.96736-6-nikita.leshchenko@oracle.com> From: Liran Alon Message-ID: Date: Thu, 5 Mar 2020 01:42:08 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200304192257.96736-6-nikita.leshchenko@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 malwarescore=0 bulkscore=0 mlxscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003040150 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 spamscore=0 impostorscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 phishscore=0 clxscore=1015 bulkscore=0 adultscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003040150 X-MIME-Autoconverted: from 8bit to quoted-printable by aserp2120.oracle.com id 024NcFCe017551 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 04/03/2020 21:22, Nikita Leshenko wrote: > Support dynamic insertion and removal of the protocol > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2390 > Signed-off-by: Nikita Leshenko > Reviewed-by: Laszlo Ersek > --- > OvmfPkg/MptScsiDxe/MptScsi.c | 179 +++++++++++++++++++++++++++++= - > OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 +- > 2 files changed, 181 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.= c > index 07f8fe267fc2..9598b82fda53 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -17,9 +17,12 @@ > =20 > #include > #include > +#include > +#include > #include > #include > #include > +#include > =20 > // > // Higher versions will be used before lower, 0x10-0xffffffef is the = version > @@ -27,6 +30,109 @@ > // > #define MPT_SCSI_BINDING_VERSION 0x10 > =20 > +// > +// Runtime Structures > +// > + > +#define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S') > +typedef struct { > + UINT32 Signature; > + EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > + EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > +} MPT_SCSI_DEV; > + > +#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ > + CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE) > + Nit: I personally prefer to put these structures & macros on a dedicated=20 header file for driver. i.e. OvmfPkg/MptScsiDxe/MptScsi.h. Similar to how this is for VirtioScsi (And also upcoming PvScsi)=20 implementation. > +// > +// Ext SCSI Pass Thru > +// > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiPassThru ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN UINT8 *Target, > + IN UINT64 Lun, > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet, > + IN EFI_EVENT Event OPTIONAL > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiGetNextTargetLun ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN OUT UINT8 **Target, > + IN OUT UINT64 *Lun > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiGetNextTarget ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN OUT UINT8 **Target > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiBuildDevicePath ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN UINT8 *Target, > + IN UINT64 Lun, > + IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiGetTargetLun ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN EFI_DEVICE_PATH_PROTOCOL *DevicePath, > + OUT UINT8 **Target, > + OUT UINT64 *Lun > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiResetChannel ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +MptScsiResetTargetLun ( > + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This, > + IN UINT8 *Target, > + IN UINT64 Lun > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > // > // Driver Binding > // > @@ -95,7 +201,49 @@ MptScsiControllerStart ( > IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTI= ONAL > ) > { > - return EFI_UNSUPPORTED; > + EFI_STATUS Status; > + MPT_SCSI_DEV *Dev; > + > + Dev =3D AllocateZeroPool (sizeof (*Dev)); > + if (Dev =3D=3D NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + Dev->Signature =3D MPT_SCSI_DEV_SIGNATURE; > + > + // > + // Host adapter channel, doesn't exist > + // > + Dev->PassThruMode.AdapterId =3D MAX_UINT32; > + Dev->PassThruMode.Attributes =3D > + EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL > + | EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL; Shouldn't "|" be on the line before? > + > + Dev->PassThru.Mode =3D &Dev->PassThruMode; > + Dev->PassThru.PassThru =3D &MptScsiPassThru; > + Dev->PassThru.GetNextTargetLun =3D &MptScsiGetNextTargetLun; > + Dev->PassThru.BuildDevicePath =3D &MptScsiBuildDevicePath; > + Dev->PassThru.GetTargetLun =3D &MptScsiGetTargetLun; > + Dev->PassThru.ResetChannel =3D &MptScsiResetChannel; > + Dev->PassThru.ResetTargetLun =3D &MptScsiResetTargetLun; > + Dev->PassThru.GetNextTarget =3D &MptScsiGetNextTarget; > + > + Status =3D gBS->InstallProtocolInterface ( > + &ControllerHandle, > + &gEfiExtScsiPassThruProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &Dev->PassThru > + ); > + if (EFI_ERROR (Status)) { > + goto FreePool; > + } > + > + return EFI_SUCCESS; > + > +FreePool: > + FreePool (Dev); > + > + return Status; > } > =20 > STATIC > @@ -108,7 +256,34 @@ MptScsiControllerStop ( > IN EFI_HANDLE *ChildHandleBuffer > ) > { > - return EFI_UNSUPPORTED; > + EFI_STATUS Status; > + EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru; > + MPT_SCSI_DEV *Dev; > + > + Status =3D gBS->OpenProtocol ( > + ControllerHandle, > + &gEfiExtScsiPassThruProtocolGuid, > + (VOID **)&PassThru, > + This->DriverBindingHandle, > + ControllerHandle, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL // Lookup only > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Dev =3D MPT_SCSI_FROM_PASS_THRU (PassThru); > + > + Status =3D gBS->UninstallProtocolInterface ( > + ControllerHandle, > + &gEfiExtScsiPassThruProtocolGuid, > + &Dev->PassThru > + ); > + ASSERT_EFI_ERROR (Status); I think this should be replaced with: if (EFI_ERROR (Status)) { =C2=A0 return Status; } Because otherwise, next line will free Dev->PassThru memory while the=20 protocol is still registered. This is also consistent with VirtioScsi implementation. > + > + FreePool (Dev); > + > + return Status; > } > =20 > STATIC > diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/Mpt= ScsiDxe.inf > index 105af20f325f..a253c5d96916 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > @@ -30,9 +30,12 @@ [Packages] > OvmfPkg/OvmfPkg.dec > =20 > [LibraryClasses] > + DebugLib > + MemoryAllocationLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > =20 > [Protocols] > - gEfiPciIoProtocolGuid ## TO_START > + gEfiPciIoProtocolGuid ## TO_START > + gEfiExtScsiPassThruProtocolGuid ## BY_START