[edk2] PiSmmIpl SMRAM Reservation Logic

[Cohen, Eugene]

I've been debugging an SMM IPL issue and have isolated it to an assumption in the SMRAM range reservation logic in SmmIplEntry. The logic checks to see if the reserved range resides within the SMRAM descriptor and if it does it reduces the size of the SMRAM range by the start address of the reservation. The purpose of this code is to find a large enough region to load the SMM Core.

This logic is flawed in that it assumes that the reserved range is only near the end of the SMRAM descriptor:

//
// This range has reserved area, calculate the left free size
//
gSmmCorePrivate->SmramRanges[Index].PhysicalSize = SmramResRegion->SmramReservedStart - gSmmCorePrivate->SmramRanges[Index].CpuStart;


Imagine the following scenario where we just reserve the first page of the SMRAM range:

SMRAM Descriptor:
Start: 0x80000000
Size: 0x02000000

Reserved Range:
Start: 0x80000000
Size: 0x00001000

In this case the adjustment to the SMRAM range size yields zero: ReservedStart - SMRAM Start is 0x80000000 - 0x80000000 = 0. So even though most of the range is still free the IPL code decides its unusable.

I don't know what the original intent was, but maybe if the math was changed to subtract the reserved size it would yield a more sensible result:

gSmmCorePrivate->SmramRanges[Index].PhysicalSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize - SmramResRegion->SmramReservedSize;

This logic only works if the reservation fits entirely within an SMRAM descriptor range. The PI SMM spec does not state that the reserved ranges (from SMM Configuration) must reside entirely within an SMRAM range (from SMM Access). If we want to keep the implementation this simple we should clarify this requirement in the spec.

Eugene



------------------------------------------------------------------------------

[Cohen, Eugene]

When trying the seemingly simple fix I see it doesn't work properly because the IPL could try to stomp over the reserved range at the beginning. To really handle this correctly we would either need to reduce the descriptor from the beginning (increase start address) and end (decrease size) or even go so far as to split the descriptor into multiple ones. I'd like to hear what the module maintainer prefers to do.

Eugene

-----Original Message-----
From: Cohen, Eugene
Sent: Thursday, June 04, 2015 1:02 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] PiSmmIpl SMRAM Reservation Logic

I've been debugging an SMM IPL issue and have isolated it to an assumption in the SMRAM range reservation logic in SmmIplEntry. The logic checks to see if the reserved range resides within the SMRAM descriptor and if it does it reduces the size of the SMRAM range by the start address of the reservation. The purpose of this code is to find a large enough region to load the SMM Core.

This logic is flawed in that it assumes that the reserved range is only near the end of the SMRAM descriptor:

//
// This range has reserved area, calculate the left free size
//
gSmmCorePrivate->SmramRanges[Index].PhysicalSize = SmramResRegion->SmramReservedStart - gSmmCorePrivate->SmramRanges[Index].CpuStart;


Imagine the following scenario where we just reserve the first page of the SMRAM range:

SMRAM Descriptor:
Start: 0x80000000
Size: 0x02000000

Reserved Range:
Start: 0x80000000
Size: 0x00001000

In this case the adjustment to the SMRAM range size yields zero: ReservedStart - SMRAM Start is 0x80000000 - 0x80000000 = 0. So even though most of the range is still free the IPL code decides its unusable.

I don't know what the original intent was, but maybe if the math was changed to subtract the reserved size it would yield a more sensible result:

gSmmCorePrivate->SmramRanges[Index].PhysicalSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize - SmramResRegion->SmramReservedSize;

This logic only works if the reservation fits entirely within an SMRAM descriptor range. The PI SMM spec does not state that the reserved ranges (from SMM Configuration) must reside entirely within an SMRAM range (from SMM Access). If we want to keep the implementation this simple we should clarify this requirement in the spec.

Eugene

[Yao, Jiewen]

Hi Eugene
Thanks to catch this.
Yes, I fully agree with you that the hidden assumption should be removed here.

After I revisit code again, my thought is below.
Fact 1: PiSmmIpl will manipulate gSmmCorePrivate->SmramRanges at least 2 places.
One is here - for SmmConfiguration.
The other is to remove PiSmmCore.
gSmmCorePrivate->SmramRanges will adjust PhysicalSize to make reserved region invisible to PiSmmCore.

Fact 2: PiSmmCore also need full Smram information, so PiSmmIpl use gSmmCorePrivate->FullSmramRanges.

Thought 1: I think we can remove gSmmCorePrivate->FullSmramRanges.
When we exclude some Smram from gSmmCorePrivate->SmramRanges, we can split record in gSmmCorePrivate->SmramRanges and mark it to be ALLOCATED in gSmmCorePrivate->SmramRanges. Then PiSmmCore can still use full record in gSmmCorePrivate->SmramRanges to calculate Smram information.
Using 2 records is confusing...

