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 <dw...@infradead.org> > --- > I made SeaBIOS cope with the zero too: > https://mail.coreboot.org/hyperkitty/list/seab...@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...@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, > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42327): https://edk2.groups.io/g/devel/message/42327 Mute This Topic: https://groups.io/mt/32045939/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
