Hi; here's the first arm pullreq for the 8.2 cycle. These are pretty much all bug fixes (mostly for the experimental FEAT_RME), rather than any major features. -- PMM The following changes since commit b0dd9a7d6dd15a6898e9c585b521e6bec79b25aa: Open 8.2 development tree (2023-08-22 07:14:07 -0700) are available in the Git repository at: https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20230824 for you to fetch changes up to cd1e4db73646006039f25879af3bff55b2295ff3: target/arm: Fix 64-bit SSRA (2023-08-22 17:31:14 +0100) ---------------------------------------------------------------- target-arm queue: * hw/gpio/nrf51: implement DETECT signal * accel/kvm: Specify default IPA size for arm64 * ptw: refactor, fix some FEAT_RME bugs * target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types * target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK * Fix SME ST1Q * Fix 64-bit SSRA ---------------------------------------------------------------- Akihiko Odaki (6): kvm: Introduce kvm_arch_get_default_type hook accel/kvm: Specify default IPA size for arm64 mips: Report an error when KVM_VM_MIPS_VZ is unavailable accel/kvm: Use negative KVM type for error propagation accel/kvm: Free as when an error occurred accel/kvm: Make kvm_dirty_ring_reaper_init() void Chris Laplante (6): hw/gpio/nrf51: implement DETECT signal qtest: factor out qtest_install_gpio_out_intercept qtest: implement named interception of out-GPIO qtest: bail from irq_intercept_in if name is specified qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed qtest: microbit-test: add tests for nRF51 DETECT Jean-Philippe Brucker (6): target/arm/ptw: Load stage-2 tables from realm physical space target/arm/helper: Fix tlbmask and tlbbits for TLBI VAE2* target/arm: Skip granule protection checks for AT instructions target/arm: Pass security space rather than flag for AT instructions target/arm/helper: Check SCR_EL3.{NSE, NS} encoding for AT instructions target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK Peter Maydell (15): target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2 faults target/arm/ptw: Set s1ns bit in fault info more consistently target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and get_phys_addr_disabled() target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled() target/arm/ptw: Pass an ARMSecuritySpace to arm_hcr_el2_eff_secstate() target/arm: Pass an ARMSecuritySpace to arm_is_el2_enabled_secstate() target/arm/ptw: Only fold in NSTable bit effects in Secure state target/arm/ptw: Remove last uses of ptw->in_secure target/arm/ptw: Remove S1Translate::in_secure target/arm/ptw: Drop S1Translate::out_secure target/arm/ptw: Set attributes correctly for MMU disabled data accesses target/arm/ptw: Check for block descriptors at invalid levels target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage 1 ptw target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types Richard Henderson (2): target/arm: Fix SME ST1Q target/arm: Fix 64-bit SSRA include/hw/gpio/nrf51_gpio.h | 1 + include/sysemu/kvm.h | 2 + target/arm/cpu.h | 19 ++-- target/arm/internals.h | 25 ++--- target/mips/kvm_mips.h | 9 -- tests/qtest/libqtest.h | 11 +++ accel/kvm/kvm-all.c | 19 ++-- hw/arm/virt.c | 2 +- hw/gpio/nrf51_gpio.c | 14 ++- hw/mips/loongson3_virt.c | 2 - hw/ppc/spapr.c | 2 +- softmmu/qtest.c | 52 +++++++--- target/arm/cpu.c | 6 ++ target/arm/helper.c | 207 ++++++++++++++++++++++++++++---------- target/arm/kvm.c | 7 ++ target/arm/ptw.c | 231 ++++++++++++++++++++++++++----------------- target/arm/tcg/sme_helper.c | 2 +- target/arm/tcg/translate.c | 2 +- target/i386/kvm/kvm.c | 5 + target/mips/kvm.c | 3 +- target/ppc/kvm.c | 5 + target/riscv/kvm.c | 5 + target/s390x/kvm/kvm.c | 5 + tests/qtest/libqtest.c | 6 ++ tests/qtest/microbit-test.c | 44 +++++++++ target/arm/trace-events | 7 +- 26 files changed, 494 insertions(+), 199 deletions(-) From: Chris Laplante <chris@laplante.io> Implement nRF51 DETECT signal in the GPIO peripheral. The reference manual makes mention of a per-pin DETECT signal, but these are not exposed to the user. See https://devzone.nordicsemi.com/f/nordic-q-a/39858/gpio-per-pin-detect-signal-available for more information. Currently, I don't see a reason to model these. Signed-off-by: Chris Laplante <chris@laplante.io> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230728160324.1159090-2-chris@laplante.io Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/gpio/nrf51_gpio.h | 1 + hw/gpio/nrf51_gpio.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/gpio/nrf51_gpio.h +++ b/include/hw/gpio/nrf51_gpio.h @@ -XXX,XX +XXX,XX @@ struct NRF51GPIOState { uint32_t old_out_connected; qemu_irq output[NRF51_GPIO_PINS]; + qemu_irq detect; }; diff --git a/hw/gpio/nrf51_gpio.c b/hw/gpio/nrf51_gpio.c index XXXXXXX..XXXXXXX 100644 --- a/hw/gpio/nrf51_gpio.c +++ b/hw/gpio/nrf51_gpio.c @@ -XXX,XX +XXX,XX @@ static void update_state(NRF51GPIOState *s) int pull; size_t i; bool connected_out, dir, connected_in, out, in, input; + bool assert_detect = false; for (i = 0; i < NRF51_GPIO_PINS; i++) { pull = pull_value(s->cnf[i]); @@ -XXX,XX +XXX,XX @@ static void update_state(NRF51GPIOState *s) qemu_log_mask(LOG_GUEST_ERROR, "GPIO pin %zu short circuited\n", i); } - if (!connected_in) { + if (connected_in) { + uint32_t detect_config = extract32(s->cnf[i], 16, 2); + if ((detect_config == 2) && (in == 1)) { + assert_detect = true; + } + if ((detect_config == 3) && (in == 0)) { + assert_detect = true; + } + } else { /* * Floating input: the output stimulates IN if connected, * otherwise pull-up/pull-down resistors put a value on both @@ -XXX,XX +XXX,XX @@ static void update_state(NRF51GPIOState *s) } update_output_irq(s, i, connected_out, out); } + + qemu_set_irq(s->detect, assert_detect); } /* @@ -XXX,XX +XXX,XX @@ static void nrf51_gpio_init(Object *obj) qdev_init_gpio_in(DEVICE(s), nrf51_gpio_set, NRF51_GPIO_PINS); qdev_init_gpio_out(DEVICE(s), s->output, NRF51_GPIO_PINS); + qdev_init_gpio_out_named(DEVICE(s), &s->detect, "detect", 1); } static void nrf51_gpio_class_init(ObjectClass *klass, void *data) -- 2.34.1 From: Chris Laplante <chris@laplante.io> Signed-off-by: Chris Laplante <chris@laplante.io> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230728160324.1159090-3-chris@laplante.io Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- softmmu/qtest.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/softmmu/qtest.c b/softmmu/qtest.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -XXX,XX +XXX,XX @@ void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words)) process_command_cb = pc_cb; } +static void qtest_install_gpio_out_intercept(DeviceState *dev, const char *name, int n) +{ + qemu_irq *disconnected = g_new0(qemu_irq, 1); + qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler, + disconnected, n); + + *disconnected = qdev_intercept_gpio_out(dev, icpt, name, n); +} + static void qtest_process_command(CharBackend *chr, gchar **words) { const gchar *command; @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) if (words[0][14] == 'o') { int i; for (i = 0; i < ngl->num_out; ++i) { - qemu_irq *disconnected = g_new0(qemu_irq, 1); - qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler, - disconnected, i); - - *disconnected = qdev_intercept_gpio_out(dev, icpt, - ngl->name, i); + qtest_install_gpio_out_intercept(dev, ngl->name, i); } } else { qemu_irq_intercept_in(ngl->in, qtest_irq_handler, -- 2.34.1 From: Chris Laplante <chris@laplante.io> Adds qtest_irq_intercept_out_named method, which utilizes a new optional name parameter to the irq_intercept_out qtest command. Signed-off-by: Chris Laplante <chris@laplante.io> Message-id: 20230728160324.1159090-4-chris@laplante.io Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- tests/qtest/libqtest.h | 11 +++++++++++ softmmu/qtest.c | 18 ++++++++++-------- tests/qtest/libqtest.c | 6 ++++++ 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -XXX,XX +XXX,XX @@ void qtest_irq_intercept_in(QTestState *s, const char *string); */ void qtest_irq_intercept_out(QTestState *s, const char *string); +/** + * qtest_irq_intercept_out_named: + * @s: #QTestState instance to operate on. + * @qom_path: QOM path of a device. + * @name: Name of the GPIO out pin + * + * Associate a qtest irq with the named GPIO-out pin of the device + * whose path is specified by @string and whose name is @name. + */ +void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name); + /** * qtest_set_irq_in: * @s: QTestState instance to operate on. diff --git a/softmmu/qtest.c b/softmmu/qtest.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) || strcmp(words[0], "irq_intercept_in") == 0) { DeviceState *dev; NamedGPIOList *ngl; + bool is_outbound; g_assert(words[1]); + is_outbound = words[0][14] == 'o'; dev = DEVICE(object_resolve_path(words[1], NULL)); if (!dev) { qtest_send_prefix(chr); @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) } QLIST_FOREACH(ngl, &dev->gpios, node) { - /* We don't support intercept of named GPIOs yet */ - if (ngl->name) { - continue; - } - if (words[0][14] == 'o') { - int i; - for (i = 0; i < ngl->num_out; ++i) { - qtest_install_gpio_out_intercept(dev, ngl->name, i); + /* We don't support inbound interception of named GPIOs yet */ + if (is_outbound) { + /* NULL is valid and matchable, for "unnamed GPIO" */ + if (g_strcmp0(ngl->name, words[2]) == 0) { + int i; + for (i = 0; i < ngl->num_out; ++i) { + qtest_install_gpio_out_intercept(dev, ngl->name, i); + } } } else { qemu_irq_intercept_in(ngl->in, qtest_irq_handler, diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -XXX,XX +XXX,XX @@ void qtest_irq_intercept_out(QTestState *s, const char *qom_path) qtest_rsp(s); } +void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name) +{ + qtest_sendf(s, "irq_intercept_out %s %s\n", qom_path, name); + qtest_rsp(s); +} + void qtest_irq_intercept_in(QTestState *s, const char *qom_path) { qtest_sendf(s, "irq_intercept_in %s\n", qom_path); -- 2.34.1 From: Chris Laplante <chris@laplante.io> Named interception of in-GPIOs is not supported yet. Signed-off-by: Chris Laplante <chris@laplante.io> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230728160324.1159090-5-chris@laplante.io Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- softmmu/qtest.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/softmmu/qtest.c b/softmmu/qtest.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) || strcmp(words[0], "irq_intercept_in") == 0) { DeviceState *dev; NamedGPIOList *ngl; + bool is_named; bool is_outbound; g_assert(words[1]); + is_named = words[2] != NULL; is_outbound = words[0][14] == 'o'; dev = DEVICE(object_resolve_path(words[1], NULL)); if (!dev) { @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) return; } + if (is_named && !is_outbound) { + qtest_send_prefix(chr); + qtest_send(chr, "FAIL Interception of named in-GPIOs not yet supported\n"); + return; + } + if (irq_intercept_dev) { qtest_send_prefix(chr); if (irq_intercept_dev != dev) { -- 2.34.1 From: Chris Laplante <chris@laplante.io> This is much better than just silently failing with OK. Signed-off-by: Chris Laplante <chris@laplante.io> Message-id: 20230728160324.1159090-6-chris@laplante.io Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- softmmu/qtest.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/softmmu/qtest.c b/softmmu/qtest.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) NamedGPIOList *ngl; bool is_named; bool is_outbound; + bool interception_succeeded = false; g_assert(words[1]); is_named = words[2] != NULL; @@ -XXX,XX +XXX,XX @@ static void qtest_process_command(CharBackend *chr, gchar **words) for (i = 0; i < ngl->num_out; ++i) { qtest_install_gpio_out_intercept(dev, ngl->name, i); } + interception_succeeded = true; } } else { qemu_irq_intercept_in(ngl->in, qtest_irq_handler, ngl->num_in); + interception_succeeded = true; } } - irq_intercept_dev = dev; + qtest_send_prefix(chr); - qtest_send(chr, "OK\n"); + if (interception_succeeded) { + irq_intercept_dev = dev; + qtest_send(chr, "OK\n"); + } else { + qtest_send(chr, "FAIL No intercepts installed\n"); + } } else if (strcmp(words[0], "set_irq_in") == 0) { DeviceState *dev; qemu_irq irq; -- 2.34.1 From: Chris Laplante <chris@laplante.io> Exercise the DETECT mechanism of the GPIO peripheral. Signed-off-by: Chris Laplante <chris@laplante.io> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230728160324.1159090-7-chris@laplante.io [PMM: fixed coding style nits] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- tests/qtest/microbit-test.c | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/microbit-test.c +++ b/tests/qtest/microbit-test.c @@ -XXX,XX +XXX,XX @@ static void test_nrf51_gpio(void) qtest_quit(qts); } +static void test_nrf51_gpio_detect(void) +{ + QTestState *qts = qtest_init("-M microbit"); + int i; + + /* Connect input buffer on pins 1-7, configure SENSE for high level */ + for (i = 1; i <= 7; i++) { + qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4, + deposit32(0, 16, 2, 2)); + } + + qtest_irq_intercept_out_named(qts, "/machine/nrf51/gpio", "detect"); + + for (i = 1; i <= 7; i++) { + /* Set pin high */ + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1); + uint32_t actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN); + g_assert_cmpuint(actual, ==, 1 << i); + + /* Check that DETECT is high */ + g_assert_true(qtest_get_irq(qts, 0)); + + /* Set pin low, check that DETECT goes low. */ + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 0); + actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN); + g_assert_cmpuint(actual, ==, 0x0); + g_assert_false(qtest_get_irq(qts, 0)); + } + + /* Set pin 0 high, check that DETECT doesn't fire */ + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1); + g_assert_false(qtest_get_irq(qts, 0)); + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0); + + /* Set pins 1, 2, and 3 high, then set 3 low. Check DETECT is still high */ + for (i = 1; i <= 3; i++) { + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1); + } + g_assert_true(qtest_get_irq(qts, 0)); + qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 3, 0); + g_assert_true(qtest_get_irq(qts, 0)); +} + static void timer_task(QTestState *qts, hwaddr task) { qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart); qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio); + qtest_add_func("/microbit/nrf51/gpio_detect", test_nrf51_gpio_detect); qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c); -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> kvm_arch_get_default_type() returns the default KVM type. This hook is particularly useful to derive a KVM type that is valid for "none" machine model, which is used by libvirt to probe the availability of KVM. For MIPS, the existing mips_kvm_type() is reused. This function ensures the availability of VZ which is mandatory to use KVM on the current QEMU. Cc: qemu-stable@nongnu.org Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-2-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> [PMM: added doc comment for new function] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/sysemu/kvm.h | 2 ++ target/mips/kvm_mips.h | 9 --------- accel/kvm/kvm-all.c | 4 +++- hw/mips/loongson3_virt.c | 2 -- target/arm/kvm.c | 5 +++++ target/i386/kvm/kvm.c | 5 +++++ target/mips/kvm.c | 2 +- target/ppc/kvm.c | 5 +++++ target/riscv/kvm.c | 5 +++++ target/s390x/kvm/kvm.c | 5 +++++ 10 files changed, 31 insertions(+), 13 deletions(-) diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -XXX,XX +XXX,XX @@ int kvm_arch_get_registers(CPUState *cpu); int kvm_arch_put_registers(CPUState *cpu, int level); +int kvm_arch_get_default_type(MachineState *ms); + int kvm_arch_init(MachineState *ms, KVMState *s); int kvm_arch_init_vcpu(CPUState *cpu); diff --git a/target/mips/kvm_mips.h b/target/mips/kvm_mips.h index XXXXXXX..XXXXXXX 100644 --- a/target/mips/kvm_mips.h +++ b/target/mips/kvm_mips.h @@ -XXX,XX +XXX,XX @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu); int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level); int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level); -#ifdef CONFIG_KVM -int mips_kvm_type(MachineState *machine, const char *vm_type); -#else -static inline int mips_kvm_type(MachineState *machine, const char *vm_type) -{ - return 0; -} -#endif - #endif /* KVM_MIPS_H */ diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index XXXXXXX..XXXXXXX 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -XXX,XX +XXX,XX @@ static int kvm_init(MachineState *ms) KVMState *s; const KVMCapabilityInfo *missing_cap; int ret; - int type = 0; + int type; uint64_t dirty_log_manual_caps; qemu_mutex_init(&kml_slots_lock); @@ -XXX,XX +XXX,XX @@ static int kvm_init(MachineState *ms) type = mc->kvm_type(ms, kvm_type); } else if (mc->kvm_type) { type = mc->kvm_type(ms, NULL); + } else { + type = kvm_arch_get_default_type(ms); } do { diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c index XXXXXXX..XXXXXXX 100644 --- a/hw/mips/loongson3_virt.c +++ b/hw/mips/loongson3_virt.c @@ -XXX,XX +XXX,XX @@ #include "qemu/datadir.h" #include "qapi/error.h" #include "elf.h" -#include "kvm_mips.h" #include "hw/char/serial.h" #include "hw/intc/loongson_liointc.h" #include "hw/mips/mips.h" @@ -XXX,XX +XXX,XX @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data) mc->max_cpus = LOONGSON_MAX_VCPUS; mc->default_ram_id = "loongson3.highram"; mc->default_ram_size = 1600 * MiB; - mc->kvm_type = mips_kvm_type; mc->minimum_page_bits = 14; mc->default_nic = "virtio-net-pci"; } diff --git a/target/arm/kvm.c b/target/arm/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -XXX,XX +XXX,XX @@ int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa) return ret > 0 ? ret : 40; } +int kvm_arch_get_default_type(MachineState *ms) +{ + return 0; +} + int kvm_arch_init(MachineState *ms, KVMState *s) { int ret = 0; diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -XXX,XX +XXX,XX @@ static void register_smram_listener(Notifier *n, void *unused) &smram_address_space, 1, "kvm-smram"); } +int kvm_arch_get_default_type(MachineState *ms) +{ + return 0; +} + int kvm_arch_init(MachineState *ms, KVMState *s) { uint64_t identity_base = 0xfffbc000; diff --git a/target/mips/kvm.c b/target/mips/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/mips/kvm.c +++ b/target/mips/kvm.c @@ -XXX,XX +XXX,XX @@ int kvm_arch_msi_data_to_gsi(uint32_t data) abort(); } -int mips_kvm_type(MachineState *machine, const char *vm_type) +int kvm_arch_get_default_type(MachineState *machine) { #if defined(KVM_CAP_MIPS_VZ) int r; diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -XXX,XX +XXX,XX @@ static int kvm_ppc_register_host_cpu_type(void); static void kvmppc_get_cpu_characteristics(KVMState *s); static int kvmppc_get_dec_bits(void); +int kvm_arch_get_default_type(MachineState *ms) +{ + return 0; +} + int kvm_arch_init(MachineState *ms, KVMState *s) { cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ); diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/riscv/kvm.c +++ b/target/riscv/kvm.c @@ -XXX,XX +XXX,XX @@ int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route, return 0; } +int kvm_arch_get_default_type(MachineState *ms) +{ + return 0; +} + int kvm_arch_init(MachineState *ms, KVMState *s) { return 0; diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -XXX,XX +XXX,XX @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) mc->default_cpu_type = S390_CPU_TYPE_NAME("host"); } +int kvm_arch_get_default_type(MachineState *ms) +{ + return 0; +} + int kvm_arch_init(MachineState *ms, KVMState *s) { object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> Before this change, the default KVM type, which is used for non-virt machine models, was 0. The kernel documentation says: > On arm64, the physical address size for a VM (IPA Size limit) is > limited to 40bits by default. The limit can be configured if the host > supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use > KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type > identifier, where IPA_Bits is the maximum width of any physical > address used by the VM. The IPA_Bits is encoded in bits[7-0] of the > machine type identifier. > > e.g, to configure a guest to use 48bit physical address size:: > > vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48)); > > The requested size (IPA_Bits) must be: > > == ========================================================= > 0 Implies default size, 40bits (for backward compatibility) > N Implies N bits, where N is a positive integer such that, > 32 <= N <= Host_IPA_Limit > == ========================================================= > Host_IPA_Limit is the maximum possible value for IPA_Bits on the host > and is dependent on the CPU capability and the kernel configuration. > The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the > KVM_CHECK_EXTENSION ioctl() at run-time. > > Creation of the VM will fail if the requested IPA size (whether it is > implicit or explicit) is unsupported on the host. https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm So if Host_IPA_Limit < 40, specifying 0 as the type will fail. This actually confused libvirt, which uses "none" machine model to probe the KVM availability, on M2 MacBook Air. Fix this by using Host_IPA_Limit as the default type when KVM_CAP_ARM_VM_IPA_SIZE is available. Cc: qemu-stable@nongnu.org Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-3-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/kvm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/target/arm/kvm.c b/target/arm/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -XXX,XX +XXX,XX @@ int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa) int kvm_arch_get_default_type(MachineState *ms) { - return 0; + bool fixed_ipa; + int size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa); + return fixed_ipa ? 0 : size; } int kvm_arch_init(MachineState *ms, KVMState *s) -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> On MIPS, QEMU requires KVM_VM_MIPS_VZ type for KVM. Report an error in such a case as other architectures do when an error occurred during KVM type decision. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-4-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- target/mips/kvm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/mips/kvm.c b/target/mips/kvm.c index XXXXXXX..XXXXXXX 100644 --- a/target/mips/kvm.c +++ b/target/mips/kvm.c @@ -XXX,XX +XXX,XX @@ int kvm_arch_get_default_type(MachineState *machine) } #endif + error_report("KVM_VM_MIPS_VZ type is not available"); return -1; } -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> On MIPS, kvm_arch_get_default_type() returns a negative value when an error occurred so handle the case. Also, let other machines return negative values when errors occur and declare returning a negative value as the correct way to propagate an error that happened when determining KVM type. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-5-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- accel/kvm/kvm-all.c | 5 +++++ hw/arm/virt.c | 2 +- hw/ppc/spapr.c | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index XXXXXXX..XXXXXXX 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -XXX,XX +XXX,XX @@ static int kvm_init(MachineState *ms) type = kvm_arch_get_default_type(ms); } + if (type < 0) { + ret = -EINVAL; + goto err; + } + do { ret = kvm_ioctl(s, KVM_CREATE_VM, type); } while (ret == -EINTR); diff --git a/hw/arm/virt.c b/hw/arm/virt.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -XXX,XX +XXX,XX @@ static int virt_kvm_type(MachineState *ms, const char *type_str) "require an IPA range (%d bits) larger than " "the one supported by the host (%d bits)", requested_pa_size, max_vm_pa_size); - exit(1); + return -1; } /* * We return the requested PA log size, unless KVM only supports diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index XXXXXXX..XXXXXXX 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -XXX,XX +XXX,XX @@ static int spapr_kvm_type(MachineState *machine, const char *vm_type) } error_report("Unknown kvm-type specified '%s'", vm_type); - exit(1); + return -1; } /* -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> An error may occur after s->as is allocated, for example if the KVM_CREATE_VM ioctl call fails. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-6-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> [PMM: tweaked commit message] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- accel/kvm/kvm-all.c | 1 + 1 file changed, 1 insertion(+) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index XXXXXXX..XXXXXXX 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -XXX,XX +XXX,XX @@ err: if (s->fd != -1) { close(s->fd); } + g_free(s->as); g_free(s->memory_listener.slots); return ret; -- 2.34.1 From: Akihiko Odaki <akihiko.odaki@daynix.com> The returned value was always zero and had no meaning. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Message-id: 20230727073134.134102-7-akihiko.odaki@daynix.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- accel/kvm/kvm-all.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index XXXXXXX..XXXXXXX 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -XXX,XX +XXX,XX @@ static void *kvm_dirty_ring_reaper_thread(void *data) return NULL; } -static int kvm_dirty_ring_reaper_init(KVMState *s) +static void kvm_dirty_ring_reaper_init(KVMState *s) { struct KVMDirtyRingReaper *r = &s->reaper; qemu_thread_create(&r->reaper_thr, "kvm-reaper", kvm_dirty_ring_reaper_thread, s, QEMU_THREAD_JOINABLE); - - return 0; } static int kvm_dirty_ring_init(KVMState *s) @@ -XXX,XX +XXX,XX @@ static int kvm_init(MachineState *ms) } if (s->kvm_dirty_ring_size) { - ret = kvm_dirty_ring_reaper_init(s); - if (ret) { - goto err; - } + kvm_dirty_ring_reaper_init(s); } if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) { -- 2.34.1 For an Unsupported Atomic Update fault where the stage 1 translation table descriptor update can't be done because it's to an unsupported memory type, this is a stage 1 abort (per the Arm ARM R_VSXXT). This means we should not set fi->s1ptw, because this will cause the code in the get_phys_addr_lpae() error-exit path to mark it as stage 2. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-2-peter.maydell@linaro.org --- target/arm/ptw.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val, if (unlikely(!host)) { fi->type = ARMFault_UnsuppAtomicUpdate; - fi->s1ptw = true; return 0; } -- 2.34.1 In S1_ptw_translate() we set up the ARMMMUFaultInfo if the attempt to translate the page descriptor address into a physical address fails. This used to only be possible if we are doing a stage 2 ptw for that descriptor address, and so the code always sets fi->stage2 and fi->s1ptw to true. However, with FEAT_RME it is also possible for the lookup of the page descriptor address to fail because of a Granule Protection Check fault. These should not be reported as stage 2, otherwise arm_deliver_fault() will incorrectly set HPFAR_EL2. Similarly the s1ptw bit should only be set for stage 2 faults on stage 1 translation table walks, i.e. not for GPC faults. Add a comment to the the other place where we might detect a stage2-fault-on-stage-1-ptw, in arm_casq_ptw(), noting why we know in that case that it must really be a stage 2 fault and not a GPC fault. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-3-peter.maydell@linaro.org --- target/arm/ptw.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, fi->type = ARMFault_GPCFOnWalk; } fi->s2addr = addr; - fi->stage2 = true; - fi->s1ptw = true; + fi->stage2 = regime_is_stage2(s2_mmu_idx); + fi->s1ptw = fi->stage2; fi->s1ns = !is_secure; return false; } @@ -XXX,XX +XXX,XX @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val, env->tlb_fi = NULL; if (unlikely(flags & TLB_INVALID_MASK)) { + /* + * We know this must be a stage 2 fault because the granule + * protection table does not separately track read and write + * permission, so all GPC faults are caught in S1_ptw_translate(): + * we only get here for "readable but not writeable". + */ assert(fi->type != ARMFault_None); fi->s2addr = ptw->out_virt; fi->stage2 = true; -- 2.34.1 The s1ns bit in ARMMMUFaultInfo is documented as "true if we faulted on a non-secure IPA while in secure state". Both the places which look at this bit only do so after having confirmed that this is a stage 2 fault and we're dealing with Secure EL2, which leaves the ptw.c code free to set the bit to any random value in the other cases. Instead of taking advantage of that freedom, consistently make the bit be set to false for the "not a stage 2 fault for Secure EL2" cases. This removes some cases where we were using an 'is_secure' boolean and leaving the reader guessing about whether that was the right thing for Realm and Root cases. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-4-peter.maydell@linaro.org --- target/arm/ptw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static ARMSecuritySpace S2_security_space(ARMSecuritySpace s1_space, } } +static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx) +{ + /* + * For stage 2 faults in Secure EL22, S1NS indicates + * whether the faulting IPA is in the Secure or NonSecure + * IPA space. For all other kinds of fault, it is false. + */ + return space == ARMSS_Secure && regime_is_stage2(s2_mmu_idx) + && s2_mmu_idx == ARMMMUIdx_Stage2_S; +} + /* Translate a S1 pagetable walk through S2 if needed. */ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, hwaddr addr, ARMMMUFaultInfo *fi) @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, fi->s2addr = addr; fi->stage2 = true; fi->s1ptw = true; - fi->s1ns = !is_secure; + fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx); return false; } } @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, fi->s2addr = addr; fi->stage2 = regime_is_stage2(s2_mmu_idx); fi->s1ptw = fi->stage2; - fi->s1ns = !is_secure; + fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx); return false; } @@ -XXX,XX +XXX,XX @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val, fi->s2addr = ptw->out_virt; fi->stage2 = true; fi->s1ptw = true; - fi->s1ns = !ptw->in_secure; + fi->s1ns = fault_s1ns(ptw->in_space, ptw->in_ptw_idx); return 0; } @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, fi->level = level; /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2. */ fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx); - fi->s1ns = mmu_idx == ARMMMUIdx_Stage2; + fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx); return true; } -- 2.34.1 In commit 6d2654ffacea813916176 we created the S1Translate struct and used it to plumb through various arguments that we were previously passing one-at-a-time to get_phys_addr_v5(), get_phys_addr_v6(), and get_phys_addr_lpae(). Extend that pattern to get_phys_addr_pmsav5(), get_phys_addr_pmsav7(), get_phys_addr_pmsav8() and get_phys_addr_disabled(), so that all the get_phys_addr_* functions we call from get_phys_addr_nogpc() take the S1Translate struct rather than the mmu_idx and is_secure bool. (This refactoring is a prelude to having the called functions look at ptw->is_space rather than using an is_secure boolean.) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-5-peter.maydell@linaro.org --- target/arm/ptw.c | 57 ++++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, return true; } -static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool is_secure, GetPhysAddrResult *result, +static bool get_phys_addr_pmsav5(CPUARMState *env, + S1Translate *ptw, + uint32_t address, + MMUAccessType access_type, + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) { int n; uint32_t mask; uint32_t base; + ARMMMUIdx mmu_idx = ptw->in_mmu_idx; bool is_user = regime_is_user(env, mmu_idx); + bool is_secure = arm_space_is_secure(ptw->in_space); if (regime_translation_disabled(env, mmu_idx, is_secure)) { /* MPU disabled. */ @@ -XXX,XX +XXX,XX @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, return regime_sctlr(env, mmu_idx) & SCTLR_BR; } -static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool secure, GetPhysAddrResult *result, +static bool get_phys_addr_pmsav7(CPUARMState *env, + S1Translate *ptw, + uint32_t address, + MMUAccessType access_type, + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) { ARMCPU *cpu = env_archcpu(env); int n; + ARMMMUIdx mmu_idx = ptw->in_mmu_idx; bool is_user = regime_is_user(env, mmu_idx); + bool secure = arm_space_is_secure(ptw->in_space); result->f.phys_addr = address; result->f.lg_page_size = TARGET_PAGE_BITS; @@ -XXX,XX +XXX,XX @@ void v8m_security_lookup(CPUARMState *env, uint32_t address, } } -static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool secure, GetPhysAddrResult *result, +static bool get_phys_addr_pmsav8(CPUARMState *env, + S1Translate *ptw, + uint32_t address, + MMUAccessType access_type, + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) { V8M_SAttributes sattrs = {}; + ARMMMUIdx mmu_idx = ptw->in_mmu_idx; + bool secure = arm_space_is_secure(ptw->in_space); bool ret; if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { @@ -XXX,XX +XXX,XX @@ static ARMCacheAttrs combine_cacheattrs(uint64_t hcr, * MMU disabled. S1 addresses within aa64 translation regimes are * still checked for bounds -- see AArch64.S1DisabledOutput(). */ -static bool get_phys_addr_disabled(CPUARMState *env, target_ulong address, +static bool get_phys_addr_disabled(CPUARMState *env, + S1Translate *ptw, + target_ulong address, MMUAccessType access_type, - ARMMMUIdx mmu_idx, bool is_secure, GetPhysAddrResult *result, ARMMMUFaultInfo *fi) { + ARMMMUIdx mmu_idx = ptw->in_mmu_idx; + bool is_secure = arm_space_is_secure(ptw->in_space); uint8_t memattr = 0x00; /* Device nGnRnE */ uint8_t shareability = 0; /* non-shareable */ int r_el; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, case ARMMMUIdx_Phys_Root: case ARMMMUIdx_Phys_Realm: /* Checking Phys early avoids special casing later vs regime_el. */ - return get_phys_addr_disabled(env, address, access_type, mmu_idx, - is_secure, result, fi); + return get_phys_addr_disabled(env, ptw, address, access_type, + result, fi); case ARMMMUIdx_Stage1_E0: case ARMMMUIdx_Stage1_E1: @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, if (arm_feature(env, ARM_FEATURE_V8)) { /* PMSAv8 */ - ret = get_phys_addr_pmsav8(env, address, access_type, mmu_idx, - is_secure, result, fi); + ret = get_phys_addr_pmsav8(env, ptw, address, access_type, + result, fi); } else if (arm_feature(env, ARM_FEATURE_V7)) { /* PMSAv7 */ - ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx, - is_secure, result, fi); + ret = get_phys_addr_pmsav7(env, ptw, address, access_type, + result, fi); } else { /* Pre-v7 MPU */ - ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx, - is_secure, result, fi); + ret = get_phys_addr_pmsav5(env, ptw, address, access_type, + result, fi); } qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32 " mmu_idx %u -> %s (prot %c%c%c)\n", @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, /* Definitely a real MMU, not an MPU */ if (regime_translation_disabled(env, mmu_idx, is_secure)) { - return get_phys_addr_disabled(env, address, access_type, mmu_idx, - is_secure, result, fi); + return get_phys_addr_disabled(env, ptw, address, access_type, + result, fi); } if (regime_using_lpae_format(env, mmu_idx)) { -- 2.34.1 Plumb the ARMSecurityState through to regime_translation_disabled() rather than just a bool is_secure. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-6-peter.maydell@linaro.org --- target/arm/ptw.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx, int ttbrn) /* Return true if the specified stage of address translation is disabled */ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx, - bool is_secure) + ARMSecuritySpace space) { uint64_t hcr_el2; + bool is_secure = arm_space_is_secure(space); if (arm_feature(env, ARM_FEATURE_M)) { switch (env->v7m.mpu_ctrl[is_secure] & @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t base; ARMMMUIdx mmu_idx = ptw->in_mmu_idx; bool is_user = regime_is_user(env, mmu_idx); - bool is_secure = arm_space_is_secure(ptw->in_space); - if (regime_translation_disabled(env, mmu_idx, is_secure)) { + if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) { /* MPU disabled. */ result->f.phys_addr = address; result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_pmsav7(CPUARMState *env, result->f.lg_page_size = TARGET_PAGE_BITS; result->f.prot = 0; - if (regime_translation_disabled(env, mmu_idx, secure) || + if (regime_translation_disabled(env, mmu_idx, ptw->in_space) || m_is_ppb_region(env, address)) { /* * MPU disabled or M profile PPB access: use default memory map. @@ -XXX,XX +XXX,XX @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, * are done in arm_v7m_load_vector(), which always does a direct * read using address_space_ldl(), rather than going via this function. */ - if (regime_translation_disabled(env, mmu_idx, secure)) { /* MPU disabled */ + if (regime_translation_disabled(env, mmu_idx, arm_secure_to_space(secure))) { + /* MPU disabled */ hit = true; } else if (m_is_ppb_region(env, address)) { hit = true; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, */ ptw->in_mmu_idx = mmu_idx = s1_mmu_idx; if (arm_feature(env, ARM_FEATURE_EL2) && - !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) { + !regime_translation_disabled(env, ARMMMUIdx_Stage2, ptw->in_space)) { return get_phys_addr_twostage(env, ptw, address, access_type, result, fi); } @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, /* Definitely a real MMU, not an MPU */ - if (regime_translation_disabled(env, mmu_idx, is_secure)) { + if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) { return get_phys_addr_disabled(env, ptw, address, access_type, result, fi); } -- 2.34.1 arm_hcr_el2_eff_secstate() takes a bool secure, which it uses to determine whether EL2 is enabled in the current security state. With the advent of FEAT_RME this is no longer sufficient, because EL2 can be enabled for Secure state but not for Root, and both of those will pass 'secure == true' in the callsites in ptw.c. As it happens in all of our callsites in ptw.c we either avoid making the call or else avoid using the returned value if we're doing a translation for Root, so this is not a behaviour change even if the experimental FEAT_RME is enabled. But it is less confusing in the ptw.c code if we avoid the use of a bool secure that duplicates some of the information in the ArmSecuritySpace argument. Make arm_hcr_el2_eff_secstate() take an ARMSecuritySpace argument instead. Because we always want to know the HCR_EL2 for the security state defined by the current effective value of SCR_EL3.{NSE,NS}, it makes no sense to pass ARMSS_Root here, and we assert that callers don't do that. To avoid the assert(), we thus push the call to arm_hcr_el2_eff_secstate() down into the cases in regime_translation_disabled() that need it, rather than calling the function and ignoring the result for the Root space translations. All other calls to this function in ptw.c are already in places where we have confirmed that the mmu_idx is a stage 2 translation or that the regime EL is not 3. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-7-peter.maydell@linaro.org --- target/arm/cpu.h | 2 +- target/arm/helper.c | 8 +++++--- target/arm/ptw.c | 15 +++++++-------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ static inline bool arm_is_el2_enabled(CPUARMState *env) * "for all purposes other than a direct read or write access of HCR_EL2." * Not included here is HCR_RW. */ -uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure); +uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space); uint64_t arm_hcr_el2_eff(CPUARMState *env); uint64_t arm_hcrx_el2_eff(CPUARMState *env); diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, * Bits that are not included here: * RW (read from SCR_EL3.RW as needed) */ -uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure) +uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space) { uint64_t ret = env->cp15.hcr_el2; - if (!arm_is_el2_enabled_secstate(env, secure)) { + assert(space != ARMSS_Root); + + if (!arm_is_el2_enabled_secstate(env, arm_space_is_secure(space))) { /* * "This register has no effect if EL2 is not enabled in the * current Security state". This is ARMv8.4-SecEL2 speak for @@ -XXX,XX +XXX,XX @@ uint64_t arm_hcr_el2_eff(CPUARMState *env) if (arm_feature(env, ARM_FEATURE_M)) { return 0; } - return arm_hcr_el2_eff_secstate(env, arm_is_secure_below_el3(env)); + return arm_hcr_el2_eff_secstate(env, arm_security_space_below_el3(env)); } /* diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx, ARMSecuritySpace space) { uint64_t hcr_el2; - bool is_secure = arm_space_is_secure(space); if (arm_feature(env, ARM_FEATURE_M)) { + bool is_secure = arm_space_is_secure(space); switch (env->v7m.mpu_ctrl[is_secure] & (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) { case R_V7M_MPU_CTRL_ENABLE_MASK: @@ -XXX,XX +XXX,XX @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx, } } - hcr_el2 = arm_hcr_el2_eff_secstate(env, is_secure); switch (mmu_idx) { case ARMMMUIdx_Stage2: case ARMMMUIdx_Stage2_S: /* HCR.DC means HCR.VM behaves as 1 */ + hcr_el2 = arm_hcr_el2_eff_secstate(env, space); return (hcr_el2 & (HCR_DC | HCR_VM)) == 0; case ARMMMUIdx_E10_0: case ARMMMUIdx_E10_1: case ARMMMUIdx_E10_1_PAN: /* TGE means that EL0/1 act as if SCTLR_EL1.M is zero */ + hcr_el2 = arm_hcr_el2_eff_secstate(env, space); if (hcr_el2 & HCR_TGE) { return true; } @@ -XXX,XX +XXX,XX @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx, case ARMMMUIdx_Stage1_E1: case ARMMMUIdx_Stage1_E1_PAN: /* HCR.DC means SCTLR_EL1.M behaves as 0 */ + hcr_el2 = arm_hcr_el2_eff_secstate(env, space); if (hcr_el2 & HCR_DC) { return true; } @@ -XXX,XX +XXX,XX @@ static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx) static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, hwaddr addr, ARMMMUFaultInfo *fi) { - bool is_secure = ptw->in_secure; ARMMMUIdx mmu_idx = ptw->in_mmu_idx; ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx; uint8_t pte_attrs; @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, } if (regime_is_stage2(s2_mmu_idx)) { - uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure); + uint64_t hcr = arm_hcr_el2_eff_secstate(env, ptw->in_space); if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) { /* @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_disabled(CPUARMState *env, ARMMMUFaultInfo *fi) { ARMMMUIdx mmu_idx = ptw->in_mmu_idx; - bool is_secure = arm_space_is_secure(ptw->in_space); uint8_t memattr = 0x00; /* Device nGnRnE */ uint8_t shareability = 0; /* non-shareable */ int r_el; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_disabled(CPUARMState *env, /* Fill in cacheattr a-la AArch64.TranslateAddressS1Off. */ if (r_el == 1) { - uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure); + uint64_t hcr = arm_hcr_el2_eff_secstate(env, ptw->in_space); if (hcr & HCR_DC) { if (hcr & HCR_DCT) { memattr = 0xf0; /* Tagged, Normal, WB, RWA */ @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, { hwaddr ipa; int s1_prot, s1_lgpgsz; - bool is_secure = ptw->in_secure; ARMSecuritySpace in_space = ptw->in_space; bool ret, ipa_secure; ARMCacheAttrs cacheattrs1; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, } /* Combine the S1 and S2 cache attributes. */ - hcr = arm_hcr_el2_eff_secstate(env, is_secure); + hcr = arm_hcr_el2_eff_secstate(env, in_space); if (hcr & HCR_DC) { /* * HCR.DC forces the first stage attributes to -- 2.34.1 Pass an ARMSecuritySpace instead of a bool secure to arm_is_el2_enabled_secstate(). This doesn't change behaviour. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-8-peter.maydell@linaro.org --- target/arm/cpu.h | 13 ++++++++----- target/arm/helper.c | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ static inline bool arm_is_secure(CPUARMState *env) /* * Return true if the current security state has AArch64 EL2 or AArch32 Hyp. - * This corresponds to the pseudocode EL2Enabled() + * This corresponds to the pseudocode EL2Enabled(). */ -static inline bool arm_is_el2_enabled_secstate(CPUARMState *env, bool secure) +static inline bool arm_is_el2_enabled_secstate(CPUARMState *env, + ARMSecuritySpace space) { + assert(space != ARMSS_Root); return arm_feature(env, ARM_FEATURE_EL2) - && (!secure || (env->cp15.scr_el3 & SCR_EEL2)); + && (space != ARMSS_Secure || (env->cp15.scr_el3 & SCR_EEL2)); } static inline bool arm_is_el2_enabled(CPUARMState *env) { - return arm_is_el2_enabled_secstate(env, arm_is_secure_below_el3(env)); + return arm_is_el2_enabled_secstate(env, arm_security_space_below_el3(env)); } #else @@ -XXX,XX +XXX,XX @@ static inline bool arm_is_secure(CPUARMState *env) return false; } -static inline bool arm_is_el2_enabled_secstate(CPUARMState *env, bool secure) +static inline bool arm_is_el2_enabled_secstate(CPUARMState *env, + ARMSecuritySpace space) { return false; } diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space) assert(space != ARMSS_Root); - if (!arm_is_el2_enabled_secstate(env, arm_space_is_secure(space))) { + if (!arm_is_el2_enabled_secstate(env, space)) { /* * "This register has no effect if EL2 is not enabled in the * current Security state". This is ARMv8.4-SecEL2 speak for -- 2.34.1 When we do a translation in Secure state, the NSTable bits in table descriptors may downgrade us to NonSecure; we update ptw->in_secure and ptw->in_space accordingly. We guard that check correctly with a conditional that means it's only applied for Secure stage 1 translations. However, later on in get_phys_addr_lpae() we fold the effects of the NSTable bits into the final descriptor attributes bits, and there we do it unconditionally regardless of the CPU state. That means that in Realm state (where in_secure is false) we will set bit 5 in attrs, and later use it to decide to output to non-secure space. We don't in fact need to do this folding in at all any more (since commit 2f1ff4e7b9f30c): if an NSTable bit was set then we have already set ptw->in_space to ARMSS_NonSecure, and in that situation we don't look at attrs bit 5. The only thing we still need to deal with is the real NS bit in the final descriptor word, so we can just drop the code that ORed in the NSTable bit. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-9-peter.maydell@linaro.org --- target/arm/ptw.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, * Extract attributes from the (modified) descriptor, and apply * table descriptors. Stage 2 table descriptors do not include * any attribute fields. HPD disables all the table attributes - * except NSTable. + * except NSTable (which we have already handled). */ attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14)); if (!regime_is_stage2(mmu_idx)) { - attrs |= !ptw->in_secure << 5; /* NS */ if (!param.hpd) { attrs |= extract64(tableattrs, 0, 2) << 53; /* XN, PXN */ /* -- 2.34.1 Replace the last uses of ptw->in_secure with appropriate checks on ptw->in_space. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-10-peter.maydell@linaro.org --- target/arm/ptw.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, ARMMMUFaultInfo *fi) { ARMMMUIdx mmu_idx = ptw->in_mmu_idx; - bool is_secure = ptw->in_secure; ARMMMUIdx s1_mmu_idx; /* @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, * cannot upgrade a NonSecure translation regime's attributes * to Secure or Realm. */ - result->f.attrs.secure = is_secure; result->f.attrs.space = ptw->in_space; + result->f.attrs.secure = arm_space_is_secure(ptw->in_space); switch (mmu_idx) { case ARMMMUIdx_Phys_S: @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, case ARMMMUIdx_Stage1_E0: case ARMMMUIdx_Stage1_E1: case ARMMMUIdx_Stage1_E1_PAN: - /* First stage lookup uses second stage for ptw. */ - ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; + /* + * First stage lookup uses second stage for ptw; only + * Secure has both S and NS IPA and starts with Stage2_S. + */ + ptw->in_ptw_idx = (ptw->in_space == ARMSS_Secure) ? + ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; break; case ARMMMUIdx_Stage2: -- 2.34.1 We no longer look at the in_secure field of the S1Translate struct anyway, so we can remove it and all the code which sets it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-11-peter.maydell@linaro.org --- target/arm/ptw.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ typedef struct S1Translate { * value being Stage2 vs Stage2_S distinguishes those. */ ARMSecuritySpace in_space; - /* - * in_secure: whether the translation regime is a Secure one. - * This is always equal to arm_space_is_secure(in_space). - * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit, - * this field is updated accordingly. - */ - bool in_secure; /* * in_debug: is this a QEMU debug access (gdbstub, etc)? Debug * accesses will not update the guest page table access flags @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, S1Translate s2ptw = { .in_mmu_idx = s2_mmu_idx, .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx), - .in_secure = arm_space_is_secure(s2_space), .in_space = s2_space, .in_debug = true, }; @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS); QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2); ptw->in_ptw_idx += 1; - ptw->in_secure = false; ptw->in_space = ARMSS_NonSecure; } @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, ptw->in_s1_is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0; ptw->in_mmu_idx = ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; - ptw->in_secure = ipa_secure; ptw->in_space = ipa_space; ptw->in_ptw_idx = ptw_idx_for_stage_2(env, ptw->in_mmu_idx); @@ -XXX,XX +XXX,XX @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, { S1Translate ptw = { .in_mmu_idx = mmu_idx, - .in_secure = is_secure, .in_space = arm_secure_to_space(is_secure), }; return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi); @@ -XXX,XX +XXX,XX @@ bool get_phys_addr(CPUARMState *env, target_ulong address, } ptw.in_space = ss; - ptw.in_secure = arm_space_is_secure(ss); return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi); } @@ -XXX,XX +XXX,XX @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr, S1Translate ptw = { .in_mmu_idx = mmu_idx, .in_space = ss, - .in_secure = arm_space_is_secure(ss), .in_debug = true, }; GetPhysAddrResult res = {}; -- 2.34.1 We only use S1Translate::out_secure in two places, where we are setting up MemTxAttrs for a page table load. We can use arm_space_is_secure(ptw->out_space) instead, which guarantees that we're setting the MemTxAttrs secure and space fields consistently, and allows us to drop the out_secure field in S1Translate entirely. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-12-peter.maydell@linaro.org --- target/arm/ptw.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ typedef struct S1Translate { * Stage 2 is indicated by in_mmu_idx set to ARMMMUIdx_Stage2{,_S}. */ bool in_s1_is_el0; - bool out_secure; bool out_rw; bool out_be; ARMSecuritySpace out_space; @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, pte_attrs = s2.cacheattrs.attrs; ptw->out_host = NULL; ptw->out_rw = false; - ptw->out_secure = s2.f.attrs.secure; ptw->out_space = s2.f.attrs.space; } else { #ifdef CONFIG_TCG @@ -XXX,XX +XXX,XX @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, ptw->out_phys = full->phys_addr | (addr & ~TARGET_PAGE_MASK); ptw->out_rw = full->prot & PAGE_WRITE; pte_attrs = full->pte_attrs; - ptw->out_secure = full->attrs.secure; ptw->out_space = full->attrs.space; #else g_assert_not_reached(); @@ -XXX,XX +XXX,XX @@ static uint32_t arm_ldl_ptw(CPUARMState *env, S1Translate *ptw, } else { /* Page tables are in MMIO. */ MemTxAttrs attrs = { - .secure = ptw->out_secure, .space = ptw->out_space, + .secure = arm_space_is_secure(ptw->out_space), }; AddressSpace *as = arm_addressspace(cs, attrs); MemTxResult result = MEMTX_OK; @@ -XXX,XX +XXX,XX @@ static uint64_t arm_ldq_ptw(CPUARMState *env, S1Translate *ptw, } else { /* Page tables are in MMIO. */ MemTxAttrs attrs = { - .secure = ptw->out_secure, .space = ptw->out_space, + .secure = arm_space_is_secure(ptw->out_space), }; AddressSpace *as = arm_addressspace(cs, attrs); MemTxResult result = MEMTX_OK; -- 2.34.1 When the MMU is disabled, data accesses should be Device nGnRnE, Outer Shareable, Untagged. We handle the other cases from AArch64.S1DisabledOutput() correctly but missed this one. Device nGnRnE is memattr == 0, so the only part we were missing was that shareability should be set to 2 for both insn fetches and data accesses. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-13-peter.maydell@linaro.org --- target/arm/ptw.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_disabled(CPUARMState *env, } } } - if (memattr == 0 && access_type == MMU_INST_FETCH) { - if (regime_sctlr(env, mmu_idx) & SCTLR_I) { - memattr = 0xee; /* Normal, WT, RA, NT */ - } else { - memattr = 0x44; /* Normal, NC, No */ + if (memattr == 0) { + if (access_type == MMU_INST_FETCH) { + if (regime_sctlr(env, mmu_idx) & SCTLR_I) { + memattr = 0xee; /* Normal, WT, RA, NT */ + } else { + memattr = 0x44; /* Normal, NC, No */ + } } shareability = 2; /* outer shareable */ } -- 2.34.1 The architecture doesn't permit block descriptors at any arbitrary level of the page table walk; it depends on the granule size which levels are permitted. We implemented only a partial version of this check which assumes that block descriptors are valid at all levels except level 3, which meant that we wouldn't deliver the Translation fault for all cases of this sort of guest page table error. Implement the logic corresponding to the pseudocode AArch64.DecodeDescriptorType() and AArch64.BlockDescSupported(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-14-peter.maydell@linaro.org --- target/arm/ptw.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static int check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint64_t tcr, return INT_MIN; } +static bool lpae_block_desc_valid(ARMCPU *cpu, bool ds, + ARMGranuleSize gran, int level) +{ + /* + * See pseudocode AArch46.BlockDescSupported(): block descriptors + * are not valid at all levels, depending on the page size. + */ + switch (gran) { + case Gran4K: + return (level == 0 && ds) || level == 1 || level == 2; + case Gran16K: + return (level == 1 && ds) || level == 2; + case Gran64K: + return (level == 1 && arm_pamax(cpu) == 52) || level == 2; + default: + g_assert_not_reached(); + } +} + /** * get_phys_addr_lpae: perform one stage of page table walk, LPAE format * @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, new_descriptor = descriptor; restart_atomic_update: - if (!(descriptor & 1) || (!(descriptor & 2) && (level == 3))) { - /* Invalid, or the Reserved level 3 encoding */ + if (!(descriptor & 1) || + (!(descriptor & 2) && + !lpae_block_desc_valid(cpu, param.ds, param.gran, level))) { + /* Invalid, or a block descriptor at an invalid level */ goto do_translation_fault; } -- 2.34.1 When we report faults due to stage 2 faults during a stage 1 page table walk, the 'level' parameter should be the level of the walk in stage 2 that faulted, not the level of the walk in stage 1. Correct the reporting of these faults. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-15-peter.maydell@linaro.org --- target/arm/ptw.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, do_translation_fault: fi->type = ARMFault_Translation; do_fault: - fi->level = level; - /* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2. */ - fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx); + if (fi->s1ptw) { + /* Retain the existing stage 2 fi->level */ + assert(fi->stage2); + } else { + fi->level = level; + fi->stage2 = regime_is_stage2(mmu_idx); + } fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx); return true; } -- 2.34.1 The PAR_EL1.SH field documents that for the cases of: * Device memory * Normal memory with both Inner and Outer Non-Cacheable the field should be 0b10 rather than whatever was in the translation table descriptor field. (In the pseudocode this is handled by PAREncodeShareability().) Perform this adjustment when assembling a PAR value. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20230807141514.19075-16-peter.maydell@linaro.org --- target/arm/helper.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri, } #ifdef CONFIG_TCG +static int par_el1_shareability(GetPhysAddrResult *res) +{ + /* + * The PAR_EL1.SH field must be 0b10 for Device or Normal-NC + * memory -- see pseudocode PAREncodeShareability(). + */ + if (((res->cacheattrs.attrs & 0xf0) == 0) || + res->cacheattrs.attrs == 0x44 || res->cacheattrs.attrs == 0x40) { + return 2; + } + return res->cacheattrs.shareability; +} + static uint64_t do_ats_write(CPUARMState *env, uint64_t value, MMUAccessType access_type, ARMMMUIdx mmu_idx, bool is_secure) @@ -XXX,XX +XXX,XX @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, par64 |= (1 << 9); /* NS */ } par64 |= (uint64_t)res.cacheattrs.attrs << 56; /* ATTR */ - par64 |= res.cacheattrs.shareability << 7; /* SH */ + par64 |= par_el1_shareability(&res) << 7; /* SH */ } else { uint32_t fsr = arm_fi_to_lfsc(&fi); -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> In realm state, stage-2 translation tables are fetched from the realm physical address space (R_PGRQD). Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230809123706.1842548-2-jean-philippe@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/ptw.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx) /* * We're OK to check the current state of the CPU here because - * (1) we always invalidate all TLBs when the SCR_EL3.NS bit changes + * (1) we always invalidate all TLBs when the SCR_EL3.NS or SCR_EL3.NSE bit + * changes. * (2) there's no way to do a lookup that cares about Stage 2 for a * different security state to the current one for AArch64, and AArch32 * never has a secure EL2. (AArch32 ATS12NSO[UP][RW] allow EL3 to do * an NS stage 1+2 lookup while the NS bit is 0.) */ - if (!arm_is_secure_below_el3(env) || !arm_el_is_aa64(env, 3)) { + if (!arm_el_is_aa64(env, 3)) { return ARMMMUIdx_Phys_NS; } - if (stage2idx == ARMMMUIdx_Stage2_S) { - s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW); - } else { - s2walk_secure = !(env->cp15.vtcr_el2 & VTCR_NSW); - } - return s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS; + switch (arm_security_space_below_el3(env)) { + case ARMSS_NonSecure: + return ARMMMUIdx_Phys_NS; + case ARMSS_Realm: + return ARMMMUIdx_Phys_Realm; + case ARMSS_Secure: + if (stage2idx == ARMMMUIdx_Stage2_S) { + s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW); + } else { + s2walk_secure = !(env->cp15.vtcr_el2 & VTCR_NSW); + } + return s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS; + default: + g_assert_not_reached(); + } } static bool regime_translation_big_endian(CPUARMState *env, ARMMMUIdx mmu_idx) -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> When HCR_EL2.E2H is enabled, TLB entries are formed using the EL2&0 translation regime, instead of the EL2 translation regime. The TLB VAE2* instructions invalidate the regime that corresponds to the current value of HCR_EL2.E2H. At the moment we only invalidate the EL2 translation regime. This causes problems with RMM, which issues TLBI VAE2IS instructions with HCR_EL2.E2H enabled. Update vae2_tlbmask() to take HCR_EL2.E2H into account. Add vae2_tlbbits() as well, since the top-byte-ignore configuration is different between the EL2&0 and EL2 regime. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230809123706.1842548-3-jean-philippe@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/helper.c | 50 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static int vae1_tlbmask(CPUARMState *env) return mask; } +static int vae2_tlbmask(CPUARMState *env) +{ + uint64_t hcr = arm_hcr_el2_eff(env); + uint16_t mask; + + if (hcr & HCR_E2H) { + mask = ARMMMUIdxBit_E20_2 | + ARMMMUIdxBit_E20_2_PAN | + ARMMMUIdxBit_E20_0; + } else { + mask = ARMMMUIdxBit_E2; + } + return mask; +} + /* Return 56 if TBI is enabled, 64 otherwise. */ static int tlbbits_for_regime(CPUARMState *env, ARMMMUIdx mmu_idx, uint64_t addr) @@ -XXX,XX +XXX,XX @@ static int vae1_tlbbits(CPUARMState *env, uint64_t addr) return tlbbits_for_regime(env, mmu_idx, addr); } +static int vae2_tlbbits(CPUARMState *env, uint64_t addr) +{ + uint64_t hcr = arm_hcr_el2_eff(env); + ARMMMUIdx mmu_idx; + + /* + * Only the regime of the mmu_idx below is significant. + * Regime EL2&0 has two ranges with separate TBI configuration, while EL2 + * only has one. + */ + if (hcr & HCR_E2H) { + mmu_idx = ARMMMUIdx_E20_2; + } else { + mmu_idx = ARMMMUIdx_E2; + } + + return tlbbits_for_regime(env, mmu_idx, addr); +} + static void tlbi_aa64_vmalle1is_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { @@ -XXX,XX +XXX,XX @@ static void tlbi_aa64_vae2_write(CPUARMState *env, const ARMCPRegInfo *ri, * flush-last-level-only. */ CPUState *cs = env_cpu(env); - int mask = e2_tlbmask(env); + int mask = vae2_tlbmask(env); uint64_t pageaddr = sextract64(value << 12, 0, 56); + int bits = vae2_tlbbits(env, pageaddr); - tlb_flush_page_by_mmuidx(cs, pageaddr, mask); + tlb_flush_page_bits_by_mmuidx(cs, pageaddr, mask, bits); } static void tlbi_aa64_vae3_write(CPUARMState *env, const ARMCPRegInfo *ri, @@ -XXX,XX +XXX,XX @@ static void tlbi_aa64_vae2is_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { CPUState *cs = env_cpu(env); + int mask = vae2_tlbmask(env); uint64_t pageaddr = sextract64(value << 12, 0, 56); - int bits = tlbbits_for_regime(env, ARMMMUIdx_E2, pageaddr); + int bits = vae2_tlbbits(env, pageaddr); - tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr, - ARMMMUIdxBit_E2, bits); + tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr, mask, bits); } static void tlbi_aa64_vae3is_write(CPUARMState *env, const ARMCPRegInfo *ri, @@ -XXX,XX +XXX,XX @@ static void tlbi_aa64_rvae1is_write(CPUARMState *env, do_rvae_write(env, value, vae1_tlbmask(env), true); } -static int vae2_tlbmask(CPUARMState *env) -{ - return ARMMMUIdxBit_E2; -} - static void tlbi_aa64_rvae2_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> GPC checks are not performed on the output address for AT instructions, as stated by ARM DDI 0487J in D8.12.2: When populating PAR_EL1 with the result of an address translation instruction, granule protection checks are not performed on the final output address of a successful translation. Rename get_phys_addr_with_secure(), since it's only used to handle AT instructions. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230809123706.1842548-4-jean-philippe@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/internals.h | 25 ++++++++++++++----------- target/arm/helper.c | 8 ++++++-- target/arm/ptw.c | 11 ++++++----- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/target/arm/internals.h b/target/arm/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -XXX,XX +XXX,XX @@ typedef struct GetPhysAddrResult { } GetPhysAddrResult; /** - * get_phys_addr_with_secure: get the physical address for a virtual address + * get_phys_addr: get the physical address for a virtual address * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute * @mmu_idx: MMU index indicating required translation regime - * @is_secure: security state for the access * @result: set on translation success. * @fi: set to fault info if the translation fails * @@ -XXX,XX +XXX,XX @@ typedef struct GetPhysAddrResult { * * for PSMAv5 based systems we don't bother to return a full FSR format * value. */ -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, - MMUAccessType access_type, - ARMMMUIdx mmu_idx, bool is_secure, - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) +bool get_phys_addr(CPUARMState *env, target_ulong address, + MMUAccessType access_type, ARMMMUIdx mmu_idx, + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) __attribute__((nonnull)); /** - * get_phys_addr: get the physical address for a virtual address + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual + * address * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute * @mmu_idx: MMU index indicating required translation regime + * @is_secure: security state for the access * @result: set on translation success. * @fi: set to fault info if the translation fails * - * Similarly, but use the security regime of @mmu_idx. + * Similar to get_phys_addr, but use the given security regime and don't perform + * a Granule Protection Check on the resulting address. */ -bool get_phys_addr(CPUARMState *env, target_ulong address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, bool is_secure, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) __attribute__((nonnull)); bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, ARMMMUFaultInfo fi = {}; GetPhysAddrResult res = {}; - ret = get_phys_addr_with_secure(env, value, access_type, mmu_idx, - is_secure, &res, &fi); + /* + * I_MXTJT: Granule protection checks are not performed on the final address + * of a successful translation. + */ + ret = get_phys_addr_with_secure_nogpc(env, value, access_type, mmu_idx, + is_secure, &res, &fi); /* * ATS operations only do S1 or S1+S2 translations, so we never diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw, return false; } -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool is_secure, GetPhysAddrResult *result, - ARMMMUFaultInfo *fi) +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, bool is_secure, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) { S1Translate ptw = { .in_mmu_idx = mmu_idx, .in_space = arm_secure_to_space(is_secure), }; - return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi); + return get_phys_addr_nogpc(env, &ptw, address, access_type, result, fi); } bool get_phys_addr(CPUARMState *env, target_ulong address, -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> At the moment we only handle Secure and Nonsecure security spaces for the AT instructions. Add support for Realm and Root. For AArch64, arm_security_space() gives the desired space. ARM DDI0487J says (R_NYXTL): If EL3 is implemented, then when an address translation instruction that applies to an Exception level lower than EL3 is executed, the Effective value of SCR_EL3.{NSE, NS} determines the target Security state that the instruction applies to. For AArch32, some instructions can access NonSecure space from Secure, so we still need to pass the state explicitly to do_ats_write(). Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20230809123706.1842548-5-jean-philippe@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/internals.h | 18 +++++++++--------- target/arm/helper.c | 27 ++++++++++++--------------- target/arm/ptw.c | 12 ++++++------ 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/target/arm/internals.h b/target/arm/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -XXX,XX +XXX,XX @@ bool get_phys_addr(CPUARMState *env, target_ulong address, __attribute__((nonnull)); /** - * get_phys_addr_with_secure_nogpc: get the physical address for a virtual - * address + * get_phys_addr_with_space_nogpc: get the physical address for a virtual + * address * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute * @mmu_idx: MMU index indicating required translation regime - * @is_secure: security state for the access + * @space: security space for the access * @result: set on translation success. * @fi: set to fault info if the translation fails * - * Similar to get_phys_addr, but use the given security regime and don't perform + * Similar to get_phys_addr, but use the given security space and don't perform * a Granule Protection Check on the resulting address. */ -bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, - MMUAccessType access_type, - ARMMMUIdx mmu_idx, bool is_secure, - GetPhysAddrResult *result, - ARMMMUFaultInfo *fi) +bool get_phys_addr_with_space_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, ARMSecuritySpace space, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) __attribute__((nonnull)); bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static int par_el1_shareability(GetPhysAddrResult *res) static uint64_t do_ats_write(CPUARMState *env, uint64_t value, MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool is_secure) + ARMSecuritySpace ss) { bool ret; uint64_t par64; @@ -XXX,XX +XXX,XX @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, * I_MXTJT: Granule protection checks are not performed on the final address * of a successful translation. */ - ret = get_phys_addr_with_secure_nogpc(env, value, access_type, mmu_idx, - is_secure, &res, &fi); + ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss, + &res, &fi); /* * ATS operations only do S1 or S1+S2 translations, so we never @@ -XXX,XX +XXX,XX @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) uint64_t par64; ARMMMUIdx mmu_idx; int el = arm_current_el(env); - bool secure = arm_is_secure_below_el3(env); + ARMSecuritySpace ss = arm_security_space(env); switch (ri->opc2 & 6) { case 0: @@ -XXX,XX +XXX,XX @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) switch (el) { case 3: mmu_idx = ARMMMUIdx_E3; - secure = true; break; case 2: - g_assert(!secure); /* ARMv8.4-SecEL2 is 64-bit only */ + g_assert(ss != ARMSS_Secure); /* ARMv8.4-SecEL2 is 64-bit only */ /* fall through */ case 1: if (ri->crm == 9 && (env->uncached_cpsr & CPSR_PAN)) { @@ -XXX,XX +XXX,XX @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) switch (el) { case 3: mmu_idx = ARMMMUIdx_E10_0; - secure = true; break; case 2: - g_assert(!secure); /* ARMv8.4-SecEL2 is 64-bit only */ + g_assert(ss != ARMSS_Secure); /* ARMv8.4-SecEL2 is 64-bit only */ mmu_idx = ARMMMUIdx_Stage1_E0; break; case 1: @@ -XXX,XX +XXX,XX @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) case 4: /* stage 1+2 NonSecure PL1: ATS12NSOPR, ATS12NSOPW */ mmu_idx = ARMMMUIdx_E10_1; - secure = false; + ss = ARMSS_NonSecure; break; case 6: /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */ mmu_idx = ARMMMUIdx_E10_0; - secure = false; + ss = ARMSS_NonSecure; break; default: g_assert_not_reached(); } - par64 = do_ats_write(env, value, access_type, mmu_idx, secure); + par64 = do_ats_write(env, value, access_type, mmu_idx, ss); A32_BANKED_CURRENT_REG_SET(env, par, par64); #else @@ -XXX,XX +XXX,XX @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t par64; /* There is no SecureEL2 for AArch32. */ - par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2, false); + par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2, + ARMSS_NonSecure); A32_BANKED_CURRENT_REG_SET(env, par, par64); #else @@ -XXX,XX +XXX,XX @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri, #ifdef CONFIG_TCG MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD; ARMMMUIdx mmu_idx; - int secure = arm_is_secure_below_el3(env); uint64_t hcr_el2 = arm_hcr_el2_eff(env); bool regime_e20 = (hcr_el2 & (HCR_E2H | HCR_TGE)) == (HCR_E2H | HCR_TGE); @@ -XXX,XX +XXX,XX @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri, break; case 6: /* AT S1E3R, AT S1E3W */ mmu_idx = ARMMMUIdx_E3; - secure = true; break; default: g_assert_not_reached(); @@ -XXX,XX +XXX,XX @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri, } env->cp15.par_el[1] = do_ats_write(env, value, access_type, - mmu_idx, secure); + mmu_idx, arm_security_space(env)); #else /* Handled by hardware accelerator. */ g_assert_not_reached(); diff --git a/target/arm/ptw.c b/target/arm/ptw.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw, return false; } -bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, - MMUAccessType access_type, - ARMMMUIdx mmu_idx, bool is_secure, - GetPhysAddrResult *result, - ARMMMUFaultInfo *fi) +bool get_phys_addr_with_space_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, ARMSecuritySpace space, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) { S1Translate ptw = { .in_mmu_idx = mmu_idx, - .in_space = arm_secure_to_space(is_secure), + .in_space = space, }; return get_phys_addr_nogpc(env, &ptw, address, access_type, result, fi); } -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> The AT instruction is UNDEFINED if the {NSE,NS} configuration is invalid. Add a function to check this on all AT instructions that apply to an EL lower than 3. Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Message-id: 20230809123706.1842548-6-jean-philippe@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/helper.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri, #endif /* CONFIG_TCG */ } +static CPAccessResult at_e012_access(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ + /* + * R_NYXTL: instruction is UNDEFINED if it applies to an Exception level + * lower than EL3 and the combination SCR_EL3.{NSE,NS} is reserved. This can + * only happen when executing at EL3 because that combination also causes an + * illegal exception return. We don't need to check FEAT_RME either, because + * scr_write() ensures that the NSE bit is not set otherwise. + */ + if ((env->cp15.scr_el3 & (SCR_NSE | SCR_NS)) == SCR_NSE) { + return CP_ACCESS_TRAP; + } + return CP_ACCESS_OK; +} + static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) { @@ -XXX,XX +XXX,XX @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri, !(env->cp15.scr_el3 & (SCR_NS | SCR_EEL2))) { return CP_ACCESS_TRAP; } - return CP_ACCESS_OK; + return at_e012_access(env, ri, isread); } static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri, @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo v8_cp_reginfo[] = { .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 0, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E1R, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S1E1W", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 1, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E1W, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S1E0R", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 2, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E0R, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S1E0W", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 3, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E0W, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S12E1R", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 4, .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S12E1W", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 5, .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S12E0R", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 6, .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S12E0W", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 7, .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, /* AT S1E2* are elsewhere as they UNDEF from EL3 if EL2 is not present */ { .name = "AT_S1E3R", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 6, .crn = 7, .crm = 8, .opc2 = 0, @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo ats1e1_reginfo[] = { .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 9, .opc2 = 0, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E1RP, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, { .name = "AT_S1E1WP", .state = ARM_CP_STATE_AA64, .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 9, .opc2 = 1, .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .fgt = FGT_ATS1E1WP, - .writefn = ats_write64 }, + .accessfn = at_e012_access, .writefn = ats_write64 }, }; static const ARMCPRegInfo ats1cp_reginfo[] = { -- 2.34.1 From: Jean-Philippe Brucker <jean-philippe@linaro.org> When FEAT_RME is implemented, these bits override the value of CNT[VP]_CTL_EL0.IMASK in Realm and Root state. Move the IRQ state update into a new gt_update_irq() function and test those bits every time we recompute the IRQ state. Since we're removing the IRQ state from some trace events, add a new trace event for gt_update_irq(). Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Message-id: 20230809123706.1842548-7-jean-philippe@linaro.org [PMM: only register change hook if not USER_ONLY and if TCG] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/cpu.h | 4 +++ target/arm/cpu.c | 6 ++++ target/arm/helper.c | 65 ++++++++++++++++++++++++++++++++++------- target/arm/trace-events | 7 +++-- 4 files changed, 68 insertions(+), 14 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ struct ArchCPU { }; unsigned int gt_cntfrq_period_ns(ARMCPU *cpu); +void gt_rme_post_el_change(ARMCPU *cpu, void *opaque); void arm_cpu_post_init(Object *obj); @@ -XXX,XX +XXX,XX @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask) #define HSTR_TTEE (1 << 16) #define HSTR_TJDBX (1 << 17) +#define CNTHCTL_CNTVMASK (1 << 18) +#define CNTHCTL_CNTPMASK (1 << 19) + /* Return the current FPSCR value. */ uint32_t vfp_get_fpscr(CPUARMState *env); void vfp_set_fpscr(CPUARMState *env, uint32_t val); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -XXX,XX +XXX,XX @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) set_feature(env, ARM_FEATURE_VBAR); } +#ifndef CONFIG_USER_ONLY + if (tcg_enabled() && cpu_isar_feature(aa64_rme, cpu)) { + arm_register_el_change_hook(cpu, &gt_rme_post_el_change, 0); + } +#endif + register_cp_regs_for_features(cpu); arm_cpu_register_gdb_regs_for_features(cpu); diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static uint64_t gt_get_countervalue(CPUARMState *env) return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu); } +static void gt_update_irq(ARMCPU *cpu, int timeridx) +{ + CPUARMState *env = &cpu->env; + uint64_t cnthctl = env->cp15.cnthctl_el2; + ARMSecuritySpace ss = arm_security_space(env); + /* ISTATUS && !IMASK */ + int irqstate = (env->cp15.c14_timer[timeridx].ctl & 6) == 4; + + /* + * If bit CNTHCTL_EL2.CNT[VP]MASK is set, it overrides IMASK. + * It is RES0 in Secure and NonSecure state. + */ + if ((ss == ARMSS_Root || ss == ARMSS_Realm) && + ((timeridx == GTIMER_VIRT && (cnthctl & CNTHCTL_CNTVMASK)) || + (timeridx == GTIMER_PHYS && (cnthctl & CNTHCTL_CNTPMASK)))) { + irqstate = 0; + } + + qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate); + trace_arm_gt_update_irq(timeridx, irqstate); +} + +void gt_rme_post_el_change(ARMCPU *cpu, void *ignored) +{ + /* + * Changing security state between Root and Secure/NonSecure, which may + * happen when switching EL, can change the effective value of CNTHCTL_EL2 + * mask bits. Update the IRQ state accordingly. + */ + gt_update_irq(cpu, GTIMER_VIRT); + gt_update_irq(cpu, GTIMER_PHYS); +} + static void gt_recalc_timer(ARMCPU *cpu, int timeridx) { ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx]; @@ -XXX,XX +XXX,XX @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) /* Note that this must be unsigned 64 bit arithmetic: */ int istatus = count - offset >= gt->cval; uint64_t nexttick; - int irqstate; gt->ctl = deposit32(gt->ctl, 2, 1, istatus); - irqstate = (istatus && !(gt->ctl & 2)); - qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate); - if (istatus) { /* Next transition is when count rolls back over to zero */ nexttick = UINT64_MAX; @@ -XXX,XX +XXX,XX @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) } else { timer_mod(cpu->gt_timer[timeridx], nexttick); } - trace_arm_gt_recalc(timeridx, irqstate, nexttick); + trace_arm_gt_recalc(timeridx, nexttick); } else { /* Timer disabled: ISTATUS and timer output always clear */ gt->ctl &= ~4; - qemu_set_irq(cpu->gt_timer_outputs[timeridx], 0); timer_del(cpu->gt_timer[timeridx]); trace_arm_gt_recalc_disabled(timeridx); } + gt_update_irq(cpu, timeridx); } static void gt_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri, @@ -XXX,XX +XXX,XX @@ static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, * IMASK toggled: don't need to recalculate, * just set the interrupt line based on ISTATUS */ - int irqstate = (oldval & 4) && !(value & 2); - - trace_arm_gt_imask_toggle(timeridx, irqstate); - qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate); + trace_arm_gt_imask_toggle(timeridx); + gt_update_irq(cpu, timeridx); } } @@ -XXX,XX +XXX,XX @@ static void gt_virt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, gt_ctl_write(env, ri, GTIMER_VIRT, value); } +static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ + ARMCPU *cpu = env_archcpu(env); + uint32_t oldval = env->cp15.cnthctl_el2; + + raw_write(env, ri, value); + + if ((oldval ^ value) & CNTHCTL_CNTVMASK) { + gt_update_irq(cpu, GTIMER_VIRT); + } else if ((oldval ^ value) & CNTHCTL_CNTPMASK) { + gt_update_irq(cpu, GTIMER_PHYS); + } +} + static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo el2_cp_reginfo[] = { * reset values as IMPDEF. We choose to reset to 3 to comply with * both ARMv7 and ARMv8. */ - .access = PL2_RW, .resetvalue = 3, + .access = PL2_RW, .type = ARM_CP_IO, .resetvalue = 3, + .writefn = gt_cnthctl_write, .raw_writefn = raw_write, .fieldoffset = offsetof(CPUARMState, cp15.cnthctl_el2) }, { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3, diff --git a/target/arm/trace-events b/target/arm/trace-events index XXXXXXX..XXXXXXX 100644 --- a/target/arm/trace-events +++ b/target/arm/trace-events @@ -XXX,XX +XXX,XX @@ # See docs/devel/tracing.rst for syntax documentation. # helper.c -arm_gt_recalc(int timer, int irqstate, uint64_t nexttick) "gt recalc: timer %d irqstate %d next tick 0x%" PRIx64 -arm_gt_recalc_disabled(int timer) "gt recalc: timer %d irqstate 0 timer disabled" +arm_gt_recalc(int timer, uint64_t nexttick) "gt recalc: timer %d next tick 0x%" PRIx64 +arm_gt_recalc_disabled(int timer) "gt recalc: timer %d timer disabled" arm_gt_cval_write(int timer, uint64_t value) "gt_cval_write: timer %d value 0x%" PRIx64 arm_gt_tval_write(int timer, uint64_t value) "gt_tval_write: timer %d value 0x%" PRIx64 arm_gt_ctl_write(int timer, uint64_t value) "gt_ctl_write: timer %d value 0x%" PRIx64 -arm_gt_imask_toggle(int timer, int irqstate) "gt_ctl_write: timer %d IMASK toggle, new irqstate %d" +arm_gt_imask_toggle(int timer) "gt_ctl_write: timer %d IMASK toggle" arm_gt_cntvoff_write(uint64_t value) "gt_cntvoff_write: value 0x%" PRIx64 +arm_gt_update_irq(int timer, int irqstate) "gt_update_irq: timer %d irqstate %d" # kvm.c kvm_arm_fixup_msi_route(uint64_t iova, uint64_t gpa) "MSI iova = 0x%"PRIx64" is translated into 0x%"PRIx64 -- 2.34.1 From: Richard Henderson <richard.henderson@linaro.org> A typo, noted in the bug report, resulting in an incorrect write offset. Cc: qemu-stable@nongnu.org Fixes: 7390e0e9ab8 ("target/arm: Implement SME LD1, ST1") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1833 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-id: 20230818214255.146905-1-richard.henderson@linaro.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/sme_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/tcg/sme_helper.c b/target/arm/tcg/sme_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/sme_helper.c +++ b/target/arm/tcg/sme_helper.c @@ -XXX,XX +XXX,XX @@ static inline void HNAME##_host(void *za, intptr_t off, void *host) \ { \ uint64_t *ptr = za + off; \ HOST(host, ptr[BE]); \ - HOST(host + 1, ptr[!BE]); \ + HOST(host + 8, ptr[!BE]); \ } \ static inline void VNAME##_v_host(void *za, intptr_t off, void *host) \ { \ -- 2.34.1 From: Richard Henderson <richard.henderson@linaro.org> Typo applied byte-wise shift instead of double-word shift. Cc: qemu-stable@nongnu.org Fixes: 631e565450c ("target/arm: Create gen_gvec_[us]sra") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1737 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-id: 20230821022025.397682-1-richard.henderson@linaro.org Signed-off-by: Peter Maydell <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 @@ void gen_gvec_ssra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, .vece = MO_32 }, { .fni8 = gen_ssra64_i64, .fniv = gen_ssra_vec, - .fno = gen_helper_gvec_ssra_b, + .fno = gen_helper_gvec_ssra_d, .prefer_i64 = TCG_TARGET_REG_BITS == 64, .opt_opc = vecop_list, .load_dest = true, -- 2.34.1 Hi; here's a target-arm pullreq for rc0; these are all bugfixes and similar minor stuff. thanks -- PMM The following changes since commit 0462a32b4f63b2448b4a196381138afd50719dc4: Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging (2025-03-14 09:31:13 +0800) are available in the Git repository at: https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20250314-1 for you to fetch changes up to a019e15edfd62beae1e2f6adc0fa7415ba20b14c: meson.build: Set RUST_BACKTRACE for all tests (2025-03-14 12:54:33 +0000) ---------------------------------------------------------------- target-arm queue: * Correctly handle corner cases of guest attempting an exception return to AArch32 when target EL is AArch64 only * MAINTAINERS: Fix status for Arm boards I "maintain" * tests/functional: Bump up arm_replay timeout * Revert "hw/char/pl011: Warn when using disabled receiver" * util/cacheflush: Make first DSB unconditional on aarch64 * target/arm: Fix SVE/SME access check logic * meson.build: Set RUST_BACKTRACE for all tests ---------------------------------------------------------------- Joe Komlodi (1): util/cacheflush: Make first DSB unconditional on aarch64 Paolo Bonzini (1): Revert "hw/char/pl011: Warn when using disabled receiver" Peter Maydell (13): target/arm: Move A32_BANKED_REG_{GET,SET} macros to cpregs.h target/arm: Un-inline access_secure_reg() linux-user/aarch64: Remove unused get/put_user macros linux-user/arm: Remove unused get_put_user macros target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h target/arm: SCR_EL3.RW should be treated as 1 if EL2 doesn't support AArch32 target/arm: HCR_EL2.RW should be RAO/WI if EL1 doesn't support AArch32 target/arm: Add cpu local variable to exception_return helper target/arm: Forbid return to AArch32 when CPU is AArch64-only MAINTAINERS: Fix status for Arm boards I "maintain" tests/functional: Bump up arm_replay timeout meson.build: Set RUST_BACKTRACE for all tests Richard Henderson (2): target/arm: Make DisasContext.{fp, sve}_access_checked tristate target/arm: Simplify pstate_sm check in sve_access_check MAINTAINERS | 14 ++-- meson.build | 9 ++- target/arm/cpregs.h | 28 +++++++ target/arm/cpu.h | 153 +----------------------------------- target/arm/internals.h | 135 +++++++++++++++++++++++++++++++ target/arm/tcg/translate-a64.h | 2 +- target/arm/tcg/translate.h | 10 ++- hw/char/pl011.c | 19 ++--- hw/intc/arm_gicv3_cpuif.c | 1 + linux-user/aarch64/cpu_loop.c | 48 ----------- linux-user/arm/cpu_loop.c | 43 +--------- target/arm/arch_dump.c | 1 + target/arm/helper.c | 16 +++- target/arm/tcg/helper-a64.c | 12 ++- target/arm/tcg/hflags.c | 9 +++ target/arm/tcg/translate-a64.c | 37 ++++----- util/cacheflush.c | 4 +- .gitlab-ci.d/buildtest-template.yml | 1 - 18 files changed, 257 insertions(+), 285 deletions(-) The A32_BANKED_REG_{GET,SET} macros are only used inside target/arm; move their definitions to cpregs.h. There's no need to have them defined in all the code that includes cpu.h. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpregs.h | 28 ++++++++++++++++++++++++++++ target/arm/cpu.h | 27 --------------------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpregs.h +++ b/target/arm/cpregs.h @@ -XXX,XX +XXX,XX @@ static inline bool arm_cpreg_traps_in_nv(const ARMCPRegInfo *ri) return ri->opc1 == 4 || ri->opc1 == 5; } +/* Macros for accessing a specified CP register bank */ +#define A32_BANKED_REG_GET(_env, _regname, _secure) \ + ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns) + +#define A32_BANKED_REG_SET(_env, _regname, _secure, _val) \ + do { \ + if (_secure) { \ + (_env)->cp15._regname##_s = (_val); \ + } else { \ + (_env)->cp15._regname##_ns = (_val); \ + } \ + } while (0) + +/* + * Macros for automatically accessing a specific CP register bank depending on + * the current secure state of the system. These macros are not intended for + * supporting instruction translation reads/writes as these are dependent + * solely on the SCR.NS bit and not the mode. + */ +#define A32_BANKED_CURRENT_REG_GET(_env, _regname) \ + A32_BANKED_REG_GET((_env), _regname, \ + (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3))) + +#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) \ + A32_BANKED_REG_SET((_env), _regname, \ + (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \ + (_val)) + #endif /* TARGET_ARM_CPREGS_H */ diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ static inline bool access_secure_reg(CPUARMState *env) return ret; } -/* Macros for accessing a specified CP register bank */ -#define A32_BANKED_REG_GET(_env, _regname, _secure) \ - ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns) - -#define A32_BANKED_REG_SET(_env, _regname, _secure, _val) \ - do { \ - if (_secure) { \ - (_env)->cp15._regname##_s = (_val); \ - } else { \ - (_env)->cp15._regname##_ns = (_val); \ - } \ - } while (0) - -/* Macros for automatically accessing a specific CP register bank depending on - * the current secure state of the system. These macros are not intended for - * supporting instruction translation reads/writes as these are dependent - * solely on the SCR.NS bit and not the mode. - */ -#define A32_BANKED_CURRENT_REG_GET(_env, _regname) \ - A32_BANKED_REG_GET((_env), _regname, \ - (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3))) - -#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) \ - A32_BANKED_REG_SET((_env), _regname, \ - (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \ - (_val)) - uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, uint32_t cur_el, bool secure); -- 2.43.0 We would like to move arm_el_is_aa64() to internals.h; however, it is used by access_secure_reg(). Make that function not be inline, so that it can stay in cpu.h. access_secure_reg() is used only in two places: * in hflags.c * in the user-mode arm emulators, to decide whether to store the TLS value in the secure or non-secure banked field The second of these is not on a super-hot path that would care about the inlining (and incidentally will always use the NS banked field because our user-mode CPUs never set ARM_FEATURE_EL3); put the definition of access_secure_reg() in hflags.c, near its only use inside target/arm. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 12 +++--------- target/arm/tcg/hflags.c | 9 +++++++++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el) return aa64; } -/* Function for determining whether guest cp register reads and writes should +/* + * Function for determining whether guest cp register reads and writes should * access the secure or non-secure bank of a cp register. When EL3 is * operating in AArch32 state, the NS-bit determines whether the secure * instance of a cp register should be used. When EL3 is AArch64 (or if * it doesn't exist at all) then there is no register banking, and all * accesses are to the non-secure version. */ -static inline bool access_secure_reg(CPUARMState *env) -{ - bool ret = (arm_feature(env, ARM_FEATURE_EL3) && - !arm_el_is_aa64(env, 3) && - !(env->cp15.scr_el3 & SCR_NS)); - - return ret; -} +bool access_secure_reg(CPUARMState *env); uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, uint32_t cur_el, bool secure); diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/hflags.c +++ b/target/arm/tcg/hflags.c @@ -XXX,XX +XXX,XX @@ static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr) #endif } +bool access_secure_reg(CPUARMState *env) +{ + bool ret = (arm_feature(env, ARM_FEATURE_EL3) && + !arm_el_is_aa64(env, 3) && + !(env->cp15.scr_el3 & SCR_NS)); + + return ret; +} + static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx, CPUARMTBFlags flags) -- 2.43.0 At the top of linux-user/aarch64/cpu_loop.c we define a set of macros for reading and writing data and code words, but we never use these macros. Delete them. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/aarch64/cpu_loop.c | 48 ----------------------------------- 1 file changed, 48 deletions(-) diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c index XXXXXXX..XXXXXXX 100644 --- a/linux-user/aarch64/cpu_loop.c +++ b/linux-user/aarch64/cpu_loop.c @@ -XXX,XX +XXX,XX @@ #include "target/arm/syndrome.h" #include "target/arm/cpu-features.h" -#define get_user_code_u32(x, gaddr, env) \ - ({ abi_long __r = get_user_u32((x), (gaddr)); \ - if (!__r && bswap_code(arm_sctlr_b(env))) { \ - (x) = bswap32(x); \ - } \ - __r; \ - }) - -#define get_user_code_u16(x, gaddr, env) \ - ({ abi_long __r = get_user_u16((x), (gaddr)); \ - if (!__r && bswap_code(arm_sctlr_b(env))) { \ - (x) = bswap16(x); \ - } \ - __r; \ - }) - -#define get_user_data_u32(x, gaddr, env) \ - ({ abi_long __r = get_user_u32((x), (gaddr)); \ - if (!__r && arm_cpu_bswap_data(env)) { \ - (x) = bswap32(x); \ - } \ - __r; \ - }) - -#define get_user_data_u16(x, gaddr, env) \ - ({ abi_long __r = get_user_u16((x), (gaddr)); \ - if (!__r && arm_cpu_bswap_data(env)) { \ - (x) = bswap16(x); \ - } \ - __r; \ - }) - -#define put_user_data_u32(x, gaddr, env) \ - ({ typeof(x) __x = (x); \ - if (arm_cpu_bswap_data(env)) { \ - __x = bswap32(__x); \ - } \ - put_user_u32(__x, (gaddr)); \ - }) - -#define put_user_data_u16(x, gaddr, env) \ - ({ typeof(x) __x = (x); \ - if (arm_cpu_bswap_data(env)) { \ - __x = bswap16(__x); \ - } \ - put_user_u16(__x, (gaddr)); \ - }) - /* AArch64 main loop */ void cpu_loop(CPUARMState *env) { -- 2.43.0 In linux-user/arm/cpu_loop.c we define a full set of get/put macros for both code and data (since the endianness handling is different between the two). However the only one we actually use is get_user_code_u32(). Remove the rest. We leave a comment noting how data-side accesses should be handled for big-endian, because that's a subtle point and we just removed the macros that were effectively documenting it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/arm/cpu_loop.c | 43 ++++----------------------------------- 1 file changed, 4 insertions(+), 39 deletions(-) diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c index XXXXXXX..XXXXXXX 100644 --- a/linux-user/arm/cpu_loop.c +++ b/linux-user/arm/cpu_loop.c @@ -XXX,XX +XXX,XX @@ __r; \ }) -#define get_user_code_u16(x, gaddr, env) \ - ({ abi_long __r = get_user_u16((x), (gaddr)); \ - if (!__r && bswap_code(arm_sctlr_b(env))) { \ - (x) = bswap16(x); \ - } \ - __r; \ - }) - -#define get_user_data_u32(x, gaddr, env) \ - ({ abi_long __r = get_user_u32((x), (gaddr)); \ - if (!__r && arm_cpu_bswap_data(env)) { \ - (x) = bswap32(x); \ - } \ - __r; \ - }) - -#define get_user_data_u16(x, gaddr, env) \ - ({ abi_long __r = get_user_u16((x), (gaddr)); \ - if (!__r && arm_cpu_bswap_data(env)) { \ - (x) = bswap16(x); \ - } \ - __r; \ - }) - -#define put_user_data_u32(x, gaddr, env) \ - ({ typeof(x) __x = (x); \ - if (arm_cpu_bswap_data(env)) { \ - __x = bswap32(__x); \ - } \ - put_user_u32(__x, (gaddr)); \ - }) - -#define put_user_data_u16(x, gaddr, env) \ - ({ typeof(x) __x = (x); \ - if (arm_cpu_bswap_data(env)) { \ - __x = bswap16(__x); \ - } \ - put_user_u16(__x, (gaddr)); \ - }) +/* + * Note that if we need to do data accesses here, they should do a + * bswap if arm_cpu_bswap_data() returns true. + */ /* * Similar to code in accel/tcg/user-exec.c, but outside the execution loop. -- 2.43.0 The arm_cpu_data_is_big_endian() and related functions are now used only in target/arm; they can be moved to internals.h. The motivation here is that we would like to move arm_current_el() to internals.h. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 48 ------------------------------------------ target/arm/internals.h | 48 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 48 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ static inline bool arm_sctlr_b(CPUARMState *env) uint64_t arm_sctlr(CPUARMState *env, int el); -static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env, - bool sctlr_b) -{ -#ifdef CONFIG_USER_ONLY - /* - * In system mode, BE32 is modelled in line with the - * architecture (as word-invariant big-endianness), where loads - * and stores are done little endian but from addresses which - * are adjusted by XORing with the appropriate constant. So the - * endianness to use for the raw data access is not affected by - * SCTLR.B. - * In user mode, however, we model BE32 as byte-invariant - * big-endianness (because user-only code cannot tell the - * difference), and so we need to use a data access endianness - * that depends on SCTLR.B. - */ - if (sctlr_b) { - return true; - } -#endif - /* In 32bit endianness is determined by looking at CPSR's E bit */ - return env->uncached_cpsr & CPSR_E; -} - -static inline bool arm_cpu_data_is_big_endian_a64(int el, uint64_t sctlr) -{ - return sctlr & (el ? SCTLR_EE : SCTLR_E0E); -} - -/* Return true if the processor is in big-endian mode. */ -static inline bool arm_cpu_data_is_big_endian(CPUARMState *env) -{ - if (!is_a64(env)) { - return arm_cpu_data_is_big_endian_a32(env, arm_sctlr_b(env)); - } else { - int cur_el = arm_current_el(env); - uint64_t sctlr = arm_sctlr(env, cur_el); - return arm_cpu_data_is_big_endian_a64(cur_el, sctlr); - } -} - #include "exec/cpu-all.h" /* @@ -XXX,XX +XXX,XX @@ static inline bool bswap_code(bool sctlr_b) #endif } -#ifdef CONFIG_USER_ONLY -static inline bool arm_cpu_bswap_data(CPUARMState *env) -{ - return TARGET_BIG_ENDIAN ^ arm_cpu_data_is_big_endian(env); -} -#endif - void cpu_get_tb_cpu_state(CPUARMState *env, vaddr *pc, uint64_t *cs_base, uint32_t *flags); diff --git a/target/arm/internals.h b/target/arm/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -XXX,XX +XXX,XX @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode) return arm_rmode_to_sf_map[rmode]; } +static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env, + bool sctlr_b) +{ +#ifdef CONFIG_USER_ONLY + /* + * In system mode, BE32 is modelled in line with the + * architecture (as word-invariant big-endianness), where loads + * and stores are done little endian but from addresses which + * are adjusted by XORing with the appropriate constant. So the + * endianness to use for the raw data access is not affected by + * SCTLR.B. + * In user mode, however, we model BE32 as byte-invariant + * big-endianness (because user-only code cannot tell the + * difference), and so we need to use a data access endianness + * that depends on SCTLR.B. + */ + if (sctlr_b) { + return true; + } +#endif + /* In 32bit endianness is determined by looking at CPSR's E bit */ + return env->uncached_cpsr & CPSR_E; +} + +static inline bool arm_cpu_data_is_big_endian_a64(int el, uint64_t sctlr) +{ + return sctlr & (el ? SCTLR_EE : SCTLR_E0E); +} + +/* Return true if the processor is in big-endian mode. */ +static inline bool arm_cpu_data_is_big_endian(CPUARMState *env) +{ + if (!is_a64(env)) { + return arm_cpu_data_is_big_endian_a32(env, arm_sctlr_b(env)); + } else { + int cur_el = arm_current_el(env); + uint64_t sctlr = arm_sctlr(env, cur_el); + return arm_cpu_data_is_big_endian_a64(cur_el, sctlr); + } +} + +#ifdef CONFIG_USER_ONLY +static inline bool arm_cpu_bswap_data(CPUARMState *env) +{ + return TARGET_BIG_ENDIAN ^ arm_cpu_data_is_big_endian(env); +} +#endif + static inline void aarch64_save_sp(CPUARMState *env, int el) { if (env->pstate & PSTATE_SP) { -- 2.43.0 The functions arm_current_el() and arm_el_is_aa64() are used only in target/arm and in hw/intc/arm_gicv3_cpuif.c. They're functions that query internal state of the CPU. Move them out of cpu.h and into internals.h. This means we need to include internals.h in arm_gicv3_cpuif.c, but this is justifiable because that file is implementing the GICv3 CPU interface, which really is part of the CPU proper; we just ended up implementing it in code in hw/intc/ for historical reasons. The motivation for this move is that we'd like to change arm_el_is_aa64() to add a condition that uses cpu_isar_feature(); but we don't want to include cpu-features.h in cpu.h. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 66 -------------------------------------- target/arm/internals.h | 67 +++++++++++++++++++++++++++++++++++++++ hw/intc/arm_gicv3_cpuif.c | 1 + target/arm/arch_dump.c | 1 + 4 files changed, 69 insertions(+), 66 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space); uint64_t arm_hcr_el2_eff(CPUARMState *env); uint64_t arm_hcrx_el2_eff(CPUARMState *env); -/* Return true if the specified exception level is running in AArch64 state. */ -static inline bool arm_el_is_aa64(CPUARMState *env, int el) -{ - /* This isn't valid for EL0 (if we're in EL0, is_a64() is what you want, - * and if we're not in EL0 then the state of EL0 isn't well defined.) - */ - assert(el >= 1 && el <= 3); - bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64); - - /* The highest exception level is always at the maximum supported - * register width, and then lower levels have a register width controlled - * by bits in the SCR or HCR registers. - */ - if (el == 3) { - return aa64; - } - - if (arm_feature(env, ARM_FEATURE_EL3) && - ((env->cp15.scr_el3 & SCR_NS) || !(env->cp15.scr_el3 & SCR_EEL2))) { - aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW); - } - - if (el == 2) { - return aa64; - } - - if (arm_is_el2_enabled(env)) { - aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW); - } - - return aa64; -} - /* * Function for determining whether guest cp register reads and writes should * access the secure or non-secure bank of a cp register. When EL3 is @@ -XXX,XX +XXX,XX @@ static inline bool arm_v7m_is_handler_mode(CPUARMState *env) return env->v7m.exception != 0; } -/* Return the current Exception Level (as per ARMv8; note that this differs - * from the ARMv7 Privilege Level). - */ -static inline int arm_current_el(CPUARMState *env) -{ - if (arm_feature(env, ARM_FEATURE_M)) { - return arm_v7m_is_handler_mode(env) || - !(env->v7m.control[env->v7m.secure] & 1); - } - - if (is_a64(env)) { - return extract32(env->pstate, 2, 2); - } - - switch (env->uncached_cpsr & 0x1f) { - case ARM_CPU_MODE_USR: - return 0; - case ARM_CPU_MODE_HYP: - return 2; - case ARM_CPU_MODE_MON: - return 3; - default: - if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) { - /* If EL3 is 32-bit then all secure privileged modes run in - * EL3 - */ - return 3; - } - - return 1; - } -} - /** * write_list_to_cpustate * @cpu: ARMCPU diff --git a/target/arm/internals.h b/target/arm/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -XXX,XX +XXX,XX @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode) return arm_rmode_to_sf_map[rmode]; } +/* Return true if the specified exception level is running in AArch64 state. */ +static inline bool arm_el_is_aa64(CPUARMState *env, int el) +{ + /* + * This isn't valid for EL0 (if we're in EL0, is_a64() is what you want, + * and if we're not in EL0 then the state of EL0 isn't well defined.) + */ + assert(el >= 1 && el <= 3); + bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64); + + /* + * The highest exception level is always at the maximum supported + * register width, and then lower levels have a register width controlled + * by bits in the SCR or HCR registers. + */ + if (el == 3) { + return aa64; + } + + if (arm_feature(env, ARM_FEATURE_EL3) && + ((env->cp15.scr_el3 & SCR_NS) || !(env->cp15.scr_el3 & SCR_EEL2))) { + aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW); + } + + if (el == 2) { + return aa64; + } + + if (arm_is_el2_enabled(env)) { + aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW); + } + + return aa64; +} + +/* + * Return the current Exception Level (as per ARMv8; note that this differs + * from the ARMv7 Privilege Level). + */ +static inline int arm_current_el(CPUARMState *env) +{ + if (arm_feature(env, ARM_FEATURE_M)) { + return arm_v7m_is_handler_mode(env) || + !(env->v7m.control[env->v7m.secure] & 1); + } + + if (is_a64(env)) { + return extract32(env->pstate, 2, 2); + } + + switch (env->uncached_cpsr & 0x1f) { + case ARM_CPU_MODE_USR: + return 0; + case ARM_CPU_MODE_HYP: + return 2; + case ARM_CPU_MODE_MON: + return 3; + default: + if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) { + /* If EL3 is 32-bit then all secure privileged modes run in EL3 */ + return 3; + } + + return 1; + } +} + static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env, bool sctlr_b) { 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 @@ #include "cpu.h" #include "target/arm/cpregs.h" #include "target/arm/cpu-features.h" +#include "target/arm/internals.h" #include "system/tcg.h" #include "system/qtest.h" diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/arch_dump.c +++ b/target/arm/arch_dump.c @@ -XXX,XX +XXX,XX @@ #include "elf.h" #include "system/dump.h" #include "cpu-features.h" +#include "internals.h" /* struct user_pt_regs from arch/arm64/include/uapi/asm/ptrace.h */ struct aarch64_user_regs { -- 2.43.0 The definition of SCR_EL3.RW says that its effective value is 1 if: - EL2 is implemented and does not support AArch32, and SCR_EL3.NS is 1 - the effective value of SCR_EL3.{EEL2,NS} is {1,0} (i.e. we are Secure and Secure EL2 is disabled) We implement the second of these in arm_el_is_aa64(), but forgot the first. Provide a new function arm_scr_rw_eff() to return the effective value of SCR_EL3.RW, and use it in arm_el_is_aa64() and the other places that currently look directly at the bit value. (scr_write() enforces that the RW bit is RAO/WI if neither EL1 nor EL2 have AArch32 support, but if EL1 does but EL2 does not then the bit must still be writeable.) This will mean that if code at EL3 attempts to perform an exception return to AArch32 EL2 when EL2 is AArch64-only we will correctly handle this as an illegal exception return: it will be caught by the "return to an EL which is configured for a different register width" check in HELPER(exception_return). We do already have some CPU types which don't implement AArch32 above EL0, so this is technically a bug; it doesn't seem worth backporting to stable because no sensible guest code will be deliberately attempting to set the RW bit to a value corresponding to an unimplemented execution state and then checking that we did the right thing. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/internals.h | 26 +++++++++++++++++++++++--- target/arm/helper.c | 4 ++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/target/arm/internals.h b/target/arm/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -XXX,XX +XXX,XX @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode) return arm_rmode_to_sf_map[rmode]; } +/* Return the effective value of SCR_EL3.RW */ +static inline bool arm_scr_rw_eff(CPUARMState *env) +{ + /* + * SCR_EL3.RW has an effective value of 1 if: + * - we are NS and EL2 is implemented but doesn't support AArch32 + * - we are S and EL2 is enabled (in which case it must be AArch64) + */ + ARMCPU *cpu = env_archcpu(env); + + if (env->cp15.scr_el3 & SCR_RW) { + return true; + } + if (env->cp15.scr_el3 & SCR_NS) { + return arm_feature(env, ARM_FEATURE_EL2) && + !cpu_isar_feature(aa64_aa32_el2, cpu); + } else { + return env->cp15.scr_el3 & SCR_EEL2; + } +} + /* Return true if the specified exception level is running in AArch64 state. */ static inline bool arm_el_is_aa64(CPUARMState *env, int el) { @@ -XXX,XX +XXX,XX @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el) return aa64; } - if (arm_feature(env, ARM_FEATURE_EL3) && - ((env->cp15.scr_el3 & SCR_NS) || !(env->cp15.scr_el3 & SCR_EEL2))) { - aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW); + if (arm_feature(env, ARM_FEATURE_EL3)) { + aa64 = aa64 && arm_scr_rw_eff(env); } if (el == 2) { diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, uint64_t hcr_el2; if (arm_feature(env, ARM_FEATURE_EL3)) { - rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW); + rw = arm_scr_rw_eff(env); } else { /* * Either EL2 is the highest EL (and so the EL2 register width @@ -XXX,XX +XXX,XX @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) switch (new_el) { case 3: - is_aa64 = (env->cp15.scr_el3 & SCR_RW) != 0; + is_aa64 = arm_scr_rw_eff(env); break; case 2: hcr = arm_hcr_el2_eff(env); -- 2.43.0 When EL1 doesn't support AArch32, the HCR_EL2.RW bit is supposed to be RAO/WI. Enforce the RAO/WI behaviour. Note that we handle "reset value should honour RES1 bits" in the same way that SCR_EL3 does, via a reset function. We do already have some CPU types which don't implement AArch32 above EL0, so this is technically a bug; it doesn't seem worth backporting to stable because no sensible guest code will be deliberately attempting to set the RW bit to a value corresponding to an unimplemented execution state and then checking that we did the right thing. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -XXX,XX +XXX,XX @@ static void do_hcr_write(CPUARMState *env, uint64_t value, uint64_t valid_mask) /* Clear RES0 bits. */ value &= valid_mask; + /* RW is RAO/WI if EL1 is AArch64 only */ + if (!cpu_isar_feature(aa64_aa32_el1, cpu)) { + value |= HCR_RW; + } + /* * These bits change the MMU setup: * HCR_VM enables stage 2 translation @@ -XXX,XX +XXX,XX @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, do_hcr_write(env, value, MAKE_64BIT_MASK(32, 32)); } +static void hcr_reset(CPUARMState *env, const ARMCPRegInfo *ri) +{ + /* hcr_write will set the RES1 bits on an AArch64-only CPU */ + hcr_write(env, ri, 0); +} + /* * Return the effective value of HCR_EL2, at the given security state. * Bits that are not included here: @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo el2_cp_reginfo[] = { .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), .nv2_redirect_offset = 0x78, + .resetfn = hcr_reset, .writefn = hcr_write, .raw_writefn = raw_write }, { .name = "HCR", .state = ARM_CP_STATE_AA32, .type = ARM_CP_ALIAS | ARM_CP_IO, -- 2.43.0 We already call env_archcpu() multiple times within the exception_return helper function, and we're about to want to add another use of the ARMCPU pointer. Add a local variable cpu so we can call env_archcpu() just once. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/tcg/helper-a64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/helper-a64.c +++ b/target/arm/tcg/helper-a64.c @@ -XXX,XX +XXX,XX @@ static void cpsr_write_from_spsr_elx(CPUARMState *env, void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) { + ARMCPU *cpu = env_archcpu(env); int cur_el = arm_current_el(env); unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el); uint32_t spsr = env->banked_spsr[spsr_idx]; @@ -XXX,XX +XXX,XX @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) } bql_lock(); - arm_call_pre_el_change_hook(env_archcpu(env)); + arm_call_pre_el_change_hook(cpu); bql_unlock(); if (!return_to_aa64) { @@ -XXX,XX +XXX,XX @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) int tbii; env->aarch64 = true; - spsr &= aarch64_pstate_valid_mask(&env_archcpu(env)->isar); + spsr &= aarch64_pstate_valid_mask(&cpu->isar); pstate_write(env, spsr); if (!arm_singlestep_active(env)) { env->pstate &= ~PSTATE_SS; @@ -XXX,XX +XXX,XX @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) aarch64_sve_change_el(env, cur_el, new_el, return_to_aa64); bql_lock(); - arm_call_el_change_hook(env_archcpu(env)); + arm_call_el_change_hook(cpu); bql_unlock(); return; -- 2.43.0 In the Arm ARM, rule R_TYTWB states that returning to AArch32 is an illegal exception return if: * AArch32 is not supported at any exception level * the target EL is configured for AArch64 via SCR_EL3.RW or HCR_EL2.RW or via CPU state at reset We check the second of these, but not the first (which can only be relevant for the case of a return to EL0, because if AArch32 is not supported at one of the higher ELs then the RW bits will have an effective value of 1 and the the "configured for AArch64" condition will hold also). Add the missing condition. Although this is technically a bug (because we have one AArch64-only CPU: a64fx) it isn't worth backporting to stable because no sensible guest code will deliberately try to return to a nonexistent execution state to check that it gets an illegal exception return. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/tcg/helper-a64.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/helper-a64.c +++ b/target/arm/tcg/helper-a64.c @@ -XXX,XX +XXX,XX @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc) goto illegal_return; } + if (!return_to_aa64 && !cpu_isar_feature(aa64_aa32, cpu)) { + /* Return to AArch32 when CPU is AArch64-only */ + goto illegal_return; + } + if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) { goto illegal_return; } -- 2.43.0 I'm down as the only listed maintainer for quite a lot of Arm SoC and board types. In some cases this is only as the "maintainer of last resort" and I'm not in practice doing anything beyond patch review and the odd bit of tidyup. Move these entries in MAINTAINERS from "Maintained" to "Odd Fixes", to better represent reality. Entries for other boards and SoCs where I do more actively care (or where there is a listed co-maintainer) remain as they are. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-id: 20250307152838.3226398-1-peter.maydell@linaro.org --- MAINTAINERS | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ F: docs/system/arm/kzm.rst Integrator CP M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/arm/integratorcp.c F: hw/misc/arm_integrator_debug.c F: include/hw/misc/arm_integrator_debug.h @@ -XXX,XX +XXX,XX @@ F: docs/system/arm/mps2.rst Musca M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/arm/musca.c F: docs/system/arm/musca.rst @@ -XXX,XX +XXX,XX @@ F: tests/functional/test_aarch64_raspi4.py Real View M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/arm/realview* F: hw/cpu/realview_mpcore.c F: hw/intc/realview_gic.c @@ -XXX,XX +XXX,XX @@ F: tests/functional/test_arm_collie.py Stellaris M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/*/stellaris* F: hw/display/ssd03* F: include/hw/input/gamepad.h @@ -XXX,XX +XXX,XX @@ F: docs/system/arm/stm32.rst Versatile Express M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/arm/vexpress.c F: hw/display/sii9022.c F: docs/system/arm/vexpress.rst @@ -XXX,XX +XXX,XX @@ F: tests/functional/test_arm_vexpress.py Versatile PB M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/*/versatile* F: hw/i2c/arm_sbcon_i2c.c F: include/hw/i2c/arm_sbcon_i2c.h @@ -XXX,XX +XXX,XX @@ F: include/hw/hyperv/vmbus*.h OMAP M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org -S: Maintained +S: Odd Fixes F: hw/*/omap* F: include/hw/arm/omap.h F: docs/system/arm/sx1.rst -- 2.43.0 From: Paolo Bonzini <pbonzini@redhat.com> The guest does not control whether characters are sent on the UART. Sending them before the guest happens to boot will now result in a "guest error" log entry that is only because of timing, even if the guest _would_ later setup the receiver correctly. This reverts the bulk of commit abf2b6a028670bd2890bb3aee7e103fe53e4b0df, and instead adds a comment about why we don't check the enable bits. Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-id: 20250311153717.206129-1-pbonzini@redhat.com [PMM: expanded comment] Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/char/pl011.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/char/pl011.c b/hw/char/pl011.c index XXXXXXX..XXXXXXX 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -XXX,XX +XXX,XX @@ static int pl011_can_receive(void *opaque) unsigned fifo_depth = pl011_get_fifo_depth(s); unsigned fifo_available = fifo_depth - s->read_count; - if (!(s->cr & CR_UARTEN)) { - qemu_log_mask(LOG_GUEST_ERROR, - "PL011 receiving data on disabled UART\n"); - } - if (!(s->cr & CR_RXE)) { - qemu_log_mask(LOG_GUEST_ERROR, - "PL011 receiving data on disabled RX UART\n"); - } - trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available); + /* + * In theory we should check the UART and RX enable bits here and + * return 0 if they are not set (so the guest can't receive data + * until you have enabled the UART). In practice we suspect there + * is at least some guest code out there which has been tested only + * on QEMU and which never bothers to enable the UART because we + * historically never enforced that. So we effectively keep the + * UART continuously enabled regardless of the enable bits. + */ + trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available); return fifo_available; } -- 2.43.0 From: Joe Komlodi <komlodi@google.com> On ARM hosts with CTR_EL0.DIC and CTR_EL0.IDC set, this would only cause an ISB to be executed during cache maintenance, which could lead to QEMU executing TBs containing garbage instructions. This seems to be because the ISB finishes executing instructions and flushes the pipeline, but the ISB doesn't guarantee that writes from the executed instructions are committed. If a small enough TB is created, it's possible that the writes setting up the TB aren't committed by the time the TB is executed. This function is intended to be a port of the gcc implementation (https://github.com/gcc-mirror/gcc/blob/85b46d0795ac76bc192cb8f88b646a647acf98c1/libgcc/config/aarch64/sync-cache.c#L67) which makes the first DSB unconditional, so we can fix the synchronization issue by doing that as well. Cc: qemu-stable@nongnu.org Fixes: 664a79735e4deb1 ("util: Specialize flush_idcache_range for aarch64") Signed-off-by: Joe Komlodi <komlodi@google.com> Message-id: 20250310203622.1827940-2-komlodi@google.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- util/cacheflush.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/util/cacheflush.c b/util/cacheflush.c index XXXXXXX..XXXXXXX 100644 --- a/util/cacheflush.c +++ b/util/cacheflush.c @@ -XXX,XX +XXX,XX @@ void flush_idcache_range(uintptr_t rx, uintptr_t rw, size_t len) for (p = rw & -dcache_lsize; p < rw + len; p += dcache_lsize) { asm volatile("dc\tcvau, %0" : : "r" (p) : "memory"); } - asm volatile("dsb\tish" : : : "memory"); } + /* DSB unconditionally to ensure any outstanding writes are committed. */ + asm volatile("dsb\tish" : : : "memory"); + /* * If CTR_EL0.DIC is enabled, Instruction cache cleaning to the Point * of Unification is not required for instruction to data coherence. -- 2.43.0 From: Richard Henderson <richard.henderson@linaro.org> The check for fp_excp_el in assert_fp_access_checked is incorrect. For SME, with StreamingMode enabled, the access is really against the streaming mode vectors, and access to the normal fp registers is allowed to be disabled. C.f. sme_enabled_check. Convert sve_access_checked to match, even though we don't currently check the exception state. Cc: qemu-stable@nongnu.org Fixes: 3d74825f4d6 ("target/arm: Add SME enablement checks") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20250307190415.982049-2-richard.henderson@linaro.org Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/translate-a64.h | 2 +- target/arm/tcg/translate.h | 10 +++++++--- target/arm/tcg/translate-a64.c | 17 +++++++++-------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/target/arm/tcg/translate-a64.h b/target/arm/tcg/translate-a64.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/translate-a64.h +++ b/target/arm/tcg/translate-a64.h @@ -XXX,XX +XXX,XX @@ TCGv_i64 gen_mte_checkN(DisasContext *s, TCGv_i64 addr, bool is_write, static inline void assert_fp_access_checked(DisasContext *s) { #ifdef CONFIG_DEBUG_TCG - if (unlikely(!s->fp_access_checked || s->fp_excp_el)) { + if (unlikely(s->fp_access_checked <= 0)) { fprintf(stderr, "target-arm: FP access check missing for " "instruction 0x%08x\n", s->insn); abort(); diff --git a/target/arm/tcg/translate.h b/target/arm/tcg/translate.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/translate.h +++ b/target/arm/tcg/translate.h @@ -XXX,XX +XXX,XX @@ typedef struct DisasContext { bool aarch64; bool thumb; bool lse2; - /* Because unallocated encodings generate different exception syndrome + /* + * Because unallocated encodings generate different exception syndrome * information from traps due to FP being disabled, we can't do a single * "is fp access disabled" check at a high level in the decode tree. * To help in catching bugs where the access check was forgotten in some * code path, we set this flag when the access check is done, and assert * that it is set at the point where we actually touch the FP regs. + * 0: not checked, + * 1: checked, access ok + * -1: checked, access denied */ - bool fp_access_checked; - bool sve_access_checked; + int8_t fp_access_checked; + int8_t sve_access_checked; /* ARMv8 single-step state (this is distinct from the QEMU gdbstub * single-step support). */ diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/translate-a64.c +++ b/target/arm/tcg/translate-a64.c @@ -XXX,XX +XXX,XX @@ static bool fp_access_check_only(DisasContext *s) { if (s->fp_excp_el) { assert(!s->fp_access_checked); - s->fp_access_checked = true; + s->fp_access_checked = -1; gen_exception_insn_el(s, 0, EXCP_UDEF, syn_fp_access_trap(1, 0xe, false, 0), s->fp_excp_el); return false; } - s->fp_access_checked = true; + s->fp_access_checked = 1; return true; } @@ -XXX,XX +XXX,XX @@ bool sve_access_check(DisasContext *s) syn_sve_access_trap(), s->sve_excp_el); goto fail_exit; } - s->sve_access_checked = true; + s->sve_access_checked = 1; return fp_access_check(s); fail_exit: /* Assert that we only raise one exception per instruction. */ assert(!s->sve_access_checked); - s->sve_access_checked = true; + s->sve_access_checked = -1; return false; } @@ -XXX,XX +XXX,XX @@ bool sme_enabled_check(DisasContext *s) * sme_excp_el by itself for cpregs access checks. */ if (!s->fp_excp_el || s->sme_excp_el < s->fp_excp_el) { - s->fp_access_checked = true; - return sme_access_check(s); + bool ret = sme_access_check(s); + s->fp_access_checked = (ret ? 1 : -1); + return ret; } return fp_access_check_only(s); } @@ -XXX,XX +XXX,XX @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) s->insn = insn; s->base.pc_next = pc + 4; - s->fp_access_checked = false; - s->sve_access_checked = false; + s->fp_access_checked = 0; + s->sve_access_checked = 0; if (s->pstate_il) { /* -- 2.43.0 From: Richard Henderson <richard.henderson@linaro.org> In StreamingMode, fp_access_checked is handled already. We cannot fall through to fp_access_check lest we fall foul of the double-check assertion. Cc: qemu-stable@nongnu.org Fixes: 285b1d5fcef ("target/arm: Handle SME in sve_access_check") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20250307190415.982049-3-richard.henderson@linaro.org Reviewed-by: Peter Maydell <peter.maydell@linaro.org> [PMM: move declaration of 'ret' to top of block] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/translate-a64.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/tcg/translate-a64.c +++ b/target/arm/tcg/translate-a64.c @@ -XXX,XX +XXX,XX @@ static int fp_access_check_vector_hsd(DisasContext *s, bool is_q, MemOp esz) bool sve_access_check(DisasContext *s) { if (s->pstate_sm || !dc_isar_feature(aa64_sve, s)) { + bool ret; + assert(dc_isar_feature(aa64_sme, s)); - if (!sme_sm_enabled_check(s)) { - goto fail_exit; - } - } else if (s->sve_excp_el) { + ret = sme_sm_enabled_check(s); + s->sve_access_checked = (ret ? 1 : -1); + return ret; + } + if (s->sve_excp_el) { + /* Assert that we only raise one exception per instruction. */ + assert(!s->sve_access_checked); gen_exception_insn_el(s, 0, EXCP_UDEF, syn_sve_access_trap(), s->sve_excp_el); - goto fail_exit; + s->sve_access_checked = -1; + return false; } s->sve_access_checked = 1; return fp_access_check(s); - - fail_exit: - /* Assert that we only raise one exception per instruction. */ - assert(!s->sve_access_checked); - s->sve_access_checked = -1; - return false; } /* -- 2.43.0 We want to capture potential Rust backtraces on panics in our test logs, which isn't Rust's default behaviour. Set RUST_BACKTRACE=1 in the add_test_setup environments, so that all our tests get run with this environment variable set. This makes the setting of that variable in the gitlab CI template redundant, so we can remove it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-id: 20250310102950.3752908-1-peter.maydell@linaro.org --- meson.build | 9 ++++++--- .gitlab-ci.d/buildtest-template.yml | 1 - 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index XXXXXXX..XXXXXXX 100644 --- a/meson.build +++ b/meson.build @@ -XXX,XX +XXX,XX @@ project('qemu', ['c'], meson_version: '>=1.5.0', meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() }) -add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true) -add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow']) -add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough']) +add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true, + env: ['RUST_BACKTRACE=1']) +add_test_setup('slow', exclude_suites: ['thorough'], + env: ['G_TEST_SLOW=1', 'SPEED=slow', 'RUST_BACKTRACE=1']) +add_test_setup('thorough', + env: ['G_TEST_SLOW=1', 'SPEED=thorough', 'RUST_BACKTRACE=1']) meson.add_postconf_script(find_program('scripts/symlink-install-tree.py')) diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml index XXXXXXX..XXXXXXX 100644 --- a/.gitlab-ci.d/buildtest-template.yml +++ b/.gitlab-ci.d/buildtest-template.yml @@ -XXX,XX +XXX,XX @@ stage: test image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG script: - - export RUST_BACKTRACE=1 - source scripts/ci/gitlab-ci-section - section_start buildenv "Setting up to run tests" - scripts/git-submodule.sh update roms/SLOF -- 2.43.0