Peter Maydell (2):

target/arm: take HSTR traps of cp15 accesses to EL2, not EL1

hw/intc/arm_gicv3: ICC_HPPIR* return SPURIOUS if int group is disabled

docs/system/arm/sbsa.rst | 35 +++++++++++++++++------

hw/arm/raspi4b.c | 4 +++

hw/intc/arm_gicv3_cpuif.c | 4 +--

target/arm/tcg/translate.c | 2 +-

tests/qtest/stm32l4x5_gpio-test.c | 59 +++++++++++++++++++++++----------------

5 files changed, 68 insertions(+), 36 deletions(-)

The HSTR_EL2 register allows the hypervisor to trap AArch32 EL1 and

EL0 accesses to cp15 registers. We incorrectly implemented this so

they trap to EL1 when we detect the need for a HSTR trap at code

generation time. (The check in access_check_cp_reg() which we do at

runtime to catch traps from EL0 is correctly routing them to EL2.)

Use the correct target EL when generating the code to take the trap.

Cc: qemu-stable@nongnu.org

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2226

Fixes: 049edada5e93df ("target/arm: Make HSTR_EL2 traps take priority over UNDEF-at-EL1")

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Message-id: 20240325133116.2075362-1-peter.maydell@linaro.org

---

target/arm/tcg/translate.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/tcg/translate.c

+++ b/target/arm/tcg/translate.c

@@ -XXX,XX +XXX,XX @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,

tcg_gen_andi_i32(t, t, 1u << maskbit);

tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, over.label);

- gen_exception_insn(s, 0, EXCP_UDEF, syndrome);

+ gen_exception_insn_el(s, 0, EXCP_UDEF, syndrome, 2);

/*

* gen_exception_insn() will set is_jmp to DISAS_NORETURN,

* but since we're conditionally branching over it, we want

--

2.34.1

From: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Hardware of sbsa-ref board is nowadays defined by both BSA and SBSA

specifications. Then BBR defines firmware interface.

Added note about DeviceTree data passed from QEMU to firmware. It is

very minimal and provides only data we use in firmware.

Added NUMA information to list of things reported by DeviceTree.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Message-id: 20240328163851.1386176-1-marcin.juszkiewicz@linaro.org

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

docs/system/arm/sbsa.rst | 35 ++++++++++++++++++++++++++---------

1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/system/arm/sbsa.rst

+++ b/docs/system/arm/sbsa.rst

@@ -XXX,XX +XXX,XX @@

Arm Server Base System Architecture Reference board (``sbsa-ref``)

==================================================================

-While the ``virt`` board is a generic board platform that doesn't match

-any real hardware the ``sbsa-ref`` board intends to look like real

-hardware. The `Server Base System Architecture

-<https://developer.arm.com/documentation/den0029/latest>`_ defines a

-minimum base line of hardware support and importantly how the firmware

-reports that to any operating system.

+The ``sbsa-ref`` board intends to look like real hardware (while the ``virt``

+board is a generic board platform that doesn't match any real hardware).

+

+The hardware part is defined by two specifications:

+

+ - `Base System Architecture <https://developer.arm.com/documentation/den0094/>`__ (BSA)

+ - `Server Base System Architecture <https://developer.arm.com/documentation/den0029/>`__ (SBSA)

+

+The `Arm Base Boot Requirements <https://developer.arm.com/documentation/den0044/>`__ (BBR)

+specification defines how the firmware reports that to any operating system.

It is intended to be a machine for developing firmware and testing

standards compliance with operating systems.

@@ -XXX,XX +XXX,XX @@ includes both internal hardware and parts affected by the qemu command line

(i.e. CPUs and memory). As a result it must have a firmware specifically built

to expect a certain hardware layout (as you would in a real machine).

+Note

+''''

+

+QEMU provides the guest EL3 firmware with minimal information about hardware

+platform using minimalistic devicetree. This is not a Linux devicetree. It is

+not even a firmware devicetree.

+

+It is information passed from QEMU to describe the information a hardware

+platform would have other mechanisms to discover at runtime, that are affected

+by the QEMU command line.

+

+Ultimately this devicetree may be replaced by IPC calls to an emulated SCP.

+

DeviceTree information

''''''''''''''''''''''

-The devicetree provided by the board model to the firmware is not intended

-to be a complete compliant DT. It currently reports:

+The devicetree reports:

- CPUs

- memory

- platform version

- GIC addresses

+ - NUMA node id for CPUs and memory

Platform version

''''''''''''''''

@@ -XXX,XX +XXX,XX @@ Platform version changes:

GIC ITS information is present in devicetree.

0.3

- The USB controller is an XHCI device, not EHCI

+ The USB controller is an XHCI device, not EHCI.

--

2.34.1

If the group of the highest priority pending interrupt is disabled

via ICC_IGRPEN*, the ICC_HPPIR* registers should return

INTID_SPURIOUS, not the interrupt ID. (See the GIC architecture

specification pseudocode functions ICC_HPPIR1_EL1[] and

HighestPriorityPendingInterrupt().)

Make HPPIR reads honour the group disable, the way we already do

when determining whether to preempt in icc_hppi_can_preempt().

Cc: qemu-stable@nongnu.org

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Message-id: 20240328153333.2522667-1-peter.maydell@linaro.org

---

hw/intc/arm_gicv3_cpuif.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/intc/arm_gicv3_cpuif.c

+++ b/hw/intc/arm_gicv3_cpuif.c

@@ -XXX,XX +XXX,XX @@ static uint64_t icc_hppir0_value(GICv3CPUState *cs, CPUARMState *env)

*/

