Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data

["Laszlo Ersek" <lersek@redhat.com>]

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; Ard
>>> Biesheuvel
>>> Subject: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct
>>> 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 going
>>> all the way through into the CSM and just letting it have a bogus
>>> pointer to the E82o data.
>>>
>>> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
>>> ---
>>> 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.
> 
> Hm, that's normal, isn't it?
> 
> 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.
> 
> 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.
> 
> I've pushed v2 with the E82o typo fixed, and using BX==0, to 
> http://git.infradead.org/users/dwmw2/edk2.git/shortlog/refs/heads/csm
> git://http://git.infradead.org/users/dwmw2/edk2.git csm
> 
> 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 — even SMTP converts to send CRLF on the
> wire and often back again at the receive side. 

You may be remembering this article:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-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 = Legacy16GetTableAddress;
>>> +    Regs.X.BX = (UINT16) 0x3; // Region
>>
>>
>> According to the spec:
>>
>> '''
>> BX = Allocation region
>> 00 = Allocate from either 0xE0000 or 0xF0000 64 KB blocks.
>> Bit 0 = 1 Allocate from 0xF0000 64 KB block
>> Bit 1 = 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.
> 
> 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.
> 
>> With the above comments handled:
>> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Thanks.
>