Thought 2: I think we can visit all gSmmCorePrivate->SmramRanges one by one, to see if there is overlap with SmmConfiguration->SmramReservedRegions. Then we split the gSmmCorePrivate->SmramRanges record.
I think the split logic can handle SmmConfiguration->SmramReservedRegions at beginning, at end, in the middle, or cross multiple SmramRanges.

Please let me know if this works.

Thank you
Yao Jiewen

-----Original Message-----
From: Cohen, Eugene [mailto:***@hp.com]
Sent: Friday, June 05, 2015 3:22 AM
To: edk2-***@lists.sourceforge.net
Cc: Yao, Jiewen
Subject: RE: PiSmmIpl SMRAM Reservation Logic

When trying the seemingly simple fix I see it doesn't work properly because the IPL could try to stomp over the reserved range at the beginning. To really handle this correctly we would either need to reduce the descriptor from the beginning (increase start address) and end (decrease size) or even go so far as to split the descriptor into multiple ones. I'd like to hear what the module maintainer prefers to do.

Eugene

-----Original Message-----
From: Cohen, Eugene
Sent: Thursday, June 04, 2015 1:02 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] PiSmmIpl SMRAM Reservation Logic

I've been debugging an SMM IPL issue and have isolated it to an assumption in the SMRAM range reservation logic in SmmIplEntry. The logic checks to see if the reserved range resides within the SMRAM descriptor and if it does it reduces the size of the SMRAM range by the start address of the reservation. The purpose of this code is to find a large enough region to load the SMM Core.

This logic is flawed in that it assumes that the reserved range is only near the end of the SMRAM descriptor:

//
// This range has reserved area, calculate the left free size
//
gSmmCorePrivate->SmramRanges[Index].PhysicalSize = SmramResRegion->SmramReservedStart - gSmmCorePrivate->SmramRanges[Index].CpuStart;


Imagine the following scenario where we just reserve the first page of the SMRAM range:

SMRAM Descriptor:
Start: 0x80000000
Size: 0x02000000

Reserved Range:
Start: 0x80000000
Size: 0x00001000

In this case the adjustment to the SMRAM range size yields zero: ReservedStart - SMRAM Start is 0x80000000 - 0x80000000 = 0. So even though most of the range is still free the IPL code decides its unusable.

I don't know what the original intent was, but maybe if the math was changed to subtract the reserved size it would yield a more sensible result:

gSmmCorePrivate->SmramRanges[Index].PhysicalSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize - SmramResRegion->SmramReservedSize;

This logic only works if the reservation fits entirely within an SMRAM descriptor range. The PI SMM spec does not state that the reserved ranges (from SMM Configuration) must reside entirely within an SMRAM range (from SMM Access). If we want to keep the implementation this simple we should clarify this requirement in the spec.

Eugene

[Cohen, Eugene]

Yao Jiewen,

Post by Yao, Jiewen
Thought 1: I think we can remove gSmmCorePrivate->FullSmramRanges.

I agree - rather than modifying a second copy to remove ranges, it would make more sense to allocate the range out of a single set of descriptors. Reducing the range in another copy is like pretending the SMRAM didn't ever exist which is misleading for debugging or other purposes.

Post by Yao, Jiewen
Thought 2: I think we can visit all gSmmCorePrivate->SmramRanges one by one

I agree, I think an algorithm that truncates the start, truncates the end or splits down the middle is necessary.

To make this manageable we need for SMRAM reservations to fit within a single SMRAM descriptor, otherwise we need to handle reservations crossing descriptors. Realistically I think this is what implementations will do but it's worthy of a clarification in the PI Specification.

You're probably wondering why we wanted to reserve the bottom portion of the SMRAM descriptor - in this case I wanted to put a stack here. For a downward-growing stack I thought I could get some level of stack overflow protection since the range below would not be mapped.

Thank you!

Eugene

-----Original Message-----
From: Yao, Jiewen [mailto:***@intel.com]
Sent: Thursday, June 04, 2015 5:36 PM
To: Cohen, Eugene; edk2-***@lists.sourceforge.net
Subject: RE: PiSmmIpl SMRAM Reservation Logic

Hi Eugene
Thanks to catch this.
Yes, I fully agree with you that the hidden assumption should be removed here.

After I revisit code again, my thought is below.
Fact 1: PiSmmIpl will manipulate gSmmCorePrivate->SmramRanges at least 2 places.
One is here - for SmmConfiguration.
The other is to remove PiSmmCore.
gSmmCorePrivate->SmramRanges will adjust PhysicalSize to make reserved region invisible to PiSmmCore.

