[edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Michael Kubacki]

From: Patrick Payne <patpa@microsoft.com>

Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE resource attribute as
per the PI 1.8 spec. This flag is used to indicate that the memory
should be treated as special purpose memory (SPM).

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---

Notes:
    V3: Assign the GCD memory type for resource HOBs with
        the SPECIAL_PURPOSE attribute set prior to assigning
        the GCD memory type for the PERSISTENT attribute.
    
        Assign the GCD memory type EfiGcdMemoryTypeReserved
        instead of EfiGcdMemoryTypeSystemMemory for resource
        HOBs with the SPECIAL_PURPOSE attribute set.
    
    V2: Adds package name to commit subject and updates
        Signed-off-by.

 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index fe1bbd6974b7..4cc0940c0a7d 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -26,7 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
                                        EFI_RESOURCE_ATTRIBUTE_16_BIT_IO           | \
                                        EFI_RESOURCE_ATTRIBUTE_32_BIT_IO           | \
                                        EFI_RESOURCE_ATTRIBUTE_64_BIT_IO           | \
-                                       EFI_RESOURCE_ATTRIBUTE_PERSISTENT          )
+                                       EFI_RESOURCE_ATTRIBUTE_PERSISTENT          | \
+                                       EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE     )
 
 #define TESTED_MEMORY_ATTRIBUTES  (EFI_RESOURCE_ATTRIBUTE_PRESENT     |     \
                                        EFI_RESOURCE_ATTRIBUTE_INITIALIZED | \
@@ -92,6 +93,7 @@ GCD_ATTRIBUTE_CONVERSION_ENTRY  mAttributeConversionTable[] = {
   { EFI_RESOURCE_ATTRIBUTE_TESTED,                  EFI_MEMORY_TESTED,        FALSE },
   { EFI_RESOURCE_ATTRIBUTE_PERSISTABLE,             EFI_MEMORY_NV,            TRUE  },
   { EFI_RESOURCE_ATTRIBUTE_MORE_RELIABLE,           EFI_MEMORY_MORE_RELIABLE, TRUE  },
+  { EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE,         EFI_MEMORY_SP,            TRUE  },
   { 0,                                              0,                        FALSE }
 };
 
@@ -691,6 +693,10 @@ ConverToCpuArchAttributes (
     CpuArchAttributes |= EFI_MEMORY_WP;
   }
 
+  if ((Attributes & EFI_MEMORY_SP) == EFI_MEMORY_SP) {
+    CpuArchAttributes |= EFI_MEMORY_SP;
+  }
+
   return CpuArchAttributes;
 }
 
@@ -2656,6 +2662,10 @@ CoreInitializeGcdServices (
             GcdMemoryType = EfiGcdMemoryTypeReserved;
           }
 
+          if ((ResourceHob->ResourceAttribute & EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE) == EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE) {
+            GcdMemoryType = EfiGcdMemoryTypeReserved;
+          }
+
           if ((ResourceHob->ResourceAttribute & EFI_RESOURCE_ATTRIBUTE_PERSISTENT) == EFI_RESOURCE_ATTRIBUTE_PERSISTENT) {
             GcdMemoryType = EfiGcdMemoryTypePersistent;
           }
-- 
2.45.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119065): https://edk2.groups.io/g/devel/message/119065
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Du Lin]

Coherent Device Attribute Table (CDAT) specification defines a EFI memory type and attribute "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute".
Can we still support this type if assigning the GCD memory type "EfiGcdMemoryTypeReserved" for resource HOBs with the SPECIAL_PURPOSE attribute set?

CDAT 1.04 specification: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.04%20published_0.pdf

BRs,
Lin, Du

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Saturday, May 18, 2024 8:58 AM
To: devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

From: Patrick Payne <patpa@microsoft.com>

Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE resource attribute as per the PI 1.8 spec. This flag is used to indicate that the memory should be treated as special purpose memory (SPM).

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---

Notes:
    V3: Assign the GCD memory type for resource HOBs with
        the SPECIAL_PURPOSE attribute set prior to assigning
        the GCD memory type for the PERSISTENT attribute.
    
        Assign the GCD memory type EfiGcdMemoryTypeReserved
        instead of EfiGcdMemoryTypeSystemMemory for resource
        HOBs with the SPECIAL_PURPOSE attribute set.
    
    V2: Adds package name to commit subject and updates
        Signed-off-by.

 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index fe1bbd6974b7..4cc0940c0a7d 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -26,7 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
                                        EFI_RESOURCE_ATTRIBUTE_16_BIT_IO           | \
                                        EFI_RESOURCE_ATTRIBUTE_32_BIT_IO           | \
                                        EFI_RESOURCE_ATTRIBUTE_64_BIT_IO           | \