bool irq_is_secure;

- if (cs->hppi.prio == 0xff) {

+ if (icc_no_enabled_hppi(cs)) {

return INTID_SPURIOUS;

}

@@ -XXX,XX +XXX,XX @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env)

*/

bool irq_is_secure;

- if (cs->hppi.prio == 0xff) {

+ if (icc_no_enabled_hppi(cs)) {

return INTID_SPURIOUS;

}

--

2.34.1

From: Cédric Le Goater <clg@redhat.com>

The test mangles the GPIO address and the pin number in the

qtest_add_data_func data parameter. Doing so, it assumes that the host

pointer size is always 64-bit, which breaks on 32-bit :

../tests/qtest/stm32l4x5_gpio-test.c: In function ‘test_gpio_output_mode’:

../tests/qtest/stm32l4x5_gpio-test.c:272:25: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

272 | unsigned int pin = ((uint64_t)data) & 0xF;

| ^

../tests/qtest/stm32l4x5_gpio-test.c:273:22: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

273 | uint32_t gpio = ((uint64_t)data) >> 32;

| ^

To fix, improve the mangling of the GPIO address and pin number fields

by using GPIO_SIZE so that the resulting value fits in a 32-bit pointer.

While at it, include some helpers to hide the details.

Cc: Arnaud Minier <arnaud.minier@telecom-paris.fr>

Cc: Inès Varhol <ines.varhol@telecom-paris.fr>

Signed-off-by: Cédric Le Goater <clg@redhat.com>

Message-id: 20240329092747.298259-1-clg@redhat.com

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

tests/qtest/stm32l4x5_gpio-test.c | 59 ++++++++++++++++++-------------

1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/tests/qtest/stm32l4x5_gpio-test.c b/tests/qtest/stm32l4x5_gpio-test.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/stm32l4x5_gpio-test.c

+++ b/tests/qtest/stm32l4x5_gpio-test.c

@@ -XXX,XX +XXX,XX @@ const uint32_t idr_reset[NUM_GPIOS] = {

0x00000000

};

+#define PIN_MASK 0xF

+#define GPIO_ADDR_MASK (~(GPIO_SIZE - 1))

+

+static inline void *test_data(uint32_t gpio_addr, uint8_t pin)

+{

+ return (void *)(uintptr_t)((gpio_addr & GPIO_ADDR_MASK) | (pin & PIN_MASK));

+}

+

+#define test_gpio_addr(data) ((uintptr_t)(data) & GPIO_ADDR_MASK)

+#define test_pin(data) ((uintptr_t)(data) & PIN_MASK)

+

static uint32_t gpio_readl(unsigned int gpio, unsigned int offset)

{

return readl(gpio + offset);

@@ -XXX,XX +XXX,XX @@ static void test_gpio_output_mode(const void *data)

* Additionally, it checks that values written to ODR

* when not in output mode are stored and not discarded.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

unsigned int gpio_id = get_gpio_id(gpio);

qtest_irq_intercept_in(global_qtest, "/machine/soc/syscfg");

@@ -XXX,XX +XXX,XX @@ static void test_gpio_input_mode(const void *data)

* corresponding GPIO line high/low : it should set the

* right bit in IDR and send an irq to syscfg.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

unsigned int gpio_id = get_gpio_id(gpio);

qtest_irq_intercept_in(global_qtest, "/machine/soc/syscfg");

@@ -XXX,XX +XXX,XX @@ static void test_pull_up_pull_down(const void *data)

* Test that a floating pin with pull-up sets the pin

* high and vice-versa.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

unsigned int gpio_id = get_gpio_id(gpio);

qtest_irq_intercept_in(global_qtest, "/machine/soc/syscfg");

@@ -XXX,XX +XXX,XX @@ static void test_push_pull(const void *data)

* disconnects the pin, that the pin can't be set or reset

* externally afterwards.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

uint32_t gpio2 = GPIO_BASE_ADDR + (GPIO_H - gpio);

qtest_irq_intercept_in(global_qtest, "/machine/soc/syscfg");

@@ -XXX,XX +XXX,XX @@ static void test_open_drain(const void *data)

* However a pin set low externally shouldn't be disconnected,

* and it can be set low externally when in open-drain mode.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

uint32_t gpio2 = GPIO_BASE_ADDR + (GPIO_H - gpio);

qtest_irq_intercept_in(global_qtest, "/machine/soc/syscfg");

@@ -XXX,XX +XXX,XX @@ static void test_bsrr_brr(const void *data)

* has the desired effect on ODR.

* In BSRR, BSx has priority over BRx.

*/

- unsigned int pin = ((uint64_t)data) & 0xF;

- uint32_t gpio = ((uint64_t)data) >> 32;

+ unsigned int pin = test_pin(data);

+ uint32_t gpio = test_gpio_addr(data);

gpio_writel(gpio, BSRR, (1 << pin));

g_assert_cmphex(gpio_readl(gpio, ODR), ==, reset(gpio, ODR) | (1 << pin));

@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)

* is problematic since the pin was already high.

*/

qtest_add_data_func("stm32l4x5/gpio/test_gpioc5_output_mode",

- (void *)((uint64_t)GPIO_C << 32 | 5),

+ test_data(GPIO_C, 5),

test_gpio_output_mode);

qtest_add_data_func("stm32l4x5/gpio/test_gpioh3_output_mode",

- (void *)((uint64_t)GPIO_H << 32 | 3),

+ test_data(GPIO_H, 3),

test_gpio_output_mode);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_input_mode1",

- (void *)((uint64_t)GPIO_D << 32 | 6),

+ test_data(GPIO_D, 6),

test_gpio_input_mode);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_input_mode2",

