From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from de-smtp-delivery-102.mimecast.com (de-smtp-delivery-102.mimecast.com [62.140.7.102]) by mx.groups.io with SMTP id smtpd.web10.2825.1594175705058680900 for ; Tue, 07 Jul 2020 19:35:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=mimecast20200619 header.b=g1EOreg0; spf=pass (domain: suse.com, ip: 62.140.7.102, mailfrom: glin@suse.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=mimecast20200619; t=1594175702; 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: in-reply-to:in-reply-to:references:references; bh=WrPk1LPhjAY1OSQRqo2DxLE46BCo1Cpde3gorpdgyc0=; b=g1EOreg02Uvofgu1cwxIJC0Ma1wIXRPxuVnQeSOd/qk85/EhDVdW6Qb0vzFweguffmemRJ Zu8Dw2PT92CtDUkkcfA0P0MCNgvjLXBTx8b9w3KYnCX94qmRlEs9fbAqcWLRsRSnzs2oCT WJifgk2urY2sZWh7GBDcFlODLfmMqb4= Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-am5eur03lp2055.outbound.protection.outlook.com [104.47.8.55]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-14-Z5MY1eDeNmigUo-27rjVdg-1; Wed, 08 Jul 2020 04:35:00 +0200 X-MC-Unique: Z5MY1eDeNmigUo-27rjVdg-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=U9vyLFHpQgujf2SIZ/egq6kv3VmtYLN5ySdkWSLPkwzLC0t23jiR/gdIuKq+eF2RgO1oneCq7OSmY877diTgTqxNnSSG0haP2Mf0wtSjxu3JZ5ikGLMz/m/2pcoU9syxNwbQxVEXAsbMuwz646msUVdqGO2Jz4m86tJBvxTDVjP70/5a5WwJM+mR8MdJb6P2FN71D9iyVu/cAnZGLZYIpnBCek90KMoMuEhd9qX+OuKiP1Q3JVhzkx2rFjgxSg13rNpySCslldeGdKYo+jzofqeRWimoBVAOoSua0aHy0Q2IdScp8yObLm04heNKRUNFWXg4zzGDo15XU8tQyDro4Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=WrPk1LPhjAY1OSQRqo2DxLE46BCo1Cpde3gorpdgyc0=; b=FDkQUAYfB5MnRRLvphJjAF0QW5TPF715G9yevmALxFT1osAj+SmzixBhDEkJjeTBPI1fk/HVdOGZdcDCFvCpy4s1MxYdlMqTyhQeNR45wmhYOCqgkCA22lwKupbuQX6HzDLwNbKY0901SHnQ0+ffCRgCmXGHJwiOmwuEIr49Blr9R8Ncy4tMLOaL7PxiV0sIgFpYdHVgwHVyF40KtffO50HdqkmkPZgH+6bJDoMPxAhXTGEEaXi39FyYvx3iLisT/bgoBKfvhrPbwcG0Jb2eupEtAtk/TUSPpIJFgpisKWw6C7f6H314uV8lcMcHNfYUYIQn4rF1VP4yODORcxEKKg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Authentication-Results: redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=suse.com; Received: from VI1PR0402MB3824.eurprd04.prod.outlook.com (2603:10a6:803:22::18) by VE1PR04MB6464.eurprd04.prod.outlook.com (2603:10a6:803:11f::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3153.28; Wed, 8 Jul 2020 02:34:59 +0000 Received: from VI1PR0402MB3824.eurprd04.prod.outlook.com ([fe80::3854:92a5:f662:2def]) by VI1PR0402MB3824.eurprd04.prod.outlook.com ([fe80::3854:92a5:f662:2def%6]) with mapi id 15.20.3153.029; Wed, 8 Jul 2020 02:34:59 +0000 Date: Wed, 8 Jul 2020 10:34:50 +0800 From: "Gary Lin" To: Laszlo Ersek Cc: devel@edk2.groups.io, Jordan Justen , Ard Biesheuvel Subject: Re: [PATCH 06/11] OvmfPkg/LsiScsiDxe: Report Targets and LUNs Message-ID: <20200708023450.GP18504@GaryWorkstation> References: <20200701040448.14871-1-glin@suse.com> <20200701040448.14871-7-glin@suse.com> In-Reply-To: X-ClientProxiedBy: AM4PR0701CA0023.eurprd07.prod.outlook.com (2603:10a6:200:42::33) To VI1PR0402MB3824.eurprd04.prod.outlook.com (2603:10a6:803:22::18) Return-Path: glin@suse.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from GaryWorkstation (60.251.47.115) by AM4PR0701CA0023.eurprd07.prod.outlook.com (2603:10a6:200:42::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.9 via Frontend Transport; Wed, 8 Jul 2020 02:34:57 +0000 X-Originating-IP: [60.251.47.115] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 5dd47373-6e67-4c5b-1314-08d822e78217 X-MS-TrafficTypeDiagnostic: VE1PR04MB6464: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8273; X-Forefront-PRVS: 04583CED1A X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: WI6Nk/pgrBaW2OjgoDgQ+LKJOxN15EYch4pkmCZuvd+GG09WqmtXrk2gPULv8ZnMEboLgTy2wJcoy6w64cQTh4UOA0cALKWAG2NMyGgSh1ftyfXfu04SExjX6GqVrJurdsdgm+saLELpSCulDOglUclJwauFJ0ZCB64YAhfzX5t1PkZ2DVgq3/pP18tAJSXZ6NFoby3EpcF+rdwAiHPa9QmSW/yx9haVKvKxvF5HEJJV9jIHnXFsVFu/wKRszKlHqUHqkciHVOJLomO3DL1oE5xpEpB/gwcDITBcsqoEDThgrd0o0/4pHDBCloov5qSh/HHFVu07V67goc/l9IpHXg== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VI1PR0402MB3824.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(346002)(376002)(366004)(396003)(136003)(39850400004)(6666004)(55236004)(2906002)(33716001)(4326008)(26005)(16526019)(186003)(83380400001)(9686003)(8936002)(66556008)(33656002)(66476007)(66946007)(5660300002)(53546011)(478600001)(6916009)(54906003)(8676002)(55016002)(52116002)(30864003)(86362001)(956004)(1076003)(6496006)(316002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: zxo9caKys71ULXzNXJhzrVOut7ZTVX4/mBjsf5zxEGotQ5fp7WHPNhG2l8fbdPoAKrmrcdMHX8Y/2q92lUz6t1NMw0U5gvpVLbTaecc7SztsFrye7xK5ERJ0PaALZraPb8y8/YFBdGIyh/lutNUvh9fp9kNwlyXrig/r5VhqnBozKGR3iYkqNOHCJ4r/h2EXRAgD+j5M5Uou0GENtNiFihQR3mxD9BNhuCWIYDtsVQN9kHAOL29jypqTGZiIXGjSkJe0sGOxQSh76312iWjyJCdgXhS15seZuWQDTsqmpxNDjeTnsAgUoK9pHOreQzh3ROMi93PM53DzlE6EGVZCgXXuUF6DlEs1kNlCCtIvYyy0icTa4sxflfSxOGeWz9b8Y4QPXdO4uSiMPif7c6Vsow/QkXiAKu7+xh9ELHbJB4AFiAwUYCeeroW7rAT3IxAxMVkQGnlEYC0vctSabpJNpxPBBoibeojQFrG1IpAODzE= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5dd47373-6e67-4c5b-1314-08d822e78217 X-MS-Exchange-CrossTenant-AuthSource: VI1PR0402MB3824.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jul 2020 02:34:58.9857 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: X7glMczoZ4m4pIFf6hwbbThcO9k50aPq3HesQQ+p5eSW+RkbX2bMyWBCae2BmAFC X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6464 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jul 07, 2020 at 11:04:21AM +0200, Laszlo Ersek wrote: > On 07/01/20 06:04, Gary Lin wrote: > > Implement LsiScsiGetNextTargetLun(), LsiScsiBuildDevicePath(), > > LsiScsiGetTargetLun(), and LsiScsiGetNextTarget() to report Targets and > > LUNs and build the device path. > > > > This commit also introduces two PCD value: PcdLsiScsiMaxTargetLimit and > > PcdLsiScsiMaxLunLimit as the limits for Targets and LUNs. > > > > Cc: Jordan Justen > > Cc: Laszlo Ersek > > Cc: Ard Biesheuvel > > Signed-off-by: Gary Lin > > --- > > OvmfPkg/LsiScsiDxe/LsiScsi.c | 143 +++++++++++++++++++++++++++++- > > OvmfPkg/LsiScsiDxe/LsiScsi.h | 3 + > > OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 6 ++ > > OvmfPkg/OvmfPkg.dec | 5 ++ > > 4 files changed, 155 insertions(+), 2 deletions(-) > > > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c > > index f633c6793298..e10a81a5f9f7 100644 > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c > > @@ -15,6 +15,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -53,6 +54,49 @@ LsiScsiGetNextTargetLun ( > > IN OUT UINT64 *Lun > > ) > > { > > + LSI_SCSI_DEV *Dev; > > + UINTN Idx; > > + UINT8 *Target; > > + UINT16 LastTarget; > > + > > + // > > + // the TargetPointer input parameter is unnecessarily a pointer-to-pointer > > + // > > + Target = *TargetPointer; > > + > > + // > > + // Search for first non-0xFF byte. If not found, return first target & LUN. > > + // > > + for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx) > > + ; > > + if (Idx == TARGET_MAX_BYTES) { > > + SetMem (Target, TARGET_MAX_BYTES, 0x00); > > + *Lun = 0; > > + return EFI_SUCCESS; > > + } > > + > > + CopyMem (&LastTarget, Target, sizeof LastTarget); > > + > > + // > > + // increment (target, LUN) pair if valid on input > > + // > > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > > + if (LastTarget > Dev->MaxTarget || *Lun > Dev->MaxLun) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (*Lun < Dev->MaxLun) { > > + ++*Lun; > > + return EFI_SUCCESS; > > + } > > + > > + if (LastTarget < Dev->MaxTarget) { > > + *Lun = 0; > > + ++LastTarget; > > + CopyMem (Target, &LastTarget, sizeof LastTarget); > > + return EFI_SUCCESS; > > + } > > + > > return EFI_NOT_FOUND; > > } > > > > @@ -65,7 +109,34 @@ LsiScsiBuildDevicePath ( > > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > > ) > > { > > - return EFI_NOT_FOUND; > > + UINT16 TargetValue; > > + LSI_SCSI_DEV *Dev; > > + SCSI_DEVICE_PATH *ScsiDevicePath; > > + > > + if (DevicePath == NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + CopyMem (&TargetValue, Target, sizeof TargetValue); > > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > > + if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun || Lun > 0xFFFF) { > > + return EFI_NOT_FOUND; > > + } > > + > > + ScsiDevicePath = AllocatePool (sizeof *ScsiDevicePath); > > + if (ScsiDevicePath == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + ScsiDevicePath->Header.Type = MESSAGING_DEVICE_PATH; > > + ScsiDevicePath->Header.SubType = MSG_SCSI_DP; > > + ScsiDevicePath->Header.Length[0] = (UINT8) sizeof *ScsiDevicePath; > > + ScsiDevicePath->Header.Length[1] = (UINT8) (sizeof *ScsiDevicePath >> 8); > > + ScsiDevicePath->Pun = TargetValue; > > + ScsiDevicePath->Lun = (UINT16) Lun; > > + > > + *DevicePath = &ScsiDevicePath->Header; > > + return EFI_SUCCESS; > > } > > > > EFI_STATUS > > @@ -77,7 +148,36 @@ LsiScsiGetTargetLun ( > > OUT UINT64 *Lun > > ) > > { > > - return EFI_UNSUPPORTED; > > + SCSI_DEVICE_PATH *ScsiDevicePath; > > + LSI_SCSI_DEV *Dev; > > + UINT8 *Target; > > + > > + if (DevicePath == NULL || TargetPointer == NULL || *TargetPointer == NULL || > > + Lun == NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (DevicePath->Type != MESSAGING_DEVICE_PATH || > > + DevicePath->SubType != MSG_SCSI_DP) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + ScsiDevicePath = (SCSI_DEVICE_PATH *) DevicePath; > > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > > + if (ScsiDevicePath->Pun > Dev->MaxTarget || > > + ScsiDevicePath->Lun > Dev->MaxLun) { > > + return EFI_NOT_FOUND; > > + } > > + > > + // > > + // This device support 8 targets only, so it's enough to set the LSB > > + // of Target. > > + // > > + Target = *TargetPointer; > > + *Target = (UINT8)ScsiDevicePath->Pun; > > I don't like this shortcut. > > The first reason is that the shortcut is not consistent with the rest of > the functions implemented in this patch. The other functions handle > target numbers that extend up to two bytes; there are no single-byte > shortcuts in them. > > (1) So please either rewrite all of those functions to use a similar > shortcut like here, or please remove this shortcut, and stick with a > 2-byte sized CopyMem() here too. I prefer the latter. > > (2) Whether you use the single-byte shortcut or not, please zero out the > bytes in (*Target) that you do not set otherwise (from Pun). > > (3) If the device supports only 8 targets, then please add an according > STATIC_ASSERT() on PcdLsiScsiMaxTargetLimit somewhere. For example: > > STATIC_ASSERT ( > FixedPcdGet8 (PcdLsiScsiMaxTargetLimit) < 8, > "LSI 53C895A supports targets [0..7]" > ); > > It's probably best to add the assert in LsiScsiControllerStart() below, > where you fetch the PCD anyway. > Will change it to CopyMem() and add an assert to make sure MaxTarget is always below 8. > > + *Lun = ScsiDevicePath->Lun; > > + > > + return EFI_SUCCESS; > > } > > > > EFI_STATUS > > @@ -107,6 +207,42 @@ LsiScsiGetNextTarget ( > > IN OUT UINT8 **TargetPointer > > ) > > { > > + LSI_SCSI_DEV *Dev; > > + UINTN Idx; > > + UINT8 *Target; > > + UINT16 LastTarget; > > + > > + // > > + // the TargetPointer input parameter is unnecessarily a pointer-to-pointer > > + // > > + Target = *TargetPointer; > > + > > + // > > + // Search for first non-0xFF byte. If not found, return first target. > > + // > > + for (Idx = 0; Idx < TARGET_MAX_BYTES && Target[Idx] == 0xFF; ++Idx) > > + ; > > + if (Idx == TARGET_MAX_BYTES) { > > + SetMem (Target, TARGET_MAX_BYTES, 0x00); > > + return EFI_SUCCESS; > > + } > > + > > + CopyMem (&LastTarget, Target, sizeof LastTarget); > > + > > + // > > + // increment target if valid on input > > + // > > + Dev = LSI_SCSI_FROM_PASS_THRU (This); > > + if (LastTarget > Dev->MaxTarget) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (LastTarget < Dev->MaxTarget) { > > + ++LastTarget; > > + CopyMem (Target, &LastTarget, sizeof LastTarget); > > + return EFI_SUCCESS; > > + } > > + > > return EFI_NOT_FOUND; > > } > > > > @@ -189,6 +325,9 @@ LsiScsiControllerStart ( > > > > Dev->Signature = LSI_SCSI_DEV_SIGNATURE; > > > > + Dev->MaxTarget = PcdGet8 (PcdLsiScsiMaxTargetLimit); > > + Dev->MaxLun = PcdGet8 (PcdLsiScsiMaxLunLimit); > > + > > // > > // Host adapter channel, doesn't exist > > // > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h > > index fca1007f9b98..a3d51d8f2386 100644 > > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h > > @@ -14,6 +14,8 @@ > > > > typedef struct { > > UINT32 Signature; > > + UINT8 MaxTarget; > > + UINT8 MaxLun; > > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > > } LSI_SCSI_DEV; > > @@ -23,6 +25,7 @@ typedef struct { > > #define LSI_SCSI_FROM_PASS_THRU(PassThruPtr) \ > > CR (PassThruPtr, LSI_SCSI_DEV, PassThru, LSI_SCSI_DEV_SIGNATURE) > > > > + > > // > > // Probe, start and stop functions of this driver, called by the DXE core for > > // specific devices. > > (4) Spurious newline, please drop it. (Or squash it in the previous > patch -- for example --, where you define the LSI_SCSI_FROM_PASS_THRU > macro.) > Will remove the newline. > > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > index 8660513e2ffd..68844c6772e3 100644 > > --- a/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > +++ b/OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf > > @@ -27,7 +27,9 @@ [Packages] > > [LibraryClasses] > > BaseLib > > BaseMemoryLib > > + DebugLib > > MemoryAllocationLib > > + PcdLib > > UefiBootServicesTableLib > > UefiDriverEntryPoint > > UefiLib > > @@ -35,3 +37,7 @@ [LibraryClasses] > > [Protocols] > > gEfiExtScsiPassThruProtocolGuid ## BY_START > > gEfiPciIoProtocolGuid ## TO_START > > + > > +[FixedPcd] > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit ## CONSUMES > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit ## CONSUMES > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > > index 65bb2bb0eb4c..ae7d1d648d22 100644 > > --- a/OvmfPkg/OvmfPkg.dec > > +++ b/OvmfPkg/OvmfPkg.dec > > @@ -174,6 +174,11 @@ [PcdsFixedAtBuild] > > ## Microseconds to stall between polling for MptScsi request result > > gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x40 > > > > + ## Set the *inclusive* number of targets and LUNs that LsiScsi exposes for > > + # scan by ScsiBusDxe. > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxTargetLimit|7|UINT8|0x3b > > + gUefiOvmfPkgTokenSpaceGuid.PcdLsiScsiMaxLunLimit|0|UINT8|0x3c > > + > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa > > > > Commit 093cceaf79b5 ("OvmfPkg/MptScsiDxe: Report targets and one LUN", > 2020-05-05) introduced PcdMptScsiMaxTargetLimit with token value 0x39. > That was OK. > > Commit 505812ae1d2d ("OvmfPkg/MptScsiDxe: Implement the PassThru > method", 2020-05-05) then introduced PcdMptScsiStallPerPollUsec with > token value 0x40. That was not OK -- but I missed it. (The token value > should have been 0x3a; as 0x39 is not succeeded by 0x40 but by 0x3a.) > > Token value 0x3a remains unused at this point. > > (5) So please introduce the new PCDs with token values 0x3a and 0x3b, to > keep the token space dense. > I actually was thinking about making PcdMptScsiStallPerPollUsec 0x3a in another patch. Since it's not related to LsiScsi, I didn't do it in this series. Sounds like you prefer to keeping 0x40 for PcdMptScsiStallPerPollUsec? Gary Lin