On Thu, 2019-06-13 at 00:40 -0700, Jordan Justen wrote:
> On 2019-06-12 23:28:12, Wu, Hao A wrote:
> > 
> > > -----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
> > > 
> > > 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.
> 
> 'E82o' => 'E820'

Yeah, spotted that just after sending and it's in my tree for v2 if
there is one. Thanks.

> > > 
> > > 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.
> 
> Based on the spec you quote, it does seem reasonable to expect that a
> CSM should treat 0 the same as 3, but it is possible that some CSM out
> there made a silly choice and would blow up on 3.

I think it's more likely that a CSM would blow up on zero (no bits set
→ no regions to allocate from) than 3. In fact, I just had to double-
check that SeaBIOS does the right thing for BX==0. (It does.)

In practice, Legacy16GetTableAddress only seems to be called with BX==1
or 3, and I haven't seen any calls with 0.

The setting of Regs.X.BX in this patch has no observable effect — it
was *already* 3 before this call, because whoever used it last happened
to leave it like that. Setting it explicitly was just a cleanup to make
sure we didn't depend on that any more.

> Since David mentioned that bx==0 works ok with SeaBIOS CSM, then maybe
> we should just drop this change? Or, we can add a comment that bx==0
> means either the e000 or f000 block?

SeaBIOS as CSM will with with *DX* == 0, after this goes in:
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/4PHW3O3Y3HJFENODFV5INBGDLZMXA5KE/

It does look like it already works with BX==0. So I'm not entirely
averse to setting it explicitly to 0 instead, if you prefer. I just
wanted it to be set explicitly to *something*.