- (void *)((uint64_t)GPIO_C << 32 | 10),

+ test_data(GPIO_C, 10),

test_gpio_input_mode);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_pull_up_pull_down1",

- (void *)((uint64_t)GPIO_B << 32 | 5),

+ test_data(GPIO_B, 5),

test_pull_up_pull_down);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_pull_up_pull_down2",

- (void *)((uint64_t)GPIO_F << 32 | 1),

+ test_data(GPIO_F, 1),

test_pull_up_pull_down);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_push_pull1",

- (void *)((uint64_t)GPIO_G << 32 | 6),

+ test_data(GPIO_G, 6),

test_push_pull);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_push_pull2",

- (void *)((uint64_t)GPIO_H << 32 | 3),

+ test_data(GPIO_H, 3),

test_push_pull);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_open_drain1",

- (void *)((uint64_t)GPIO_C << 32 | 4),

+ test_data(GPIO_C, 4),

test_open_drain);

qtest_add_data_func("stm32l4x5/gpio/test_gpio_open_drain2",

- (void *)((uint64_t)GPIO_E << 32 | 11),

+ test_data(GPIO_E, 11),

test_open_drain);

qtest_add_data_func("stm32l4x5/gpio/test_bsrr_brr1",

- (void *)((uint64_t)GPIO_A << 32 | 12),

+ test_data(GPIO_A, 12),

test_bsrr_brr);

qtest_add_data_func("stm32l4x5/gpio/test_bsrr_brr2",

- (void *)((uint64_t)GPIO_D << 32 | 0),

+ test_data(GPIO_D, 0),

test_bsrr_brr);

qtest_start("-machine b-l475e-iot01a");

--

2.34.1