devel@edk2.groups.io | [PATCH] OvmfPkg/XenPlatformPei: Relocate shared

Home Messages Hashtags Subgroups Calendar

Date Date 1 - 7 of 7

[PATCH] OvmfPkg/XenPlatformPei: Relocate shared_info page mapping

Anthony PERARD

From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
-- 
Anthony PERARD

Laszlo Ersek

#77256

On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.

Thanks
Laszlo

Laszlo Ersek

#77266

On 06/29/21 12:35, Andrew Cooper wrote:
On 29/06/2021 11:07, Jan Beulich wrote:
On 29.06.2021 11:20, Laszlo Ersek wrote:
On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.Well, I can informally ack that a move like this is needed, but I can't
really give an Acked-by on the change, as I know next to nothing about
ovmf, and hence I cannot, for example, tell whether the chosen new
location is okay to use there.That bit is easy, although it probably does warrant a code comment.  The
choice of location doesn't matter specifically (so long as it's not too
high for 32bit toolstacks to cope with), but wants to be something which
isn't RAM, and ideally doesn't shatter a superpage.

The various MMIO ranges (IO-APIC, TPM, LAPIC) are 2M aligned, so LAPIC +
1M is firmly in the middle of a range not used by anything else, and
with APICV/AVIC, the LAPIC does have a 4k mapping to the sink page.I'm OK with informal ACKs of course, just please let me know when you
guys feel it's OK for me to merge the patch. I saw the CCs and so I
didn't want to hasten and merge the patch before you could object.

Thanks
Laszlo

Jan Beulich <jbeulich@...>

#77267

On 29.06.2021 11:20, Laszlo Ersek wrote:
On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.Well, I can informally ack that a move like this is needed, but I can't
really give an Acked-by on the change, as I know next to nothing about
ovmf, and hence I cannot, for example, tell whether the chosen new
location is okay to use there.

Jan

Andrew Cooper <andrew.cooper3@...>

#77268

On 29/06/2021 11:07, Jan Beulich wrote:
On 29.06.2021 11:20, Laszlo Ersek wrote:
On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.Well, I can informally ack that a move like this is needed, but I can't
really give an Acked-by on the change, as I know next to nothing about
ovmf, and hence I cannot, for example, tell whether the chosen new
location is okay to use there.That bit is easy, although it probably does warrant a code comment.  The
choice of location doesn't matter specifically (so long as it's not too
high for 32bit toolstacks to cope with), but wants to be something which
isn't RAM, and ideally doesn't shatter a superpage.

The various MMIO ranges (IO-APIC, TPM, LAPIC) are 2M aligned, so LAPIC +
1M is firmly in the middle of a range not used by anything else, and
with APICV/AVIC, the LAPIC does have a 4k mapping to the sink page.

~Andrew

Jan Beulich <jbeulich@...>

#77269

On 29.06.2021 13:29, Laszlo Ersek wrote:
On 06/29/21 12:35, Andrew Cooper wrote:
On 29/06/2021 11:07, Jan Beulich wrote:
On 29.06.2021 11:20, Laszlo Ersek wrote:
On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.Well, I can informally ack that a move like this is needed, but I can't
really give an Acked-by on the change, as I know next to nothing about
ovmf, and hence I cannot, for example, tell whether the chosen new
location is okay to use there.That bit is easy, although it probably does warrant a code comment.  The
choice of location doesn't matter specifically (so long as it's not too
high for 32bit toolstacks to cope with), but wants to be something which
isn't RAM, and ideally doesn't shatter a superpage.

The various MMIO ranges (IO-APIC, TPM, LAPIC) are 2M aligned, so LAPIC +
1M is firmly in the middle of a range not used by anything else, and
with APICV/AVIC, the LAPIC does have a 4k mapping to the sink page.I'm OK with informal ACKs of course, just please let me know when you
guys feel it's OK for me to merge the patch. I saw the CCs and so I
didn't want to hasten and merge the patch before you could object.I think you are fine going ahead as long as from the ovmf side all is
good here.

Jan

Laszlo Ersek

#77276

On 06/29/21 13:41, Jan Beulich via groups.io wrote:
On 29.06.2021 13:29, Laszlo Ersek wrote:
On 06/29/21 12:35, Andrew Cooper wrote:
On 29/06/2021 11:07, Jan Beulich wrote:
On 29.06.2021 11:20, Laszlo Ersek wrote:
On 06/28/21 15:23, Anthony PERARD via groups.io wrote:
From: Anthony PERARD <anthony.perard@...>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@...>
CC: Andrew Cooper <andrew.cooper3@...>
Signed-off-by: Anthony PERARD <anthony.perard@...>
---
 OvmfPkg/XenPlatformPei/Xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
   EFI_STATUS            Status;
 
 
-  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
   Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
Acked-by: Laszlo Ersek <lersek@...>

I guess I should merge this after Jan and/or Andrew ack it.Well, I can informally ack that a move like this is needed, but I can't
really give an Acked-by on the change, as I know next to nothing about
ovmf, and hence I cannot, for example, tell whether the chosen new
location is okay to use there.That bit is easy, although it probably does warrant a code comment.  The
choice of location doesn't matter specifically (so long as it's not too
high for 32bit toolstacks to cope with), but wants to be something which
isn't RAM, and ideally doesn't shatter a superpage.

The various MMIO ranges (IO-APIC, TPM, LAPIC) are 2M aligned, so LAPIC +
1M is firmly in the middle of a range not used by anything else, and
with APICV/AVIC, the LAPIC does have a 4k mapping to the sink page.I'm OK with informal ACKs of course, just please let me know when you
guys feel it's OK for me to merge the patch. I saw the CCs and so I
didn't want to hasten and merge the patch before you could object.I think you are fine going ahead as long as from the ovmf side all is
good here.Merged as commit b37cfdd28071, via
<https://github.com/tianocore/edk2/pull/1771>.

I had to replace the "CC:" lines with "Cc:" ones, in order to placate
"PatchCheck.py".

Thanks
Laszlo

1 - 7 of 7

Previous Topic Next Topic

Home Hashtags Subgroups Calendar About Features Pricing Updates Terms Help Twitter