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

["Jordan Justen" <jordan.l.justen@intel.com>]

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'

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

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?

-Jordan