[PATCH v2 2/5] MdeMoudlePkg: introduce new PCD for Acpi/rsdp


Jianyong Wu
 

As there is lack of a machnism in Cloud Hypervisor to pass the base
address of Rsdp in Acpi, so a PCD varialbe is introduced here to
feed it.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
MdeModulePkg/MdeModulePkg.dec | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 148395511034..4c8baac35a9e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -910,6 +910,13 @@ [PcdsFixedAtBuild]
# @Expression 0x80000001 | (gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable == 0xFFFFFFFFFFFFFFFF || gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable <= 0x0FFFFFFFFFFFFFFF)
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable|0|UINT64|0x30001015

+ ##
+ # This is the physical address of rsdp which is the core struct of Acpi.
+ # Some hypervisor may has no way to pass rsdp address to the guest, so a PCD
+ # is worth for those.
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress|0x0|UINT64|0x30001056
+
## Progress Code for OS Loader LoadImage start.<BR><BR>
# PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000<BR>
# @Prompt Progress Code for OS Loader LoadImage start.
--
2.17.1


Sami Mujawar
 

Hi Jianyon,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 17/05/2021 07:50 AM, Jianyong Wu wrote:
As there is lack of a machnism in Cloud Hypervisor to pass the base
address of Rsdp in Acpi, so a PCD varialbe is introduced here to
[SAMI] Please fix the following typos: 'machnism' & 'varialbe'
feed it.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
MdeModulePkg/MdeModulePkg.dec | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 148395511034..4c8baac35a9e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -910,6 +910,13 @@ [PcdsFixedAtBuild]
# @Expression 0x80000001 | (gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable == 0xFFFFFFFFFFFFFFFF || gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable <= 0x0FFFFFFFFFFFFFFF)
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable|0|UINT64|0x30001015
+ ##
+ # This is the physical address of rsdp which is the core struct of Acpi.
+ # Some hypervisor may has no way to pass rsdp address to the guest, so a PCD
+ # is worth for those.
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress|0x0|UINT64|0x30001056
[SAMI] Can this PCD be defined in ArmVirtPkg\ArmVirtPkg.dec ?
+
## Progress Code for OS Loader LoadImage start.<BR><BR>
# PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000<BR>
# @Prompt Progress Code for OS Loader LoadImage start.


Laszlo Ersek
 

On 05/17/21 08:50, Jianyong Wu wrote:
As there is lack of a machnism in Cloud Hypervisor to pass the base
address of Rsdp in Acpi, so a PCD varialbe is introduced here to
feed it.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
MdeModulePkg/MdeModulePkg.dec | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 148395511034..4c8baac35a9e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -910,6 +910,13 @@ [PcdsFixedAtBuild]
# @Expression 0x80000001 | (gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable == 0xFFFFFFFFFFFFFFFF || gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable <= 0x0FFFFFFFFFFFFFFF)
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable|0|UINT64|0x30001015

+ ##
+ # This is the physical address of rsdp which is the core struct of Acpi.
+ # Some hypervisor may has no way to pass rsdp address to the guest, so a PCD
+ # is worth for those.
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress|0x0|UINT64|0x30001056
+
## Progress Code for OS Loader LoadImage start.<BR><BR>
# PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000<BR>
# @Prompt Progress Code for OS Loader LoadImage start.
This PCD is not useful enough to be placed in MdeModulePkg -- it is only
used in the next two patches, which are for ArmVirtPkg.

(1) Therefore, please add the PCD to the "ArmVirtPkg.dec" file.

(2) The PCD should arguably refer to "CloudHv" in the name.

(3) In my opinion, this patch (once reimplemented for ArmVirtPkg.dec)
should be squashed into the CloudHvAcpiPlatformDxe patch. The PCD is
being introduced *for* CloudHvAcpiPlatformDxe, and *only* for that
driver. In such cases, we usually keep the DEC modifications in the same
patch as the driver addition, assuming the PCD goes in the same package
as the driver.

(4) "some hypervisor" in the DEC comment is bogus. Please be as explicit
about the use case as possible.

Thanks
Laszlo


Jianyong Wu
 

Hi Sami,

