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

["Wu, Hao A" <hao.a.wu@intel.com>]

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.


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

With the above comments handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>      Regs.X.CX = (UINT16) CopySize;
> +    Regs.X.DX = (UINT16) 0x4; // Alignment
>      Private->LegacyBios.FarCall86 (
>        &Private->LegacyBios,
>        Private->Legacy16Table->Compatibility16CallSegment,
> @@ -942,6 +944,7 @@ GenericLegacyBoot (
>      Private->Legacy16Table->E820Length  = (UINT32) CopySize;
>      if (Regs.X.AX != 0) {
>        DEBUG ((EFI_D_ERROR, "Legacy16 E820 length insufficient\n"));
> +      return EFI_OUT_OF_RESOURCES;
>      } else {
>        CopyMem (
>          (VOID *)(UINTN) Private->Legacy16Table->E820Pointer,
> 
> 
>