-                                       EFI_RESOURCE_ATTRIBUTE_PERSISTENT          )
+                                       EFI_RESOURCE_ATTRIBUTE_PERSISTENT          | \
+                                       EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE     )
 
 #define TESTED_MEMORY_ATTRIBUTES  (EFI_RESOURCE_ATTRIBUTE_PRESENT     |     \
                                        EFI_RESOURCE_ATTRIBUTE_INITIALIZED | \ @@ -92,6 +93,7 @@ GCD_ATTRIBUTE_CONVERSION_ENTRY  mAttributeConversionTable[] = {
   { EFI_RESOURCE_ATTRIBUTE_TESTED,                  EFI_MEMORY_TESTED,        FALSE },
   { EFI_RESOURCE_ATTRIBUTE_PERSISTABLE,             EFI_MEMORY_NV,            TRUE  },
   { EFI_RESOURCE_ATTRIBUTE_MORE_RELIABLE,           EFI_MEMORY_MORE_RELIABLE, TRUE  },
+  { EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE,         EFI_MEMORY_SP,            TRUE  },
   { 0,                                              0,                        FALSE }
 };
 
@@ -691,6 +693,10 @@ ConverToCpuArchAttributes (
     CpuArchAttributes |= EFI_MEMORY_WP;
   }
 
+  if ((Attributes & EFI_MEMORY_SP) == EFI_MEMORY_SP) {
+    CpuArchAttributes |= EFI_MEMORY_SP;  }
+
   return CpuArchAttributes;
 }
 
@@ -2656,6 +2662,10 @@ CoreInitializeGcdServices (
             GcdMemoryType = EfiGcdMemoryTypeReserved;
           }
 
+          if ((ResourceHob->ResourceAttribute & EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE) == EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE) {
+            GcdMemoryType = EfiGcdMemoryTypeReserved;
+          }
+
           if ((ResourceHob->ResourceAttribute & EFI_RESOURCE_ATTRIBUTE_PERSISTENT) == EFI_RESOURCE_ATTRIBUTE_PERSISTENT) {
             GcdMemoryType = EfiGcdMemoryTypePersistent;
           }
--
2.45.1.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119065): https://edk2.groups.io/g/devel/message/119065
Mute This Topic: https://groups.io/mt/106165072/8327890
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [du.lin@intel.com]
-=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119111): https://edk2.groups.io/g/devel/message/119111
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Oliver Smith-Denny]

On 5/21/2024 8:40 PM, Du Lin wrote:
> Coherent Device Attribute Table (CDAT) specification defines a EFI memory type and attribute "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute".
> Can we still support this type if assigning the GCD memory type "EfiGcdMemoryTypeReserved" for resource HOBs with the SPECIAL_PURPOSE attribute set?
> 
> CDAT 1.04 specification: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.04%20published_0.pdf

Thanks for pointing out the CDAT specification. I think one of the
issues here is that the PI and UEFI specs are very vague on
EFI_MEMORY_SP, to the point that there is no user story on it.
This CDAT spec helps close some of the gap, but I think the
PI and UEFI spec should be updated to be more verbose in the
intended usage of this flag.

Does any CDAT code exist in edk2? A naive search didn't turn up
anything (and even looking at some of the CXL structures I didn't
see any usage outside of the header). Is the intention that the
CDAT would be read in PEI and a resource descriptor HOB created
from it? If so, I agree this would prevent EfiConventionalMemory
from being set. I think the CDAT spec should not be referencing
EFI memory types if that is the case, because resource descriptor
HOBs deal with resource types, not EFI types, so there is a mismatch.

If the CDAT is read at a different time and a resource descriptor
HOB is not created from it, then this should not affect it.

The reason that GCD reserved type was chosen here was that the
assumption for CXL or other remote memory is that UEFI would not
want to use it (what if the bus went down and DXE core was
allocated there). By marking it reserved with the EFI_MEMORY_SP
bit set, we ensure that UEFI does not use it and that the OS knows
that this is usable, but the kernel itself may not want to use it,
for the same reason (or for performance). Again, the UEFI spec does
not describe what memory type this should be, so if there is a different
use case here, please do share.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119115): https://edk2.groups.io/g/devel/message/119115
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Du Lin]

Thanks for the quick response.

Agree that the PI and UEFI specs are vague on SP. That is also why I opted to minimize code changes to DXE core for SP support in patch https://edk2.groups.io/g/devel/message/118712.