Fact 2: PiSmmCore also need full Smram information, so PiSmmIpl use gSmmCorePrivate->FullSmramRanges.

Thought 1: I think we can remove gSmmCorePrivate->FullSmramRanges.
When we exclude some Smram from gSmmCorePrivate->SmramRanges, we can split record in gSmmCorePrivate->SmramRanges and mark it to be ALLOCATED in gSmmCorePrivate->SmramRanges. Then PiSmmCore can still use full record in gSmmCorePrivate->SmramRanges to calculate Smram information.
Using 2 records is confusing...

Thought 2: I think we can visit all gSmmCorePrivate->SmramRanges one by one, to see if there is overlap with SmmConfiguration->SmramReservedRegions. Then we split the gSmmCorePrivate->SmramRanges record.
I think the split logic can handle SmmConfiguration->SmramReservedRegions at beginning, at end, in the middle, or cross multiple SmramRanges.

Please let me know if this works.

Thank you
Yao Jiewen

-----Original Message-----
From: Cohen, Eugene [mailto:***@hp.com]
Sent: Friday, June 05, 2015 3:22 AM
To: edk2-***@lists.sourceforge.net
Cc: Yao, Jiewen
Subject: RE: PiSmmIpl SMRAM Reservation Logic

When trying the seemingly simple fix I see it doesn't work properly because the IPL could try to stomp over the reserved range at the beginning. To really handle this correctly we would either need to reduce the descriptor from the beginning (increase start address) and end (decrease size) or even go so far as to split the descriptor into multiple ones. I'd like to hear what the module maintainer prefers to do.

Eugene

-----Original Message-----
From: Cohen, Eugene
Sent: Thursday, June 04, 2015 1:02 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] PiSmmIpl SMRAM Reservation Logic

I've been debugging an SMM IPL issue and have isolated it to an assumption in the SMRAM range reservation logic in SmmIplEntry. The logic checks to see if the reserved range resides within the SMRAM descriptor and if it does it reduces the size of the SMRAM range by the start address of the reservation. The purpose of this code is to find a large enough region to load the SMM Core.

This logic is flawed in that it assumes that the reserved range is only near the end of the SMRAM descriptor:

//
// This range has reserved area, calculate the left free size
//
gSmmCorePrivate->SmramRanges[Index].PhysicalSize = SmramResRegion->SmramReservedStart - gSmmCorePrivate->SmramRanges[Index].CpuStart;


Imagine the following scenario where we just reserve the first page of the SMRAM range:

SMRAM Descriptor:
Start: 0x80000000
Size: 0x02000000

Reserved Range:
Start: 0x80000000
Size: 0x00001000

In this case the adjustment to the SMRAM range size yields zero: ReservedStart - SMRAM Start is 0x80000000 - 0x80000000 = 0. So even though most of the range is still free the IPL code decides its unusable.

I don't know what the original intent was, but maybe if the math was changed to subtract the reserved size it would yield a more sensible result:

gSmmCorePrivate->SmramRanges[Index].PhysicalSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize - SmramResRegion->SmramReservedSize;

This logic only works if the reservation fits entirely within an SMRAM descriptor range. The PI SMM spec does not state that the reserved ranges (from SMM Configuration) must reside entirely within an SMRAM range (from SMM Access). If we want to keep the implementation this simple we should clarify this requirement in the spec.

Eugene

[Yao, Jiewen]

Ah, thanks for clarification. It is good to have stack overflow check.

Very interesting feature. We have also implemented it before, and we did it in PiSmmCpu driver directly.

PiSmmCpu driver allocate stack for SMM, and PiSmmCpu driver will reserve 2 pages.
The last but one page is marked to be non-exist as guard. The last page is used for guard.
In case stack overflow happens, we still have stack to use to dump debug message. :-)

Thank you
Yao Jiewen

-----Original Message-----
From: Cohen, Eugene [mailto:***@hp.com]
Sent: Friday, June 05, 2015 9:19 PM
To: Yao, Jiewen; edk2-***@lists.sourceforge.net
Subject: RE: PiSmmIpl SMRAM Reservation Logic

Yao Jiewen,

Post by Yao, Jiewen
Thought 1: I think we can remove gSmmCorePrivate->FullSmramRanges.

I agree - rather than modifying a second copy to remove ranges, it would make more sense to allocate the range out of a single set of descriptors. Reducing the range in another copy is like pretending the SMRAM didn't ever exist which is misleading for debugging or other purposes.

Post by Yao, Jiewen
Thought 2: I think we can visit all gSmmCorePrivate->SmramRanges one by one

