From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::231; helo=mail-it0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x231.google.com (mail-it0-x231.google.com [IPv6:2607:f8b0:4001:c0b::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1F3C120352A84 for ; Fri, 1 Dec 2017 09:30:27 -0800 (PST) Received: by mail-it0-x231.google.com with SMTP id d137so3265533itc.2 for ; Fri, 01 Dec 2017 09:34:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=jXSgRx3FcXRqRG0m3TvJ3Xhr/Vk6NlrUnmPwXKEZfl8=; b=ZduIcLy4vilyXb6Q2Q0vU7UzmjeiWUveE5gZOSTVqWktV2qg7qy7Mpt6XEXX/h+yj+ 2/kC2vQ0fjj4nFJxWV55XY1CMZg7NknPPooIeeQoc0Vll2BlwPbCRj2kFPezxK3EPVin s1rwbwqVcwXum1gciDKF5yFnRyACXVAsMEW8U= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=jXSgRx3FcXRqRG0m3TvJ3Xhr/Vk6NlrUnmPwXKEZfl8=; b=psoFn9kG0r6bdYQxmwFNMIY7xrFwsxjZ3TN1QKxhWy6caBZLpSC1laPZzjxWH6cEiF 7lcqUw/qX2bcqsVXsmSv2ggLKSWrhaKah39x8z4EkbJ/jjRNzLFIusj0U53qPGZokh3s JCx76I3ZLqJynHT+QwfzM1CH5sR/D9GCFUcsP9xi47VIsHRhHC4CGFOs8sOyKqc7Tdj3 rzsGEANOtVnnO8JuweNn5swJzhAHyJeWe5upA7d0nmzBypqjREQYnU3p6O/uADTGzKgD 3Fsyz5wcNV/BwkMCHmHUsUo216uerqouDKuGGnoHXCJF7zhBcL2Lg2Ad4KTqwOfIDTG8 5NAA== X-Gm-Message-State: AJaThX778YfZctzhA+Y4OsVIn/MCBegiHUAZlXmh7liJQ9F8DGyJqF6O Ug08ETbbD1n062QiuV/jF27vPHsQ0oC+gl3WlvjVVmN8 X-Google-Smtp-Source: AGs4zMZOcyAXWbc9zs7pTczbZranjmbCWSRMO8E05ntjKy39BSfhCIHJl4G76DnKXIhVNft7GBli9cmvEJVM2x3yj84= X-Received: by 10.36.55.138 with SMTP id r132mr2874779itr.34.1512149693003; Fri, 01 Dec 2017 09:34:53 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Fri, 1 Dec 2017 09:34:52 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Fri, 1 Dec 2017 17:34:52 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , "ard.biesheuvel@linaro.org@arm.com" <"ard.biesheuvel@linaro.org"@arm.com>, "leif.lindholm@linaro.org@arm.com" <"leif.lindholm@linaro.org"@arm.com>, "Matteo.Carlini@arm.com@arm.com" <"Matteo.Carlini@arm.com"@arm.com>, "nd@arm.com@arm.com" <"nd@arm.com"@arm.com> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Dec 2017 17:30:27 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 1 December 2017 at 16:33, Evan Lloyd wrote: > Responses inline: > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 13 October 2017 08:33 >> To: Evan Lloyd >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com; >> "leif.lindholm@linaro.org"@arm.com; >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add >> debug asserts >> >> On 26 September 2017 at 21:15, wrote: >> > From: Girish Pathak >> > >> > This change adds some debug assertions e.g to catch NULL pointer >> > errors missing in PL11Lcd and HdLcd modules. >> > >> > This change also improves related error handling code. >> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Girish Pathak >> > Signed-off-by: Evan Lloyd >> > --- >> > >> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr >> mVExpress.c | 44 ++++++++++++++++++-- >> > >> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11 >> 1LcdArmVExpress.c | 43 ++++++++++++++++++- >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c >> | 8 ++-- >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c >> | 8 ++-- >> > 4 files changed, 90 insertions(+), 13 deletions(-) >> > >> > diff --git >> > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd >> ArmVE >> > xpress.c >> > >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd >> ArmVE >> > xpress.c index >> > >> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a >> 71d0c7c >> > ce72d71c6da5 100644 >> > --- >> > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd >> ArmVE >> > xpress.c >> > +++ >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd >> A >> > +++ rmVExpress.c >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay ( >> > * buffer in bytes >> > * >> > * @retval EFI_SUCCESS Frame buffer memory allocation su= ccess. >> > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize >> are NULL. >> > * @retval !(EFI_SUCCESS) Other errors. >> > **/ >> > EFI_STATUS >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram ( >> > EFI_STATUS Status; >> > EFI_ALLOCATE_TYPE AllocationType; >> > >> > + // Check VramBaseAddress and VramSize are not NULL. >> > + if (VramBaseAddress =3D=3D NULL || VramSize =3D=3D NULL) { >> > + ASSERT (VramBaseAddress !=3D NULL); >> > + ASSERT (VramSize !=3D NULL); >> > + return EFI_INVALID_PARAMETER; >> > + } >> > + >> > // Set the vram size >> > *VramSize =3D LCD_VRAM_SIZE; >> > >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram ( >> > VramBaseAddress >> > ); >> > if (EFI_ERROR (Status)) { >> > + ASSERT_EFI_ERROR (Status); >> > return Status; >> > } >> > >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram ( >> > *VramSize, >> > EFI_MEMORY_WC >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > + ASSERT_EFI_ERROR (Status); >> >> What is the point of this change? > [[Evan Lloyd]] It is a minor efficiency improvement. Since the ASSERT ca= n only fire when the if condition is true, it removes a duplicated test fro= m the main (non-error) code flow. This is irrelevant on hardware, but actu= ally significant when running debug builds on an emulator environment. > Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is not on X86 either). Code is complex enough as it is, and how to move trivial tests like this around for 'performance' reasons is a dimension I would like to avoid. >> >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); >> > return Status; >> > } >> > @@ -215,6 +224,7 @@ LcdPlatformSetMode ( >> > EFI_STATUS Status; >> > >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -264,6 +274,7 @@ LcdPlatformSetMode ( >> > * >> > * @retval EFI_SUCCESS Success if the requested mode is = found. >> > * @retval EFI_INVALID_PARAMETER Requested mode not found. >> > + * @retval EFI_INVALID_PARAMETER Info is NULL. >> > **/ >> > EFI_STATUS >> > LcdPlatformQueryMode ( >> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode ( >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info >> > ) >> > { >> > - if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Info =3D=3D NULL) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> >> Please don't put anything that may have a side effect inside ASSERT (), = since >> they are dropped from RELEASE builds. You can just use ASSERT (FALSE) >> here, or use a temp variable. > > [[Evan Lloyd]] Agreed > >> >> >> > + ASSERT (Info !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings ( >> > ) >> > { >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> >> same here > [[Evan Lloyd]] Agreed >> >> > + return EFI_INVALID_PARAMETER; >> > + } >> > + >> > + if (HRes =3D=3D NULL >> > + || HSync =3D=3D NULL >> > + || HBackPorch =3D=3D NULL >> > + || HFrontPorch =3D=3D NULL >> > + || VRes =3D=3D NULL >> > + || VSync =3D=3D NULL >> > + || VBackPorch =3D=3D NULL >> > + || VFrontPorch =3D=3D NULL) >> > + { >> > + // One of the pointers is NULL >> > + ASSERT (HRes !=3D NULL); >> > + ASSERT (HSync !=3D NULL); >> > + ASSERT (HBackPorch !=3D NULL); >> > + ASSERT (HFrontPorch !=3D NULL); >> > + ASSERT (VRes !=3D NULL); >> > + ASSERT (VSync !=3D NULL); >> > + ASSERT (VBackPorch !=3D NULL); >> > + ASSERT (VFrontPorch !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings ( >> > * >> > * @retval EFI_SUCCESS The requested mode is found. >> > * @retval EFI_INVALID_PARAMETER Requested mode not found. >> > + * @retval EFI_INVALID_PARAMETER Bpp is NULL. >> > **/ >> > EFI_STATUS >> > LcdPlatformGetBpp ( >> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp ( >> > OUT LCD_BPP * CONST Bpp >> > ) >> > { >> > - if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Bpp =3D=3D NULL) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> > + ASSERT (Bpp !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > diff --git >> > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1 >> 11Lc >> > dArmVExpress.c >> > >> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1 >> 11Lc >> > dArmVExpress.c index >> > >> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a >> f7233c1 >> > cebfe1ad339b 100644 >> > --- >> > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1 >> 11Lc >> > dArmVExpress.c >> > +++ >> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1 >> > +++ 11LcdArmVExpress.c >> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay ( >> > * buffer in bytes >> > * >> > * @retval EFI_SUCCESS Frame buffer memory allocation su= ccess. >> > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize is >> NULL. >> > * @retval !(EFI_SUCCESS) Other errors. >> > **/ >> > EFI_STATUS >> > @@ -203,6 +204,13 @@ LcdPlatformGetVram ( >> > >> > Status =3D EFI_SUCCESS; >> > >> > + // Check VramBaseAddress and VramSize are not NULL. >> > + if (VramBaseAddress =3D=3D NULL || VramSize =3D=3D NULL) { >> > + ASSERT (VramBaseAddress !=3D NULL); >> > + ASSERT (VramSize !=3D NULL); >> > + return EFI_INVALID_PARAMETER; >> > + } >> > + >> > // Is it on the motherboard or on the daughterboard? >> > switch (PL111_CLCD_SITE) { >> > >> > @@ -223,6 +231,7 @@ LcdPlatformGetVram ( >> > VramBaseAddress >> > ); >> > if (EFI_ERROR (Status)) { >> > + ASSERT_EFI_ERROR (Status); >> > return Status; >> > } >> > >> > @@ -233,8 +242,8 @@ LcdPlatformGetVram ( >> > *VramSize, >> > EFI_MEMORY_WC >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > + ASSERT_EFI_ERROR (Status); >> >> Please drop this change > > [[Evan Lloyd]] As stated above, this is not inconsequential for some use= cases. The ASSERT can only fire when the if is satisfied, so moving the A= SSERT reduces the number of tests executed in normal execution (depending o= n optimisation level). > As stated above, let's fix DEBUG vs NOOPT instead. >> >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES >> (*VramSize)); >> > return Status; >> > } >> > @@ -293,6 +302,7 @@ LcdPlatformSetMode ( >> > UINT32 SysId; >> > >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> >> No function calls inside ASSERT () please > [[Evan Lloyd]] Agreed > >> >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -359,6 +369,7 @@ LcdPlatformSetMode ( >> > * (on success). >> > * >> > * @retval EFI_SUCCESS Success if the requested mode is = found. >> > + * @retval EFI_INVALID_PARAMETER Info is NULL. >> > * @retval EFI_INVALID_PARAMETER Requested mode not found. >> > **/ >> > EFI_STATUS >> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode ( >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info >> > ) >> > { >> > - if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Info =3D=3D NULL) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> > + ASSERT (Info !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode ( >> > * >> > * @retval EFI_SUCCESS Success if the requested mode is = found. >> > * @retval EFI_INVALID_PARAMETER Requested mode not found. >> > + * @retval EFI_INVALID_PARAMETER One of the OUT parameters is >> NULL. >> > **/ >> > EFI_STATUS >> > LcdPlatformGetTimings ( >> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings ( >> > ) >> > { >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> > + return EFI_INVALID_PARAMETER; >> > + } >> > + >> > + if (HRes =3D=3D NULL >> > + || HSync =3D=3D NULL >> > + || HBackPorch =3D=3D NULL >> > + || HFrontPorch =3D=3D NULL >> > + || VRes =3D=3D NULL >> > + || VSync =3D=3D NULL >> > + || VBackPorch =3D=3D NULL >> > + || VFrontPorch =3D=3D NULL) >> > + { >> > + // One of the pointers is NULL >> > + ASSERT (HRes !=3D NULL); >> > + ASSERT (HSync !=3D NULL); >> > + ASSERT (HBackPorch !=3D NULL); >> > + ASSERT (HFrontPorch !=3D NULL); >> > + ASSERT (VRes !=3D NULL); >> > + ASSERT (VSync !=3D NULL); >> > + ASSERT (VBackPorch !=3D NULL); >> > + ASSERT (VFrontPorch !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings ( >> > * >> > * @retval EFI_SUCCESS The requested mode is found. >> > * @retval EFI_INVALID_PARAMETER Requested mode not found. >> > + * @retval EFI_INVALID_PARAMETER Bpp is NULL. >> > **/ >> > EFI_STATUS >> > LcdPlatformGetBpp ( >> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp ( >> > ) >> > { >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); >> > + ASSERT (Bpp !=3D NULL); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c >> > index >> > >> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd >> b0cf7cbf >> > e59d2f4dc49c 100644 >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c >> > @@ -109,15 +109,15 @@ LcdSetMode ( >> > &VBackPorch, >> > &VFrontPorch >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > - return EFI_DEVICE_ERROR; >> > + ASSERT_EFI_ERROR (Status); >> > + return Status; >> > } >> > >> > Status =3D LcdPlatformGetBpp (ModeNumber, &LcdBpp); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > - return EFI_DEVICE_ERROR; >> > + ASSERT_EFI_ERROR (Status); >> > + return Status; >> > } >> > >> > BytesPerPixel =3D GetBytesPerPixel (LcdBpp); diff --git >> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c >> > index >> > >> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd >> ab3898f >> > 6e0dbf9a1572 100644 >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c >> > @@ -110,15 +110,15 @@ LcdSetMode ( >> > &VBackPorch, >> > &VFrontPorch >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > - return EFI_DEVICE_ERROR; >> > + ASSERT_EFI_ERROR (Status); >> > + return Status; >> > } >> > >> > Status =3D LcdPlatformGetBpp (ModeNumber, &LcdBpp); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > - return EFI_DEVICE_ERROR; >> > + ASSERT_EFI_ERROR (Status); >> > + return Status; >> > } >> > >> > // Disable the CLCD_LcdEn bit >> > -- >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >> > > IMPORTANT NOTICE: The contents of this email and any attachments are conf= idential and may also be privileged. If you are not the intended recipient,= please notify the sender immediately and do not disclose the contents to a= ny other person, use it for any purpose, or store or copy the information i= n any medium. Thank you.