Would it make more sense to let the caller determine if SP memory is available for UEFI via EFI resource types (e.g., EFI_RESOURCE_SYSTEM_MEMORY vs EFI_RESOURCE_MEMORY_RESERVED)?

CDAT can be read in PEI phase via DOE method and CDAT is important to support CXL 2.0. I believe CDAT spec is referencing EFI_MEMORY_TYPE and Memory Attributes defined in UEFI spec section 7.2. "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute" may suggest that the memory type shall be EfiConventionalMemory and the attribute shall have SP set when reporting the memory to OS. And the concern is whether this combination can still be supported if we always mark resource HOBs with SP set as EfiGcdMemoryTypeReserved.

BRs,
Lin, Du

-----Original Message-----
From: Oliver Smith-Denny <osde@linux.microsoft.com> 
Sent: Wednesday, May 22, 2024 11:04 PM
To: devel@edk2.groups.io; Lin, Du <du.lin@intel.com>; mikuback@linux.microsoft.com
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

On 5/21/2024 8:40 PM, Du Lin wrote:
> Coherent Device Attribute Table (CDAT) specification defines a EFI memory type and attribute "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute".
> Can we still support this type if assigning the GCD memory type "EfiGcdMemoryTypeReserved" for resource HOBs with the SPECIAL_PURPOSE attribute set?
> 
> CDAT 1.04 specification: 
> https://uefi.org/sites/default/files/resources/Coherent%20Device%20Att
> ribute%20Table_1.04%20published_0.pdf

Thanks for pointing out the CDAT specification. I think one of the issues here is that the PI and UEFI specs are very vague on EFI_MEMORY_SP, to the point that there is no user story on it.
This CDAT spec helps close some of the gap, but I think the PI and UEFI spec should be updated to be more verbose in the intended usage of this flag.

Does any CDAT code exist in edk2? A naive search didn't turn up anything (and even looking at some of the CXL structures I didn't see any usage outside of the header). Is the intention that the CDAT would be read in PEI and a resource descriptor HOB created from it? If so, I agree this would prevent EfiConventionalMemory from being set. I think the CDAT spec should not be referencing EFI memory types if that is the case, because resource descriptor HOBs deal with resource types, not EFI types, so there is a mismatch.

If the CDAT is read at a different time and a resource descriptor HOB is not created from it, then this should not affect it.

The reason that GCD reserved type was chosen here was that the assumption for CXL or other remote memory is that UEFI would not want to use it (what if the bus went down and DXE core was allocated there). By marking it reserved with the EFI_MEMORY_SP bit set, we ensure that UEFI does not use it and that the OS knows that this is usable, but the kernel itself may not want to use it, for the same reason (or for performance). Again, the UEFI spec does not describe what memory type this should be, so if there is a different use case here, please do share.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119137): https://edk2.groups.io/g/devel/message/119137
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Oliver Smith-Denny]

On 5/23/2024 2:17 AM, Du Lin wrote:
> Thanks for the quick response.
> 
> Agree that the PI and UEFI specs are vague on SP. That is also why I opted to minimize code changes to DXE core for SP support in patch https://edk2.groups.io/g/devel/message/118712.
> 
> Would it make more sense to let the caller determine if SP memory is available for UEFI via EFI resource types (e.g., EFI_RESOURCE_SYSTEM_MEMORY vs EFI_RESOURCE_MEMORY_RESERVED)?
> 
> CDAT can be read in PEI phase via DOE method and CDAT is important to support CXL 2.0. I believe CDAT spec is referencing EFI_MEMORY_TYPE and Memory Attributes defined in UEFI spec section 7.2. "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute" may suggest that the memory type shall be EfiConventionalMemory and the attribute shall have SP set when reporting the memory to OS. And the concern is whether this combination can still be supported if we always mark resource HOBs with SP set as EfiGcdMemoryTypeReserved.

Thanks for the clarification. I agree that it makes sense to let the
resource HOB creator determine whether UEFI will put this in
system memory or reserved memory. DxeCore at that point could decide
to not allocate any memory with the EFI_MEMORY_SP attribute (or it
could decide it doesn't care).

We are meeting with some CXL stakeholders to make sure there is no
concern with changing this patch and then we will respin this.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119168): https://edk2.groups.io/g/devel/message/119168
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Du Lin]

Is there any feedback from CXL stakeholders?

It is OK if more time is needed to check with CXL stakeholders. But looks like we all agree that the GCD attribute conversion table shall be updated to convert EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE to EFI_MEMORY_SP.
So could we review and merge the conversion table update first, while we are waiting for the feedback from CXL stakeholders? If you agree, a pull request has been created for the conversion table update: https://github.com/tianocore/edk2/pull/5691.

BRs,
Lin, Du

