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.129.124]) by mx.groups.io with SMTP id smtpd.web10.18614.1673424383114654629 for ; Wed, 11 Jan 2023 00:06:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LWnm4PHo; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673424382; 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: in-reply-to:in-reply-to:references:references; bh=oQ6eRQDeawvYxClf9FnZrdfHLlJXjNbkZp0gKQzE7hE=; b=LWnm4PHokwh4MtxDpnMI50+xdeLSP9AYA7BzxWXtFE3HE53kv/aDhYu9cJcsOe2wL2tAVr FHQcsHKRKO5K24tzZuT15hpI0lt/HkCKQcbDOlXp6U5rE635UzatmqHhMfIpmL0QWFrTl3 W+/lZn+nB+FuOWi7llm4B8SXipiAiKo= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-225-iK_IiXH2NtKjJg_1adgrtA-1; Wed, 11 Jan 2023 03:06:18 -0500 X-MC-Unique: iK_IiXH2NtKjJg_1adgrtA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 82629101A52E; Wed, 11 Jan 2023 08:06:18 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.192.238]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1644440C2004; Wed, 11 Jan 2023 08:06:18 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id B28EC180060E; Wed, 11 Jan 2023 09:06:16 +0100 (CET) Date: Wed, 11 Jan 2023 09:06:16 +0100 From: "Gerd Hoffmann" To: Laszlo Ersek Cc: devel@edk2.groups.io, Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel Subject: Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Message-ID: <20230111080616.cxrs3kfqmmsmqpts@sirius.home.kraxel.org> References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-4-kraxel@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline > > + 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. > (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. > (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. > (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 ;) > > + 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