From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.3214.1588846540282857413 for ; Thu, 07 May 2020 03:15:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=zwm0OyKz; spf=pass (domain: nuviainc.com, ip: 209.85.128.66, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f66.google.com with SMTP id k12so5811794wmj.3 for ; Thu, 07 May 2020 03:15:39 -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=j95VG/SYoZVIrsU14GhRwM5pSEZ7NstL/Nz3P062hKU=; b=zwm0OyKzqs1u0gCT+j2HxV4XAgqPXsW4AWYKJz4u3SXNmyB2dczRpfhEYKJg/pCDcd Ey8UM8WwySeMuBzR/K0st0n9dy+lzWNIm+sbpacn3PC0KyZ014X7Ye3nTXKJ2YivA+2J SYJUfP80WDGo0tY3V/8paJJVWzkOUzIV9UMUS5ECVSxdClRwGsXnYSKBaxr+v29HS9CQ 2aVaA8uhiqLhgHkqet0HILs4hvP6mxr79noBzG9zWWaVwmGfjrAsNm784G7bi8tOptdb YSupZRVwZwwemk0WqTjtVnNp+jgYw3XBrCSu/64Cnf1xmaxbAtP7T0WfdG8xZR+5iCGS 4/iw== 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=j95VG/SYoZVIrsU14GhRwM5pSEZ7NstL/Nz3P062hKU=; b=s4AzC7ECPC5I3H9CeijJib1SMxjZXv59wCtMLLDVfsQAbAAsk6z3qj/PAOV2akBgaU WWxXJe+KY/C4bqut3IN7QrzI+JoNYvLPc3nUE9jd6EiW9rn3XTPUNOj4Wwrrw+j+ZgNj srmg7DJOVmPhPs14uEr3VYbsImO2ngw4orHfPGlodLPgiZ3RWgqGV5086gfcHLXy6Lml 3H6wL2TqPusP8PArXW129KlzRZFvpDZR9pg+aak6OX7q4f311zM1iCI0MIH7HDVV6xQg 3MxF7aOMJDS/dZrrS7WABCpJSldn0CtPhbJhPJLF/JZ7RSyGILwdOoHvOVQP1TVBMkn6 Y1tg== X-Gm-Message-State: AGi0PuZEk8QEtGHtioNJyeRPCJb3L4qIV5NYpoTyLYlNKhFz7P3GdIYG LW/vcgDbazt8mjRSj58xd0U6XQ== X-Google-Smtp-Source: APiQypL9ODpHz26F01XGvK9o60xVV+nvQ+DrT7g+5Wr2zchMRZMAplCvVKzm1hpuJk858Sl0z/6DAQ== X-Received: by 2002:a1c:4e12:: with SMTP id g18mr9461163wmh.11.1588846538601; Thu, 07 May 2020 03:15:38 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id z7sm2147851wrl.88.2020.05.07.03.15.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 May 2020 03:15:38 -0700 (PDT) Date: Thu, 7 May 2020 11:15:35 +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: <20200507101535.GQ21486@vanye> References: <20200501054955.13025-1-pankaj.bansal@oss.nxp.com> <20200501054955.13025-13-pankaj.bansal@oss.nxp.com> <20200506104405.GL21486@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 On Thu, May 07, 2020 at 07:28:30 +0000, Pankaj Bansal (OSS) wrote: > Hi Leif, > > > > + ARM_SMC_ARGS ArmSmcArgs; > > > > > > -Routine Description: > > > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > > > + ArmSmcArgs.Arg1 = -1; > > > > Should this be SMC_UNK? > > No. SMC_OK / SMC_UNK is returned values. > While x0, x1 are arguments. > I have explained this in the MemoryInitPeiLib.h OK, then that -1 needs a separate #define. > // This SMC call works in this way: > // x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size > // x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK > // 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return > // x0: SMC_OK, x1: Base address of DRAM region, > // x2: Size of DRAM region > > > > > > > > > + ArmCallSmc (&ArmSmcArgs); > > > > > > + if (ArmSmcArgs.Arg0 == SMC_OK) { > > > + return ArmSmcArgs.Arg1; > > > + } > > > > > .... > > > > - { > > > - 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. > > 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. / Leif