-----Original Message-----
From: Oliver Smith-Denny <osde@linux.microsoft.com> 
Sent: Friday, May 24, 2024 5:05 AM
To: devel@edk2.groups.io; Lin, Du <du.lin@intel.com>; mikuback@linux.microsoft.com
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

On 5/23/2024 2:17 AM, Du Lin wrote:
> Thanks for the quick response.
> 
> Agree that the PI and UEFI specs are vague on SP. That is also why I opted to minimize code changes to DXE core for SP support in patch https://edk2.groups.io/g/devel/message/118712.
> 
> Would it make more sense to let the caller determine if SP memory is available for UEFI via EFI resource types (e.g., EFI_RESOURCE_SYSTEM_MEMORY vs EFI_RESOURCE_MEMORY_RESERVED)?
> 
> CDAT can be read in PEI phase via DOE method and CDAT is important to support CXL 2.0. I believe CDAT spec is referencing EFI_MEMORY_TYPE and Memory Attributes defined in UEFI spec section 7.2. "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute" may suggest that the memory type shall be EfiConventionalMemory and the attribute shall have SP set when reporting the memory to OS. And the concern is whether this combination can still be supported if we always mark resource HOBs with SP set as EfiGcdMemoryTypeReserved.

Thanks for the clarification. I agree that it makes sense to let the resource HOB creator determine whether UEFI will put this in system memory or reserved memory. DxeCore at that point could decide to not allocate any memory with the EFI_MEMORY_SP attribute (or it could decide it doesn't care).

We are meeting with some CXL stakeholders to make sure there is no concern with changing this patch and then we will respin this.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119402): https://edk2.groups.io/g/devel/message/119402
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Oliver Smith-Denny]

On 5/30/2024 8:44 PM, Du Lin wrote:
> Is there any feedback from CXL stakeholders?

I am meeting with some folks today and will follow up on this thread.

> 
> It is OK if more time is needed to check with CXL stakeholders. But looks like we all agree that the GCD attribute conversion table shall be updated to convert EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE to EFI_MEMORY_SP.
> So could we review and merge the conversion table update first, while we are waiting for the feedback from CXL stakeholders? If you agree, a pull request has been created for the conversion table update: https://github.com/tianocore/edk2/pull/5691.
> 

Yes, I agree with your PR and have already approved it. I think the
clarifications we are looking for would be in the UEFI spec and around
the memory type of EFI_MEMORY_SP, this PR is not controversial.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119412): https://edk2.groups.io/g/devel/message/119412
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Du Lin]

The pull request has been merged. Thanks Oliver and Liming.

BRs,
Lin, Du

-----Original Message-----
From: Oliver Smith-Denny <osde@linux.microsoft.com> 
Sent: Saturday, June 1, 2024 1:01 AM
To: devel@edk2.groups.io; Lin, Du <du.lin@intel.com>; mikuback@linux.microsoft.com
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

On 5/30/2024 8:44 PM, Du Lin wrote:
> Is there any feedback from CXL stakeholders?

I am meeting with some folks today and will follow up on this thread.

> 
> It is OK if more time is needed to check with CXL stakeholders. But looks like we all agree that the GCD attribute conversion table shall be updated to convert EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE to EFI_MEMORY_SP.
> So could we review and merge the conversion table update first, while we are waiting for the feedback from CXL stakeholders? If you agree, a pull request has been created for the conversion table update: https://github.com/tianocore/edk2/pull/5691.
> 

Yes, I agree with your PR and have already approved it. I think the clarifications we are looking for would be in the UEFI spec and around the memory type of EFI_MEMORY_SP, this PR is not controversial.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119414): https://edk2.groups.io/g/devel/message/119414
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Dhaval Sharma]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

Related to this, I also faced this issue where in order to prevent edk2 from allocating this memory I had to modify CoreFindFreePagesI
//
// Don't allocate out of Special-Purpose memory.
//
if ((Entry->Attribute & EFI_MEMORY_SP) != 0) {
continue;
}
Can't we add PCD based logic here to selectively NOT use SP memory for edk2 allocations? I think "reserved-memory" attr does not work well because it would force OS/drivers not to use it. Which is not what we want. We really want special drivers to make use of it.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119519): https://edk2.groups.io/g/devel/message/119519
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1426 bytes --]

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Oliver Smith-Denny]

