From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.49346.1606206217629953339 for ; Tue, 24 Nov 2020 00:23:37 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZmmMh9I3; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606206216; 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=nEj/dR5+VZeM+N+rjrurst93h5V2rp7G374wl5XVQMQ=; b=ZmmMh9I34GLW1aW2h4ldyL6GkW1MrOszcK6TBRtE9suQENWaSJe/WcHgJAlqYs86OQbouZ ez5JL2VvSWIf/cK2xZeVQHL8Ac02/5OIRTe51Zmpf0D3dYSFxbGvSE1YrLNaXU5f47CbSl x61UfXVwZrA8wRrgS/2wOOaEhOHxEXQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-155-3oRM60aNM-6o0jJXxwEY6Q-1; Tue, 24 Nov 2020 03:23:32 -0500 X-MC-Unique: 3oRM60aNM-6o0jJXxwEY6Q-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7D02C89CD11; Tue, 24 Nov 2020 08:23:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-113.ams2.redhat.com [10.36.113.113]) by smtp.corp.redhat.com (Postfix) with ESMTP id E95C919C46; Tue, 24 Nov 2020 08:23:27 +0000 (UTC) Subject: Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package To: jejb@linux.ibm.com, devel@edk2.groups.io Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-3-jejb@linux.ibm.com> <28e99174-79b3-e805-b977-5fed0071a702@redhat.com> <06b9425507ab8c1b35d377cf9bba155b0cc44147.camel@linux.ibm.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 24 Nov 2020 09:23:26 +0100 MIME-Version: 1.0 In-Reply-To: <06b9425507ab8c1b35d377cf9bba155b0cc44147.camel@linux.ibm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/24/20 07:38, James Bottomley wrote: > On Mon, 2020-11-23 at 22:08 +0100, Laszlo Ersek wrote: >> In [a](5) I requested the removal of everything in this INF file that >> was not required for the desired semantics (i.e., for unconditionally >> booting the builtin GRUB binary). >> >> So again I'm unsure if you missed my feedback, or thought it was not >> important. (I didn't get a request for clarification either.) >> >> Keeping INF files minimal is relevant for future contributions. We >> frequently need to determine the set of modules that depend on a >> particular PCD. If some INF files list PCDs unjustifiedly, then the >> affected module set may appear larger than it really is. >> >> This applies to all sections of the INF file, not just [Pcd]. > > I did try stripping quite a lot, but then it wouldn't boot. It seems > that the PCI devices are needed for grub to find the encrypted volume, > so I put most of it back again. Is there some way of identifying > superfluous pieces so I can detect if I put too much back? I suggest proceeding element by element in the INF file (and matching that, in the header / C files) -- remove one element per patch. Then you can either build+test it as you go, or create a series of micro-patches up-front, and if it doesn't boot, bisect it. Finally, squash the removals that are justifiable into this patch (for posting), and drop the rest. It's unfortunately trial and error to some extent; however, given that each step removes 1 element (PCD, Protocol, GUID, lib class, and finally Package), and the number of elements summed over the INF file sections is finite, this process is guaranteed to terminate. Working in the other direction is much simpler, because the compiler and/or the "build" tool will tell you if something is missing. Alas, they don't tell us when something is listed superfluously. (The same problem applies to #include directives in any C project -- I'm unaware of any tool that flags a superfluous #include directive as such.) >> (13) the indentation seems strange (Linux kernel style?); please >> don't use hardware tabs. (Hmmm, applies to other parts of this script >> too.) > > OK, I got emacs to untabify it. I did try converting it to DOS format > like the rest of edk2 but bash really doesn't like that: > > /home/jejb/git/edk2/OvmfPkg/AmdSev/Grub/grub.sh: line 10: $'\r': > command not found Yup, "grub.cfg" and "grub.sh" should use LF line terminators, not CRLF. If you use 8bit or base64 Content-Transfer-Encoding with git-send-email, it should be possible to send a patch that contains hunks with both LF and CRLF line terminators. This CRLF mess is a long-standing problem in edk2, and it has trained me to look for files such as "grub.cfg" and "grub.sh" in patches. I tend to verify them for LF manually, and if they appear to have CRLF after I apply them locally -- which may or may not mean that they had CRLF on the contributor's side too --, then I push them through dos2unix first (rebasing the series). ... apologies that I sounded irritated earlier; it had been another hectic day. Thanks! Laszlo