From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by mx.groups.io with SMTP id smtpd.web11.9331.1589192824820940312 for ; Mon, 11 May 2020 03:27:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=fRR3topS; spf=pass (domain: nuviainc.com, ip: 209.85.128.41, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f41.google.com with SMTP id e26so17470202wmk.5 for ; Mon, 11 May 2020 03:27:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=pCNt7OVdQvjHQ0fK8dOjSHvZ3qRHflBczjW1FjTvaug=; b=fRR3topSe+w6/m/mgqK5BdJhSDYGfEKX43y+UkySOmbsY+Icww48HhmpiGMe5L6hmM 5EPMxqeuEZvkdqFiw8s3aFJtSyXtUoYqQJurtTicJ39LpJag4wGAsVNrGGip3KBrAzmp H4WtszFhDFpqeTaUuCI1yDrtoANXgqBhwEjq9AtFPbPGKNAWGNesO+yip5f9iy2mfDR9 9qh/rofPsSHi/FT3ckycX9ginPEsGcJ4hxoJkdvLChEXuWwSIUgvZ9objVaNnLCu+lKN OkpHosoqB0j3Ncsn6tTcsCStPKjcG26VzjHtjbO+SEyQz9PVREdBMTkhaLMDBMjiba01 9JaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=pCNt7OVdQvjHQ0fK8dOjSHvZ3qRHflBczjW1FjTvaug=; b=knRR8Nw7GbuR0qHm94OvQVa3ps0+UunshuWBm80KvdJQhBcDJBwOD+V3XyTe/VBPel 9MFR+4MWPaYWsCwIGE71in5NQF+Z35wbj2zSE3HBTmT73FW1MBYktbLY2/Z0NFn7CDiG NQhzZi3H86EOPID74WdFf1JX7HSdVAOaSwU47dRcBWIOqqPbHxJEBvEbKNG7FCnY/yP+ S6Ae+Li3OB34J1mrZ4XB7EFoOGlK4hXMh6tzUUgRldw901npXVuBSaVJO6vN05atl3uX v1LS0w+5n6k9mez0DPWxHk3pBeZX6lP6U07g7X7bQLd78K2PCrB8Ryod/auYkvhG5rql OGCw== X-Gm-Message-State: AGi0PuZH40DoXVW+5dRJOxL7O8LnnEboNiQH8+D51rLSBNgMyNimo4BA GQxuJhyDNikeiFuoPg7O5eln0g== X-Google-Smtp-Source: APiQypJwe0hP0qeMyuKPd1dpvauKU65eb4viZ0FtjYMEFRrTqyISN0uGVVwtWwapXv+77Z2dYqMk1Q== X-Received: by 2002:a7b:c7d5:: with SMTP id z21mr14726707wmk.112.1589192823201; Mon, 11 May 2020 03:27:03 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id 88sm17752915wrq.77.2020.05.11.03.27.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 May 2020 03:27:02 -0700 (PDT) Date: Mon, 11 May 2020 11:27:00 +0100 From: "Leif Lindholm" To: "Pankaj Bansal (OSS)" Cc: Meenakshi Aggarwal , Michael D Kinney , "devel@edk2.groups.io" , Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton , Ard Biesheuvel Subject: Re: [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib Message-ID: <20200511102700.GU21486@vanye> References: <20200501054955.13025-1-pankaj.bansal@oss.nxp.com> <20200501054955.13025-13-pankaj.bansal@oss.nxp.com> <20200506104405.GL21486@vanye> <20200507101535.GQ21486@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Pankaj, On Fri, May 08, 2020 at 05:31:53 +0000, Pankaj Bansal (OSS) wrote: > > > > > - { > > > > > - Found = TRUE; > > > > > - break; > > > > > - } > > > > > - NextHob.Raw = GET_NEXT_HOB (NextHob); > > > > > + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE > > (DramRegions)); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > > Slightly concerned here because we end up with a variable Status that > > > > is only *used* in DEBUG builds. That could lead to toolchain warnings. > > > > > > I don't think this would cause build warnings in RELEASE builds. I have tested it. > > > Also the Status is frequently being handled in this way in other places in edk2: > > > > > > > > https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/ > > PlatformPeim.c#L90 > > > > The example you point to sets Status, overwrites it (once or twice > > depending on conditionals), then returns it. > > This patch in its current form sets Status, accesses it only in DEBUG > > builds and does not return it. > > Ok. Then I can return status at the end of this function (MemoryPeim). > I still can't bring myself to just ignore the critical error status from a function. If it is considered critical, it needs to be handled. If it is not critical (but would, for example, be useful to log and tell someone about) it can be explicitly cast away until such a time as the full error reporting can be iplemented. That should not be confused for ignoring it. > > > > And I was completely serious last time around, I am OK with the return > > > > value being cast away explicitly. What I meant with that is: > > > > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions)); > > > > > > I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from > > GetDramRegionsInfo, > > > But If we discard the return value it means we are OK with some RAM not > > being reported to UEFI firmware > > > and subsequently to OS. Isn't this a critical error ? > > > > ASSERTs are only triggered in DEBUG builds, and send the processor > > into WFI. > > > > If it is a critical error (is it critical if you have found some RAM, > > but been unable to fully reconcile all of the RAM in the system?), it > > should do more than that. > > > > I am all for properly handling that situation, but this patch has > > never done that. Feel free to rework before submitting v5, or leave it > > until (if) adding ACPI support and report the condition properly > > through BERT. > > I have referred reference platform lib https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c#L83 > As per this lib, I have added ASSERTS in all the scenarios, which > are critical. Adding the ASSERT is good hygiene, I have no objection to that. > I thought ASSERT means we will halt the program execution regardless > of DEBUG or RELEASE build. Dying without attempted recovery is not an appropriate response for a RELEASE build. Especially in the case where we have successfully located memory to use, but have not fully managed to reconcile all entries. > Can you point me to a reference platform which handles these errors > using ACPI BERT methods ? Sadly, no. The Hisilicon 1620 platform implements BERT support as part of its APEI driver, but it does not appear to actually log anything. > I see that in DebugLib.h : > > Note that a reserved macro named MDEPKG_NDEBUG is introduced for the intention > of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is > defined, then debug and assert related macros wrapped by it are the NULL implementations. > > We have NOT defined MDEPKG_NDEBUG in our platforms for RELEASE or DEBUG builds. > Up until we have not implemented the ACPI BERT methods, can we keep > it this way to avoid masking the errors? It would be unfortunate if the NXP platforms behaved differently from the majority of other EDK2 platforms. It would also mean that if someone was to create a derivative platform (which I expect is what NXP wants), then various drivers in this reference implementation would start failing to build if *they* added MDEPKG_NDEBUG to *their* .dsc. So it would be much preferred if you could add the MDEPKG_NDEBUG stanzas to RELEASE CC_FLAGS. But thank you for explaining why I wasn't seeing the "set but not used" warning :) Best Regards, Leif