1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 00/10] Duplicate required CSM components for OVMF
  4. View patch

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

Wu, Hao A posted 10 patches 6 years, 8 months ago
[edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by David Woodhouse 6 years, 8 months ago 
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/4PHW3O3Y3HJFENODFV5INBGDLZMXA5KE/

 OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c | 3 +++
 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
     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 (#42317): https://edk2.groups.io/g/devel/message/42317
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by Wu, Hao A 6 years, 8 months ago 
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,
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by David Woodhouse 6 years, 8 months ago 
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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42349): https://edk2.groups.io/g/devel/message/42349
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by Laszlo Ersek 6 years, 8 months ago 
On 06/13/19 10:34, David Woodhouse wrote:
> 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. 

You may be remembering this article:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

I think Hao may have missed "--keep-cr" with git-am (or "git config
am.keepcr true" for a permanent setting).

Leif recently implemented a python script that configures one's git
clone the way we suggest:

1 5b3e695d8ac5 BaseTools: add centralized location for git config files
2 4eb0acb1e2be BaseTools: add script to configure local git options


The conversion of the repo's internal representation to LF has not been
forgotten (nor abandoned); it's just that I've personally learned to
cope with CRLF, and everyone's got to pick their fights. I support the
conversion but I have no capacity for working on it.

Thanks,
Laszlo

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42360): https://edk2.groups.io/g/devel/message/42360
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by Jordan Justen 6 years, 8 months ago 
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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42335): https://edk2.groups.io/g/devel/message/42335
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by David Woodhouse 6 years, 8 months ago 
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*.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42339): https://edk2.groups.io/g/devel/message/42339
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe: Correct Legacy16GetTableAddress call for E820 data
Posted by Ni, Ray 6 years, 8 months ago 
> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, June 13, 2019 2:28 PM
> To: devel@edk2.groups.io; dwmw2@infradead.org; Ni, Ray
> <ray.ni@intel.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [edk2-devel] [PATCH 11/10] OvmfPkg/Csm/LegacyBiosDxe:
> Correct Legacy16GetTableAddress call for E820 data
> 
> Hello Ray,
> 
> Do you have comment on this?

No.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42331): https://edk2.groups.io/g/devel/message/42331
Mute This Topic: https://groups.io/mt/32045939/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 11/10] OvmfPkg/Csm/LegacyBiosDxe: Fix Legacy16GetTableAddress call for E820 data
Posted by David Woodhouse 6 years, 8 months ago 
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.

Also set BX to indicate the regions it's OK to allocate in too. That
wasn't being initialised and was just using whatever the previous user
of the structure had left there.

Finally, 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 E820 data.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
index 211750c012..cd4cd24f42 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) 0x0; // Any region
     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,
-- 
2.21.0


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42350): https://edk2.groups.io/g/devel/message/42350
Mute This Topic: https://groups.io/mt/32050165/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 11/10] OvmfPkg/Csm/LegacyBiosDxe: Fix Legacy16GetTableAddress call for E820 data
Posted by Laszlo Ersek 6 years, 8 months ago 
On 06/13/19 10:40, David Woodhouse wrote:
> 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.
> 
> Also set BX to indicate the regions it's OK to allocate in too. That
> wasn't being initialised and was just using whatever the previous user
> of the structure had left there.
> 
> Finally, 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 E820 data.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
> index 211750c012..cd4cd24f42 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) 0x0; // Any region
>      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,
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42375): https://edk2.groups.io/g/devel/message/42375
Mute This Topic: https://groups.io/mt/32050165/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.