From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.24435.1673446095080143955 for ; Wed, 11 Jan 2023 06:08:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=TmYi64SA; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673446094; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=BJvIL2Ac9eIJ6JQ9lyFV+p8ULFHtPlWcZpQ9qujJZ8k=; b=TmYi64SAD2z5P38AuFGDcilIGVfnPb1nlevKaYGoRd2pBWNNSVawiq+gje5Ir+nb87weBC p0B6oWqpjW9TTwSkPb3LB6T5BngIZVA8nmaE3PRhM3iH77FpxUjefMUqdY6G+0L8ORpqzE MvOgi5DkjpUdz4akMLcnu6ZatZmgDAw= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-576-xoJnyZ8hO06err0SZ93sxg-1; Wed, 11 Jan 2023 09:08:13 -0500 X-MC-Unique: xoJnyZ8hO06err0SZ93sxg-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 901693806702; Wed, 11 Jan 2023 14:08:12 +0000 (UTC) Received: from [10.39.192.40] (unknown [10.39.192.40]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DED7F492B00; Wed, 11 Jan 2023 14:08:10 +0000 (UTC) Message-ID: <38553500-02cc-f61c-4036-a409dceb7d8f@redhat.com> Date: Wed, 11 Jan 2023 15:08:09 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB To: Gerd Hoffmann Cc: devel@edk2.groups.io, Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-4-kraxel@redhat.com> <20230111080616.cxrs3kfqmmsmqpts@sirius.home.kraxel.org> From: "Laszlo Ersek" In-Reply-To: <20230111080616.cxrs3kfqmmsmqpts@sirius.home.kraxel.org> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/11/23 09:06, Gerd Hoffmann wrote: >>> + DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End)); >> >> (3) I've not noticed before, but I'm noticing now: before this series, >> the DEBUG levels (or more precisely, "debug masks") for the messages >> emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of >> them were DEBUG_VERBOSE (per original intent), but then commit >> bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from >> qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the >> perspective of this series, that's a preexistent inconsistency. >> >> Is that worth fixing up at first, separately? (Changing it to >> DEBUG_VERBOSE.) >> >> (4) Also relating to logging. The current patch set seems to move all >> DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm >> unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are >> slow; are we sure we want these logs emitted in a defult build of OVMF? >> >> (5) Still about logging. Before this series, the >> PlatformScanOrAdd64BitE820Ram() function would log each E820 entry >> before investigating them. (And, based on the investigation, further >> messages may be logged.) With the whole series applied however, as far >> as I can tell, we'll never get a complete dump of the E820 map, because >> PlatformScanE820() doesn't log entries at all, and the callbacks only >> log entries that prove "interesting" for them. >> >> Is this intentional? I think it may make debugging harder. I didn't >> notice it under patch#1, but I think we should add generic >> (unconditional) logging there. > > Yes, all intentional. > > I've dropped all logging from PlatformScanE820, we run that multiple > times and logging the e820 map there would make it show up multiple > times in the logs. Instead I'm logging at the places where the code > actually does something with the values (set LowMemory, add HOB, ...), > which should give us good insights into the code flow in the logs. > > I've turned them on by default because the logging should be less > verbose than it used to be and also because I've found myself flipping > these from VERBOSE to INFO frequently when debugging something. I think the last part should be replaced by you just building with DEBUG_VERBOSE :), carrying perhaps a few patches for the DSC files for re-disabling DEBUG_VERBOSE for a number of unjustifiably chatty modules (I can give you a list); *regardless*, I think the above is all good, just please mention it somewhere in the commit messages. (Best would be to change the debug levels after the conversion, separately, but I don't want to get on your nerves.) > >> (6) *Yet more* logging-related observations. The original log message >> uses a bracket "[" on the left hand side of the logged intervals, and a >> parenthesis ")" on the RHS, to express that the RHS limit is exclusive: >> >> "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n", >> >> This detail is lost in this patch (in all three DEBUG messages); both >> sides use brackets. > > Oh, I wasn't aware of that. It looked like a typo to me so I "fixed" it > along the way. I think I prefer to stick with the (inclusive) brackets > and print "End - 1" then so it actually is inclusive. Works for me -- as long as you won't be searching the firmware logs for the *exclusive end* hex strings :) :) :) (Now of course you can satisfy both goals, as long as you don't print (End-1) with "0x%Lx", but print (End) with "0x%Lx - 1"! In fact, you may have meant the latter already, above.) > >> (7) Sorry, I'm getting tired and my observations are starting to fall >> apart. Anyway -- I think all the actual callback functions should be >> STATIC. > > Yes. PlatformScanE820() is static too. Isn't there a gcc warning which > complains about non-static functions without prototype in some header? > Seems this is not turned on for edk2, I'm somehow used to gcc throwing > warnings on that. Haha, good catch -- it's intentionally not turned on in edk2. Namely, some version of MSVC (maybe even bleeding edge ones, I don't know) screw up source-level debugging for such functions that are marked STATIC, as far as I remember. This is why, in core packages, you'll see an inexplicably low number of STATIC functions. So the MSVC shortcoming (if I can call it that) actively conflicts with the nice gcc warning you mention. The gcc warning is a no-brainer for when you can actually enable it. > >> (8) Furthermore, *perhaps* we should put E820 in their names somewhere >> (I don't insist at all), instead of the "Platform" prefix -- these >> functions are not public PlatformInitLib APIs. > > There is a simple reason for the naming: > I love 'grep ^Platform' on edk2 log files ;) That sounds convincing enough, thanks. Log analysis is important, we should support it! > >>> + DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End)); >> >> (10) I meant to ask earlier -- what is now the maximum line length? >> >> I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg": >> >>> # Code width / line splitting >>> #code_width =120 # TODO: This causes non-deterministic behaviour in some cases when code wraps >>> ls_code_width =false >> >> Does that mean that 120 is considered a soft limit? (I used to ask for >> 80 chars under OvmfPkg, but there's no need to stick with that anymore.) > > Oh, 120. I already wondered why uncrustify didn't wrap that one, I > didn't expect the limit being higher than 100 which seems to be the new > standard in many projects. > > take care, > Gerd > Thanks! Laszlo