On 6/9/2024 6:44 PM, Dhaval Sharma wrote:
> Related to this, I also faced this issue where in order to prevent edk2 
> from allocating this memory I had to modify CoreFindFreePagesI
>      //
>      // Don't allocate out of Special-Purpose memory.
>      //
>      if ((Entry->Attribute & EFI_MEMORY_SP) != 0) {
>        continue;
>      }
> Can't we add PCD based logic here to selectively NOT use SP memory for 
> edk2 allocations? I think "reserved-memory" attr does not work well 
> because it would force OS/drivers not to use it. Which is not what we 
> want. We really want special drivers to make use of it.

We've had some other conversations around this and I agree, we don't
want DXE Core to allocate this memory unless it has no other option.
I don't think this should be a PCD, I think DXE can make the statement
that it won't allocate EFI_MEMORY_SP memory unless it has run out of
other memory.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119550): https://edk2.groups.io/g/devel/message/119550
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Michael D Kinney]

Hi Oliver,

The DXE Core already has a policy to auto convert untested memory to
tested memory if the DXE Core runs out of memory.

Should we have a different memory type to allocate memory with the
EFI_MEMORY_SP attribute?

What would be the side effects of performing general allocations from
memory with the EFI_MEMORY_SP attribute?  If there are potentially bad 
side effects, then the DXE Core should never allocate from those
ranges and it is up to the platform to make sure there is enough normal
memory to boot.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver Smith-
> Denny
> Sent: Tuesday, June 11, 2024 10:04 AM
> To: devel@edk2.groups.io; dhaval@rivosinc.com; Michael Kubacki
> <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the
> EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute
> 
> On 6/9/2024 6:44 PM, Dhaval Sharma wrote:
> > Related to this, I also faced this issue where in order to prevent edk2
> > from allocating this memory I had to modify CoreFindFreePagesI
> >      //
> >      // Don't allocate out of Special-Purpose memory.
> >      //
> >      if ((Entry->Attribute & EFI_MEMORY_SP) != 0) {
> >        continue;
> >      }
> > Can't we add PCD based logic here to selectively NOT use SP memory for
> > edk2 allocations? I think "reserved-memory" attr does not work well
> > because it would force OS/drivers not to use it. Which is not what we
> > want. We really want special drivers to make use of it.
> 
> We've had some other conversations around this and I agree, we don't
> want DXE Core to allocate this memory unless it has no other option.
> I don't think this should be a PCD, I think DXE can make the statement
> that it won't allocate EFI_MEMORY_SP memory unless it has run out of
> other memory.
> 
> Thanks,
> Oliver
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119552): https://edk2.groups.io/g/devel/message/119552
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute

[Oliver Smith-Denny]

Hi Mike,

This conversation has gotten a little fragmented across this thread and
Dhaval put up a PR I commented on :).

On 6/11/2024 10:30 AM, Michael D Kinney wrote:
> Hi Oliver,
> 
> The DXE Core already has a policy to auto convert untested memory to
> tested memory if the DXE Core runs out of memory.
> 

Yeah, this was my suggestion on Dhaval's PR, that we follow a similar
memory promotion with EFI_MEMORY_SP as we do with untested memory. I
was planning to write this up, but haven't gotten to it yet.

> Should we have a different memory type to allocate memory with the
> EFI_MEMORY_SP attribute?

There is a lot of ambiguity in the PI/UEFI specs around what memory
types EFI_MEMORY_SP can be. This is further complicated by the CDAT
spec which uses EDKII-isms in defining the memory type (which it
probably shouldn't).

Amongst the various groups I have been chatting with (OS folks, this
mailing thread, firmware folks), there does seem to be a consensus that
the UEFI spec, at least, needs clarification here, as currently it just
says this attribute means special memory, which of course in and of
itself means nothing :).

However, these systems don't really exist yet. As these discussions
continue, I am changing my opinion to say less is more, for now. I think
we can't do too much architecting yet without knowing what these systems
are going to look like and what OSes will expect. If we get to a point
where we have systems with only remote attached memory (or the vast
majority remote attached memory) then having a separate memory type
probably does make sense. It would also solve the unenlightened OS
case where it wouldn't use SP memory if it was a new memory type,
whereas just the attribute on EfiConventionalMemory will have an
unenlightened OS use the memory.

> 
> What would be the side effects of performing general allocations from
> memory with the EFI_MEMORY_SP attribute?  If there are potentially bad
> side effects, then the DXE Core should never allocate from those
> ranges and it is up to the platform to make sure there is enough normal
> memory to boot.
> 

Right, I think we don't know enough yet, and so defaulting to UEFI not
using the memory and letting these systems mature a bit makes sense.
Inevitably if we build something now before these systems are fleshed
out, we will have built something that doesn't quite work.

Thanks,
Oliver



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119553): https://edk2.groups.io/g/devel/message/119553
Mute This Topic: https://groups.io/mt/106165072/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-