From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3447421B02822 for ; Thu, 4 Oct 2018 06:06:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D9DFF86675; Thu, 4 Oct 2018 13:06:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-104.rdu2.redhat.com [10.10.120.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4442D5D6B5; Thu, 4 Oct 2018 13:06:19 +0000 (UTC) To: Leif Lindholm , aaron.young@oracle.com Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" , Brijesh Singh , pjones@redhat.com References: <3b4a4980-42d0-51a0-eccd-ceba83b6c78f@oracle.com> <20181004092411.4bgio3ewvmwkfuyq@bivouac.eciton.net> From: Laszlo Ersek Message-ID: Date: Thu, 4 Oct 2018 15:06:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181004092411.4bgio3ewvmwkfuyq@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 04 Oct 2018 13:06:21 +0000 (UTC) Subject: Re: Regression with PXE boot on OvmfPkg/VirtioNetDxe driver on aarch64 system X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Oct 2018 13:06:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/04/18 11:24, Leif Lindholm wrote: > +Peter > > On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote: >> I am suspecting that this patch to GRUB is the cause of a Buffer being >> re-transmitted before reaping the Buffer via SNP->GetStatus(): >> >> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch >> >> So, to reproduce the issue, the GRUB used via PXE boot needs to include >> this patch. > > So the issue cannot be reproduced with upstream GRUB? > > Does Fedora/Red Hat include the same patch? Here's what I can see. (1) In upstream grub , at commit 8ada906031d9 ("msdos: Fix overflow in converting partition start and length into 512B blocks", 2018-09-27), on the master branch, the patch is not present. (2) In "rhboot" grub2 , where the master branch seems to track upstream grub, the patch is present on at least the "fedora-28" and "rhel-7.5" branches. Commit hashes, respectively: c2b126f52143, 1b9767c136082. (3) In the commit message, Josef wrote, "When I fixed the txbuf handling I ripped out the retransmission code". I think he referred to his earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy firmware properly", 2015-08-09). That commit is upstream. In my opinion, commit 4fe8e6d4a127, the chronologically first, and upstream, tweak, was right (assuming the comment it added was true, about grub). On the other hand, the downstream-only (chronologically 2nd) commit was wrong. Not only did it break the spec, it broke even grub's own internal invariant, described in the comment that was added in upstream commit 4fe8e6d4a127. The comment states, "we only transmit one packet at a time". With the downstream-only tweak applied, that's no longer true. Namely, SNP.Transmit() is called while we know another transmission is pending on the egress queue. That's the definition of sending more than one packet at a time. I'm curious why the patch in question is not upstream. Was it submitted and rejected? Submitted and ignored? Not submitted at all? I'm not a fan of the hard-liner "spec above everything" approach. In this case though, after the downstream-only tweak, grub is inconsistent not only with the spec, but with itself too. IMO, downstream(s) should revert the downstream-only patch. Thanks, Laszlo