From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web12.864.1595519259159377730 for ; Thu, 23 Jul 2020 08:47:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=FK2n+v8B; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id o2so5649300wmh.2 for ; Thu, 23 Jul 2020 08:47:38 -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=w+I71YruUQlytuieHDhK9jQn/rYAyYpivodr/TanXww=; b=FK2n+v8B0j8AETBXMQnCT8y05VWeXIjBPHfDptvxQAUOx7aVAsrsHWe1CyXm5furTY xKr32Hk8iyykVSW3F2VKAO9lwd71ljJVfOei5i9GoUg3z0vSGAsqhzeI1Q3iqR2osFqE Arx7P65wdF4iVt9hLWE9gBMksJohQRuXCruMsnPMibCweOipLwOyfvmhtIrHEW2ml6x8 4k8vF0FhxpNyQBTmhFL0rop6/lqZJDAHSaJGOB+RnAfpwjd3xc8GV5laoCdBuNMAM9Z0 JsGm+9dxArlZ7YkVHhr4/3OXb4PJ64PNmLAjqdoMLL4lQmAHqQPJEVAX4PmjfoPmRHww mnHg== 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=w+I71YruUQlytuieHDhK9jQn/rYAyYpivodr/TanXww=; b=hd30pYAVQ2ecpL8l/38iooH17gbgemOIbLIXXFA03dPptw3d3t8rHif1POuwJ8cE9M JA02NcmLCCPrdaMwIXsNM9sJl5gM8O5uWeJZBBAWEaszT2QeNXzoQtIAVIvSte21pl/K 1BljolgELYAjElRvBvbBI1kHm7xneR4Z7wIAOjAJKF0cCQZvlHbGRiI6XrPBGDPKZjUg DxcmUn/rA/u7jpv+hrgMsy8PLL+Vr+hiPMw3Rsm5dZ92gE2tEcYA2A09WUjHCTnuD7LD xHLTy/9NRY7kv+vmExXS0WmMB32JLRT8hHI6P8NqscWX4REgWejFh1eKd0mz0Y/btq/7 KinA== X-Gm-Message-State: AOAM533u5j1Rm/Gcm5ndkwDVTKckruxmZKcDsSO7/gdQUYmTzq+Kuqsb PCsiCl75iwlpSKAcEq7H94xrKg== X-Google-Smtp-Source: ABdhPJwgiQqnkzw3ZzefLXgdE5NLeeogJaXHT3iyYaol03X5KNdKofDaY3zXCFgirGIiTgH6eVAn0w== X-Received: by 2002:a1c:a756:: with SMTP id q83mr4551054wme.168.1595519257574; Thu, 23 Jul 2020 08:47:37 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id w125sm4350358wma.15.2020.07.23.08.47.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jul 2020 08:47:37 -0700 (PDT) Date: Thu, 23 Jul 2020 16:47:35 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@arm.com, awarkentin@vmware.com, samer.el-haj-mahmoud@arm.com Subject: Re: [edk2-platforms][PATCH 1/1] Platforms/RaspberryPi: Fix BIOS Release Date and System Manufacturer Message-ID: <20200723154735.GO1337@vanye> References: <20200720161507.14352-1-pete@akeo.ie> <20200723110826.GM1337@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 Pete, On Thu, Jul 23, 2020 at 15:38:55 +0100, Pete Batard wrote: > > > @@ -626,6 +627,28 @@ BIOSInfoUpdateSmbiosType0 ( > > > INTN i; > > > INTN State = 0; > > > INTN Value[2]; > > > + INTN Year = (__DATE__[7] == '?' ? 1900 \ > > > + : (((__DATE__[7] - '0') * 1000 ) \ > > > + + (__DATE__[8] - '0') * 100 \ > > > + + (__DATE__[9] - '0') * 10 \ > > > + + __DATE__[10] - '0')); > > > + INTN Month = ( __DATE__ [2] == '?' ? 1 \ > > > + : __DATE__ [2] == 'n' ? ( \ > > > + __DATE__ [1] == 'a' ? 1 : 6) \ > > > + : __DATE__ [2] == 'b' ? 2 \ > > > + : __DATE__ [2] == 'r' ? ( \ > > > + __DATE__ [0] == 'M' ? 3 : 4) \ > > > + : __DATE__ [2] == 'y' ? 5 \ > > > + : __DATE__ [2] == 'l' ? 7 \ > > > + : __DATE__ [2] == 'g' ? 8 \ > > > + : __DATE__ [2] == 'p' ? 9 \ > > > + : __DATE__ [2] == 't' ? 10 \ > > > + : __DATE__ [2] == 'v' ? 11 \ > > > + : 12); > > > + INTN Day = ( __DATE__[4] == '?' ? 1 \ > > > + : ((__DATE__[4] == ' ' ? 0 : \ > > > + ((__DATE__[4] - '0') * 10)) \ > > > + + __DATE__[5] - '0')); > > > > So, this hunk is very neat, but nigh-on unreviewable. > > I.e. we should defintely have it - but only once. > > > > Could you break this up into some macros to go into some generic > > helper lib? (I don't have a better idea than EmbeddedPkg TimeBaseLib, > > but then that is already included in this module.) > > So you would like to have a set of macros like: > TIME_GET_BUILD_YEAR, TIME_GET_BUILD_MONTH, TIME_GET_BUILD_DAY in > TimeBaseLib.h that perform the above? > > > Would you be OK to break that snippet out separate? > > I think that's a good idea, especially as there is a potential underlying > issue with the __DATE__ format being specific to each compiler, so we > probably want to handle compiler detection somewhere, preferably globally. > For instance, the Intel compiler's __DATE__ format would not work with the > above, so I'll add some "vetted compiler" detection for the macros. Yes, thanks. A friendly "your compiler probably won't know what to do with this" #error would be friendlier than whatever non-GCC/clang would generate on encountering this. > The one thing I am not planning to look into is that, of course, as long as > you don't explicitly force the compiler to rebuild the sources where these > macros are used, then you may end up with a very obsolete build date > compared to the actual date of your last re-build. Yeah, that's fine. > But that's mostly because > I don't think there exists a generic solution we can ise to force > recompilation of a file that uses a specific macro and also because our main > goal with these is to ensure that the Pi firmwares, that we produce through > AppVeyor, have a proper build date, and AppVeyor builds are are always > clean. Yeah, I really don't care about that. Or the flipside, where someone modifies only (say) the Smbios populating code a month after they built the rest of the tree. If someone wants to put a *specific* date in, that should be done manually, and if someone wants information on what was actually built, they should put in the hashes of the top commits of all repositories on PACKAGES_PATH as well as whether the state of each was clean. Managing these bits through a rigid build process, like you are, is very much valid. > I'll send an EDK2 patch for the macros, and then a revised patch for this > when I get a chance. Thanks! Regards, Leif > Regards, > > /Pete > > > > > / > > Leif > > > > > // Populate the Firmware major and minor. > > > Status = mFwProtocol->GetFirmwareRevision (&EpochSeconds); > > > @@ -648,6 +671,10 @@ BIOSInfoUpdateSmbiosType0 ( > > > mBiosVendor, sizeof (mBiosVendor)); > > > UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString), > > > mBiosVersion, sizeof (mBiosVersion)); > > > + ASSERT (Year >= 0 && Year <= 9999); > > > + ASSERT (Month >= 1 && Month <= 12); > > > + ASSERT (Day >= 1 && Day <= 31); > > > + AsciiSPrint (mBiosDate, sizeof (mBiosDate), "%02d/%02d/%04d", Month, Day, Year); > > > // Look for a "x.y" numeric string anywhere in mBiosVersion and > > > // try to parse it to populate the BIOS major and minor. > > > -- > > > 2.21.0.windows.1 > > > >