From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 13 Jun 2019 05:45:20 -0700 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 1134722389F; Thu, 13 Jun 2019 12:45:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-127.ams2.redhat.com [10.36.117.127]) by smtp.corp.redhat.com (Postfix) with ESMTP id 82695176BA; Thu, 13 Jun 2019 12:45:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data To: David Woodhouse , "Wu, Hao A" , "devel@edk2.groups.io" , "Ni, Ray" Cc: "Justen, Jordan L" , Ard Biesheuvel References: <20190527030350.11996-1-hao.a.wu@intel.com> <18b24dae9f4e5c022849502fc4b60ac3ba59d6f1.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <32298b39-71f5-274a-6f53-2dcb2eda2fc5@redhat.com> Date: Thu, 13 Jun 2019 14:45:11 +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: 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.39]); Thu, 13 Jun 2019 12:45:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 06/13/19 10:34, David Woodhouse wrote: > On Thu, 2019-06-13 at 06:28 +0000, Wu, Hao A wrote: >> Hello Ray, >> >> Do you have comment on this? >> >> Some inline comments below: >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>> David Woodhouse >>> Sent: Thursday, June 13, 2019 5:43 AM >>> To: Wu, Hao A >>> Cc: devel@edk2.groups.io; Ni, Ray; Justen, Jordan L; Laszlo Ersek; Ar= d >>> Biesheuvel >>> Subject: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correc= t >>> Legacy16GetTableAddress call for E820 data >>> >>> The DX register is supposed to contain the required alignment for the >>> allocation. It was zero, and SeaBIOS doesn't (well, didn't) cope well >>> with that. Set it appropriately, and set BX to indicate the regions >>> it's OK to allocate in too. That was already OK but let's make sure >>> it's initialised properly and not just working by chance. >>> >>> Also actually return an error if the allocation fails. Instead of goi= ng >>> all the way through into the CSM and just letting it have a bogus >>> pointer to the E82o data. >>> >>> Signed-off-by: David Woodhouse >>> --- >>> I made SeaBIOS cope with the zero too: >>> https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/= 4PH >>> W3O3Y3HJFENODFV5INBGDLZMXA5KE/ >>> >>> OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c | 3 +++ >> >> >> Not sure if it is the issue of my mail client, the new lines added are >> with LF line ending on my local machine after applying the patch. >=20 > Hm, that's normal, isn't it? >=20 > We still haven't fixed the repository to store LF line endings > internally and then let all the tools automatically do the right thing > by checking out to LF or CRLF as appropriate for the system you're > checking out on. >=20 > I vaguely recall that Laszlo has some scripts which mangle an email and > make it apply with CRLF? But only vaguely... which is why I asked for a > git tree instead of trying to run the gauntlet of trying to apply > patches 1-10 of your series myself. >=20 > I've pushed v2 with the E82o typo fixed, and using BX=3D=3D0, to=20 > http://git.infradead.org/users/dwmw2/edk2.git/shortlog/refs/heads/csm > git://http://git.infradead.org/users/dwmw2/edk2.git csm >=20 > I'll try sending that with git-send-email directly rather than with via > my mailer, but IIRC that doesn't make any difference. CRLF conversions > are baked into every level =E2=80=94 even SMTP converts to send CRLF on= the > wire and often back again at the receive side.=20 You may be remembering this article: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-gi= t-guide-for-edk2-contributors-and-maintainers I think Hao may have missed "--keep-cr" with git-am (or "git config am.keepcr true" for a permanent setting). Leif recently implemented a python script that configures one's git clone the way we suggest: 1 5b3e695d8ac5 BaseTools: add centralized location for git config files 2 4eb0acb1e2be BaseTools: add script to configure local git options The conversion of the repo's internal representation to LF has not been forgotten (nor abandoned); it's just that I've personally learned to cope with CRLF, and everyone's got to pick their fights. I support the conversion but I have no capacity for working on it. Thanks, Laszlo >> >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c >>> b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c >>> index 211750c012..e7766eb2b1 100644 >>> --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c >>> +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c >>> @@ -928,7 +928,9 @@ GenericLegacyBoot ( >>> if (CopySize > Private->Legacy16Table->E820Length) { >>> ZeroMem (&Regs, sizeof (EFI_IA32_REGISTER_SET)); >>> Regs.X.AX =3D Legacy16GetTableAddress; >>> + Regs.X.BX =3D (UINT16) 0x3; // Region >> >> >> According to the spec: >> >> ''' >> BX =3D Allocation region >> 00 =3D Allocate from either 0xE0000 or 0xF0000 64 KB blocks. >> Bit 0 =3D 1 Allocate from 0xF0000 64 KB block >> Bit 1 =3D 1 Allocate from 0xE0000 64 KB block >> ''' >> >> I think the value 0 for BX is fine which indicates the allocation can >> happen in either ranges. Not sure if setting BX to 0x11 is a valid >> operation. >=20 > Note, I may have lied in my reply to Jordan when I said that 0x11 is > what was already happening. The way SeaBIOS copes with zero is by > setting it to 3... *before* the debug print showing what it was set to. >=20 >> With the above comments handled: >> Reviewed-by: Hao A Wu >=20 > Thanks. >=20