-----Original Message-----
From: Sami Mujawar <Sami.Mujawar@...>
Sent: Wednesday, May 19, 2021 4:26 AM
To: Jianyong Wu <Jianyong.Wu@...>; devel@edk2.groups.io;
lersek@...; ardb+tianocore@...
Cc: hao.a.wu@...; Justin He <Justin.He@...>; Jian J Wang
<jian.j.wang@...>; nd <nd@...>
Subject: Re: [PATCH v2 2/5] MdeMoudlePkg: introduce new PCD for
Acpi/rsdp

Hi Jianyon,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 17/05/2021 07:50 AM, Jianyong Wu wrote:
As there is lack of a machnism in Cloud Hypervisor to pass the base
address of Rsdp in Acpi, so a PCD varialbe is introduced here to
[SAMI] Please fix the following typos: 'machnism' & 'varialbe'
feed it.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
MdeModulePkg/MdeModulePkg.dec | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec
b/MdeModulePkg/MdeModulePkg.dec index
148395511034..4c8baac35a9e
100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -910,6 +910,13 @@ [PcdsFixedAtBuild]
# @Expression 0x80000001 |
(gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable
== 0xFFFFFFFFFFFFFFFF ||
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable <=
0x0FFFFFFFFFFFFFFF)

gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable|0|
UINT6
4|0x30001015

+ ##
+ # This is the physical address of rsdp which is the core struct of Acpi.
+ # Some hypervisor may has no way to pass rsdp address to the guest,
+ so a PCD # is worth for those.
+ #
+
+
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress|0x0|UINT64
|0x3
+ 0001056
[SAMI] Can this PCD be defined in ArmVirtPkg\ArmVirtPkg.dec ?
Ok, It's better to do this change.

Thanks
Jianyong
+
## Progress Code for OS Loader LoadImage start.<BR><BR>
# PROGRESS_CODE_OS_LOADER_LOAD =
(EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) =
0x03058000<BR>
# @Prompt Progress Code for OS Loader LoadImage start.


Jianyong Wu
 

Hi Laszlo,

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Wednesday, May 19, 2021 2:17 PM
To: devel@edk2.groups.io; Jianyong Wu <Jianyong.Wu@...>;
ardb+tianocore@...; Sami Mujawar <Sami.Mujawar@...>
Cc: hao.a.wu@...; Justin He <Justin.He@...>; Jian J Wang
<jian.j.wang@...>
Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeMoudlePkg: introduce new
PCD for Acpi/rsdp

On 05/17/21 08:50, Jianyong Wu wrote:
As there is lack of a machnism in Cloud Hypervisor to pass the base
address of Rsdp in Acpi, so a PCD varialbe is introduced here to feed
it.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
MdeModulePkg/MdeModulePkg.dec | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec
b/MdeModulePkg/MdeModulePkg.dec index
148395511034..4c8baac35a9e
100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -910,6 +910,13 @@ [PcdsFixedAtBuild]
# @Expression 0x80000001 |
(gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable
== 0xFFFFFFFFFFFFFFFF ||
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable <=
0x0FFFFFFFFFFFFFFF)

gEfiMdeModulePkgTokenSpaceGuid.PcdLoadModuleAtFixAddressEnable|0|
UINT6
4|0x30001015

+ ##
+ # This is the physical address of rsdp which is the core struct of Acpi.
+ # Some hypervisor may has no way to pass rsdp address to the guest,
+ so a PCD # is worth for those.
+ #
+
+
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress|0x0|UINT64
|0x3
+ 0001056
+
## Progress Code for OS Loader LoadImage start.<BR><BR>
# PROGRESS_CODE_OS_LOADER_LOAD =
(EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) =
0x03058000<BR>
# @Prompt Progress Code for OS Loader LoadImage start.
This PCD is not useful enough to be placed in MdeModulePkg -- it is only
used in the next two patches, which are for ArmVirtPkg.

(1) Therefore, please add the PCD to the "ArmVirtPkg.dec" file.

(2) The PCD should arguably refer to "CloudHv" in the name.

(3) In my opinion, this patch (once reimplemented for ArmVirtPkg.dec)
should be squashed into the CloudHvAcpiPlatformDxe patch. The PCD is
being introduced *for* CloudHvAcpiPlatformDxe, and *only* for that driver.
In such cases, we usually keep the DEC modifications in the same patch as
the driver addition, assuming the PCD goes in the same package as the driver.

(4) "some hypervisor" in the DEC comment is bogus. Please be as explicit
about the use case as possible.
OK, that's better.

Thanks
Jianyong


Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.