[RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[Leif Lindholm]

From: Leif Lindholm <Leif Lindholm quic_llindhol@quicinc.com>

We have mainly (well, as will become clear, in fact "exclusively") been using
sbsa-ref with the "max" CPU. But sbsa-ref was created with a default CPU of
Cortex-A57, which we have not updated along the way.

However, the "max" cpu has seen a bug where Linux boot fails around UEFI
ExitBootServices. Marcin Juszkiewicz has found the cause for that, but that
requires a patch to TF-A. (Has that been submitted upstream?)

Turns out that due to a change in upstream TF-A last year, all supported cpus
other than "max" fail to even boot UEFI fully, due to the top-level (TF-A)
Makefile defaulting to enabling the maximum ARM architectural version
(currently 8.6), in combination with not verifying all features at runtime
using the ID registers.

Since the *point* of sbsa-ref is to serve as a continuously evolving platform
tracking (with some obvious lag) the evolution of the ARM architecture and the
SystemReady specifications, I don't really want to restrict the enabled
feature set in TF-A to the Cortex-A57 one.

My preferred course of action would be to change the default cpu to max -
maybe even dropping support for other cpus. I would then step the version
field that was added to the DT. *But* this would break existing boots with
old TF-A that can currently boot Linux.

I did contemplate weaving this into the platform versioning, but truth is
I'm not convinced that would help ... and it would delay getting the
reworked memory map out.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Cc: Shashi Mallela <shashi.mallela@linaro.org>
Cc: Radoslaw Biernacki <rad@semihalf.com>

Leif Lindholm (1):
  hw/arm: use -cpu max by default for sbsa-ref

 hw/arm/sbsa-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[RFC PATCH 1/1] hw/arm: use -cpu max by default for sbsa-ref

[Leif Lindholm]

From: Leif Lindholm <Leif Lindholm quic_llindhol@quicinc.com>

Due to a change in upstream TF-A, boot fails (in edk2) on all cpus that
don't implement FEAT_DIT. The only currently emulated cpu that "supports"
this feature is "max". So switch to using that cpu by default for
sbsa-ref.

However, it is worth noting that the "max" cpu triggers another bug in
TF-A where Linux boot fails around UEFI ExitBootServices.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Cc: Shashi Mallela <shashi.mallela@linaro.org>
Cc: Radoslaw Biernacki <rad@semihalf.com>
---
 hw/arm/sbsa-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 4bb444684f..8bb5c0fc70 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -852,7 +852,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
 
     mc->init = sbsa_ref_init;
     mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
-    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a57");
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
     mc->max_cpus = 512;
     mc->pci_allow_0_address = true;
     mc->minimum_page_bits = 12;
-- 
2.30.2

Re: [RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[Peter Maydell]

On Wed, 9 Nov 2022 at 13:35, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> From: Leif Lindholm <Leif Lindholm quic_llindhol@quicinc.com>
>
> We have mainly (well, as will become clear, in fact "exclusively") been using
> sbsa-ref with the "max" CPU. But sbsa-ref was created with a default CPU of
> Cortex-A57, which we have not updated along the way.

So, I don't have a strong opinion on what sbsa-ref's CPU type
should be: if 'max' works better for the use case we should
change to it.

> However, the "max" cpu has seen a bug where Linux boot fails around UEFI
> ExitBootServices. Marcin Juszkiewicz has found the cause for that, but that
> requires a patch to TF-A. (Has that been submitted upstream?)
>
> Turns out that due to a change in upstream TF-A last year, all supported cpus
> other than "max" fail to even boot UEFI fully, due to the top-level (TF-A)
> Makefile defaulting to enabling the maximum ARM architectural version
> (currently 8.6), in combination with not verifying all features at runtime
> using the ID registers.

This seems to me straightforwardly to be a problem that should be
fixed in TF-A. "Default to the newest possible architecture version
and don't check ID registers" is a recipe for "can't boot on
anything". Many *hardware* CPUs aren't that new yet!

Part of the purpose of sbsa-ref is to find problems with the
software stack -- so we should expect that sometimes the answer
is "fix the software stack", not "change the model's behaviour".

> Since the *point* of sbsa-ref is to serve as a continuously evolving platform
> tracking (with some obvious lag) the evolution of the ARM architecture and the
> SystemReady specifications, I don't really want to restrict the enabled
> feature set in TF-A to the Cortex-A57 one.
>
> My preferred course of action would be to change the default cpu to max -
> maybe even dropping support for other cpus. I would then step the version
> field that was added to the DT. *But* this would break existing boots with
> old TF-A that can currently boot Linux.

An intermediate option would be to move forward to for instance
the neoverse-n1 CPU type.

If you want to use 'max' for sbsa-ref then you probably also want to
look at making sure that TF-A is doing the same thing Linux does
where it checks ID registers for feature presence before enabling
feature use, because 'max' is a moving target and relies on the
guest code doing the right thing with feature checks. In particular
it does not honour the nominal architectural requirements about
v8.x/v9.x levels, but may implement features from further in the
"future" than is strictly permitted given the absence of some
other older features.

thanks
-- PMM

Re: [RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[Marcin Juszkiewicz]

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

W dniu 9.11.2022 o 14:35, Leif Lindholm pisze:

> We have mainly (well, as will become clear, in fact "exclusively") been using
> sbsa-ref with the "max" CPU. But sbsa-ref was created with a default CPU of
> Cortex-A57, which we have not updated along the way.
> 
> However, the "max" cpu has seen a bug where Linux boot fails around UEFI
> ExitBootServices. Marcin Juszkiewicz has found the cause for that, but that
> requires a patch to TF-A. (Has that been submitted upstream?)

TF-A gerrit instance refuses to handle any of my SSH keys. RSA, ED25519 
ones.

The only change I did to TF-A was removal of coherent memory stuff as it 
was for a57/72 and blocked me from building it for a76/n1.

Attached it.

[-- Attachment #2: 0001-qemu_sbsa-drop-whole-coherent-memory-stuff.patch --]
[-- Type: text/x-patch, Size: 5148 bytes --]

From 246b709867b4fd9a62ea1ae6bce2f454971b548f Mon Sep 17 00:00:00 2001
From: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Date: Tue, 6 Sep 2022 13:48:40 +0200
Subject: [PATCH] qemu_sbsa: drop whole coherent memory stuff

We would like to use Cortex-A76/N1 cpu emulation in sbsa-ref and that
whole coherent memory stuff is not going there.
---
 plat/qemu/common/qemu_bl1_setup.c  |  4 +---
 plat/qemu/common/qemu_bl2_setup.c  |  4 +---
 plat/qemu/common/qemu_bl31_setup.c |  4 +---
 plat/qemu/common/qemu_common.c     | 12 +-----------
 plat/qemu/common/qemu_private.h    |  8 ++------
 plat/qemu/qemu_sbsa/platform.mk    |  3 +++
 6 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/plat/qemu/common/qemu_bl1_setup.c b/plat/qemu/common/qemu_bl1_setup.c
index 67f33273f..5243293ae 100644
--- a/plat/qemu/common/qemu_bl1_setup.c
+++ b/plat/qemu/common/qemu_bl1_setup.c
@@ -51,9 +51,7 @@ void bl1_plat_arch_setup(void)
 {
 	QEMU_CONFIGURE_BL1_MMU(bl1_tzram_layout.total_base,
 				bl1_tzram_layout.total_size,
-				BL_CODE_BASE, BL1_CODE_END,
-				BL1_RO_DATA_BASE, BL1_RO_DATA_END,
-				BL_COHERENT_RAM_BASE, BL_COHERENT_RAM_END);
+				BL_CODE_BASE, BL1_CODE_END);
 }
 
 void bl1_platform_setup(void)
diff --git a/plat/qemu/common/qemu_bl2_setup.c b/plat/qemu/common/qemu_bl2_setup.c
index 2c0da15b9..6e13ece72 100644
--- a/plat/qemu/common/qemu_bl2_setup.c
+++ b/plat/qemu/common/qemu_bl2_setup.c
@@ -92,9 +92,7 @@ void bl2_plat_arch_setup(void)
 {
 	QEMU_CONFIGURE_BL2_MMU(bl2_tzram_layout.total_base,
 			      bl2_tzram_layout.total_size,
-			      BL_CODE_BASE, BL_CODE_END,
-			      BL_RO_DATA_BASE, BL_RO_DATA_END,
-			      BL_COHERENT_RAM_BASE, BL_COHERENT_RAM_END);
+			      BL_CODE_BASE, BL_CODE_END);
 }
 
 /*******************************************************************************
diff --git a/plat/qemu/common/qemu_bl31_setup.c b/plat/qemu/common/qemu_bl31_setup.c
index 4f60eb163..61b6cbc10 100644
--- a/plat/qemu/common/qemu_bl31_setup.c
+++ b/plat/qemu/common/qemu_bl31_setup.c
@@ -65,9 +65,7 @@ void bl31_early_platform_setup2(u_register_t arg0, u_register_t arg1,
 void bl31_plat_arch_setup(void)
 {
 	qemu_configure_mmu_el3(BL31_BASE, (BL31_END - BL31_BASE),
-			      BL_CODE_BASE, BL_CODE_END,
-			      BL_RO_DATA_BASE, BL_RO_DATA_END,
-			      BL_COHERENT_RAM_BASE, BL_COHERENT_RAM_END);
+			      BL_CODE_BASE, BL_CODE_END);
 }
 
 static void qemu_gpio_init(void)
diff --git a/plat/qemu/common/qemu_common.c b/plat/qemu/common/qemu_common.c
index 0c184f49b..3d5b197ae 100644
--- a/plat/qemu/common/qemu_common.c
+++ b/plat/qemu/common/qemu_common.c
@@ -129,11 +129,7 @@ static const mmap_region_t plat_qemu_mmap[] = {
 	void qemu_configure_mmu_##_el(unsigned long total_base,	\
 				   unsigned long total_size,		\
 				   unsigned long code_start,		\
-				   unsigned long code_limit,		\
-				   unsigned long ro_start,		\
-				   unsigned long ro_limit,		\
-				   unsigned long coh_start,		\
-				   unsigned long coh_limit)		\
+				   unsigned long code_limit)		\
 	{								\
 		mmap_add_region(total_base, total_base,			\
 				total_size,				\
@@ -141,12 +137,6 @@ static const mmap_region_t plat_qemu_mmap[] = {
 		mmap_add_region(code_start, code_start,			\
 				code_limit - code_start,		\
 				MT_CODE | MT_SECURE);			\
-		mmap_add_region(ro_start, ro_start,			\
-				ro_limit - ro_start,			\
-				MT_RO_DATA | MT_SECURE);		\
-		mmap_add_region(coh_start, coh_start,			\
-				coh_limit - coh_start,			\
-				MT_DEVICE | MT_RW | MT_SECURE);		\
 		mmap_add(plat_qemu_mmap);				\
 		init_xlat_tables();					\
 									\
diff --git a/plat/qemu/common/qemu_private.h b/plat/qemu/common/qemu_private.h
index c313cb63f..ec59e2f31 100644
--- a/plat/qemu/common/qemu_private.h
+++ b/plat/qemu/common/qemu_private.h
@@ -16,14 +16,10 @@ void qemu_configure_mmu_svc_mon(unsigned long total_base,
 			unsigned long coh_start, unsigned long coh_limit);
 
 void qemu_configure_mmu_el1(unsigned long total_base, unsigned long total_size,
-			unsigned long code_start, unsigned long code_limit,
-			unsigned long ro_start, unsigned long ro_limit,
-			unsigned long coh_start, unsigned long coh_limit);
+			unsigned long code_start, unsigned long code_limit);
 
 void qemu_configure_mmu_el3(unsigned long total_base, unsigned long total_size,
-			unsigned long code_start, unsigned long code_limit,
-			unsigned long ro_start, unsigned long ro_limit,
-			unsigned long coh_start, unsigned long coh_limit);
+			unsigned long code_start, unsigned long code_limit);
 
 void plat_qemu_io_setup(void);
 unsigned int plat_qemu_calc_core_pos(u_register_t mpidr);
diff --git a/plat/qemu/qemu_sbsa/platform.mk b/plat/qemu/qemu_sbsa/platform.mk
index 5a6b1e11e..9c39f6190 100644
--- a/plat/qemu/qemu_sbsa/platform.mk
+++ b/plat/qemu/qemu_sbsa/platform.mk
@@ -49,6 +49,9 @@ BL1_SOURCES		+=	drivers/io/io_semihosting.c			\
 				${PLAT_QEMU_COMMON_PATH}/${ARCH}/plat_helpers.S	\
 				${PLAT_QEMU_COMMON_PATH}/qemu_bl1_setup.c
 
+USE_COHERENT_MEM := 0
+HW_ASSISTED_COHERENCY:=1
+
 BL1_SOURCES		+=	lib/cpus/aarch64/cortex_a57.S			\
 				lib/cpus/aarch64/cortex_a72.S			\
 				lib/cpus/aarch64/qemu_max.S			\
-- 
2.37.3