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. 


> 
> >  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.