I agree, I think an algorithm that truncates the start, truncates the end or splits down the middle is necessary.

To make this manageable we need for SMRAM reservations to fit within a single SMRAM descriptor, otherwise we need to handle reservations crossing descriptors. Realistically I think this is what implementations will do but it's worthy of a clarification in the PI Specification.

You're probably wondering why we wanted to reserve the bottom portion of the SMRAM descriptor - in this case I wanted to put a stack here. For a downward-growing stack I thought I could get some level of stack overflow protection since the range below would not be mapped.

Thank you!

Eugene

-----Original Message-----
From: Yao, Jiewen [mailto:***@intel.com]
Sent: Thursday, June 04, 2015 5:36 PM
To: Cohen, Eugene; edk2-***@lists.sourceforge.net
Subject: RE: PiSmmIpl SMRAM Reservation Logic

Hi Eugene
Thanks to catch this.
Yes, I fully agree with you that the hidden assumption should be removed here.

After I revisit code again, my thought is below.
Fact 1: PiSmmIpl will manipulate gSmmCorePrivate->SmramRanges at least 2 places.
One is here - for SmmConfiguration.
The other is to remove PiSmmCore.
gSmmCorePrivate->SmramRanges will adjust PhysicalSize to make reserved region invisible to PiSmmCore.

Fact 2: PiSmmCore also need full Smram information, so PiSmmIpl use gSmmCorePrivate->FullSmramRanges.

Thought 1: I think we can remove gSmmCorePrivate->FullSmramRanges.
When we exclude some Smram from gSmmCorePrivate->SmramRanges, we can split record in gSmmCorePrivate->SmramRanges and mark it to be ALLOCATED in gSmmCorePrivate->SmramRanges. Then PiSmmCore can still use full record in gSmmCorePrivate->SmramRanges to calculate Smram information.
Using 2 records is confusing...

Thought 2: I think we can visit all gSmmCorePrivate->SmramRanges one by one, to see if there is overlap with SmmConfiguration->SmramReservedRegions. Then we split the gSmmCorePrivate->SmramRanges record.
I think the split logic can handle SmmConfiguration->SmramReservedRegions at beginning, at end, in the middle, or cross multiple SmramRanges.

Please let me know if this works.

Thank you
Yao Jiewen

-----Original Message-----
From: Cohen, Eugene [mailto:***@hp.com]
Sent: Friday, June 05, 2015 3:22 AM
To: edk2-***@lists.sourceforge.net
Cc: Yao, Jiewen
Subject: RE: PiSmmIpl SMRAM Reservation Logic

When trying the seemingly simple fix I see it doesn't work properly because the IPL could try to stomp over the reserved range at the beginning. To really handle this correctly we would either need to reduce the descriptor from the beginning (increase start address) and end (decrease size) or even go so far as to split the descriptor into multiple ones. I'd like to hear what the module maintainer prefers to do.

Eugene

-----Original Message-----
From: Cohen, Eugene
Sent: Thursday, June 04, 2015 1:02 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] PiSmmIpl SMRAM Reservation Logic

I've been debugging an SMM IPL issue and have isolated it to an assumption in the SMRAM range reservation logic in SmmIplEntry. The logic checks to see if the reserved range resides within the SMRAM descriptor and if it does it reduces the size of the SMRAM range by the start address of the reservation. The purpose of this code is to find a large enough region to load the SMM Core.

This logic is flawed in that it assumes that the reserved range is only near the end of the SMRAM descriptor:

//
// This range has reserved area, calculate the left free size
//
gSmmCorePrivate->SmramRanges[Index].PhysicalSize = SmramResRegion->SmramReservedStart - gSmmCorePrivate->SmramRanges[Index].CpuStart;


Imagine the following scenario where we just reserve the first page of the SMRAM range:

SMRAM Descriptor:
Start: 0x80000000
Size: 0x02000000

Reserved Range:
Start: 0x80000000
Size: 0x00001000

In this case the adjustment to the SMRAM range size yields zero: ReservedStart - SMRAM Start is 0x80000000 - 0x80000000 = 0. So even though most of the range is still free the IPL code decides its unusable.

I don't know what the original intent was, but maybe if the math was changed to subtract the reserved size it would yield a more sensible result:

gSmmCorePrivate->SmramRanges[Index].PhysicalSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize - SmramResRegion->SmramReservedSize;

This logic only works if the reservation fits entirely within an SMRAM descriptor range. The PI SMM spec does not state that the reserved ranges (from SMM Configuration) must reside entirely within an SMRAM range (from SMM Access). If we want to keep the implementation this simple we should clarify this requirement in the spec.

Eugene



------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel



------------------------------------------------------------------------------