From 11a3c4a286d5dc603582ea0a1fca62c2ec0a1aee Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 20 Nov 2023 15:01:21 +0000 Subject: [PATCH 01/13] target/arm: Set IL bit for pauth, SVE access, BTI trap syndromes The syndrome register value always has an IL field at bit 25, which is 0 for a trap on a 16 bit instruction, and 1 for a trap on a 32 bit instruction (or for exceptions which aren't traps on a known instruction, like PC alignment faults). This means that our syn_*() functions should always either take an is_16bit argument to determine whether to set the IL bit, or else unconditionally set it. We missed setting the IL bit for the syndrome for three kinds of trap: * an SVE access exception * a pointer authentication check failure * a BTI (branch target identification) check failure All of these traps are AArch64 only, and so the instruction causing the trap is always 64 bit. This means we can unconditionally set the IL bit in the syn_*() function. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20231120150121.3458408-1-peter.maydell@linaro.org Cc: qemu-stable@nongnu.org Reviewed-by: Peter Maydell --- target/arm/syndrome.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h index 5d34755508..95454b5b3b 100644 --- a/target/arm/syndrome.h +++ b/target/arm/syndrome.h @@ -216,7 +216,7 @@ static inline uint32_t syn_simd_access_trap(int cv, int cond, bool is_16bit) static inline uint32_t syn_sve_access_trap(void) { - return EC_SVEACCESSTRAP << ARM_EL_EC_SHIFT; + return (EC_SVEACCESSTRAP << ARM_EL_EC_SHIFT) | ARM_EL_IL; } /* @@ -242,12 +242,12 @@ static inline uint32_t syn_pacfail(bool data, int keynumber) static inline uint32_t syn_pactrap(void) { - return EC_PACTRAP << ARM_EL_EC_SHIFT; + return (EC_PACTRAP << ARM_EL_EC_SHIFT) | ARM_EL_IL; } static inline uint32_t syn_btitrap(int btype) { - return (EC_BTITRAP << ARM_EL_EC_SHIFT) | btype; + return (EC_BTITRAP << ARM_EL_EC_SHIFT) | ARM_EL_IL | btype; } static inline uint32_t syn_bxjtrap(int cv, int cond, int rm) From 8d37a1425b9954d7e445615dcad23456515e24c0 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 20 Nov 2023 17:35:06 +0000 Subject: [PATCH 02/13] target/arm: Handle overflow in calculation of next timer tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In commit edac4d8a168 back in 2015 when we added support for the virtual timer offset CNTVOFF_EL2, we didn't correctly update the timer-recalculation code that figures out when the timer interrupt is next going to change state. We got it wrong in two ways: * for the 0->1 transition, we didn't notice that gt->cval + offset can overflow a uint64_t * for the 1->0 transition, we didn't notice that the transition might now happen before the count rolls over, if offset > count In the former case, we end up trying to set the next interrupt for a time in the past, which results in QEMU hanging as the timer fires continuously. In the latter case, we would fail to update the interrupt status when we are supposed to. Fix the calculations in both cases. The test case is Alex Bennée's from the bug report, and tests the 0->1 transition overflow case. Fixes: edac4d8a168 ("target-arm: Add CNTVOFF_EL2") Cc: qemu-stable@nongnu.org Resolves: https://gitlab.com/qemu-project/qemu/-/issues/60 Signed-off-by: Alex Bennée Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20231120173506.3729884-1-peter.maydell@linaro.org Reviewed-by: Peter Maydell --- target/arm/helper.c | 25 ++++++++++-- tests/tcg/aarch64/Makefile.softmmu-target | 7 +++- tests/tcg/aarch64/system/vtimer.c | 48 +++++++++++++++++++++++ 3 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 tests/tcg/aarch64/system/vtimer.c diff --git a/target/arm/helper.c b/target/arm/helper.c index ff1970981e..2746d3fdac 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2646,11 +2646,28 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) gt->ctl = deposit32(gt->ctl, 2, 1, istatus); if (istatus) { - /* Next transition is when count rolls back over to zero */ - nexttick = UINT64_MAX; + /* + * Next transition is when (count - offset) rolls back over to 0. + * If offset > count then this is when count == offset; + * if offset <= count then this is when count == offset + 2^64 + * For the latter case we set nexttick to an "as far in future + * as possible" value and let the code below handle it. + */ + if (offset > count) { + nexttick = offset; + } else { + nexttick = UINT64_MAX; + } } else { - /* Next transition is when we hit cval */ - nexttick = gt->cval + offset; + /* + * Next transition is when (count - offset) == cval, i.e. + * when count == (cval + offset). + * If that would overflow, then again we set up the next interrupt + * for "as far in the future as possible" for the code below. + */ + if (uadd64_overflow(gt->cval, offset, &nexttick)) { + nexttick = UINT64_MAX; + } } /* * Note that the desired next expiry time might be beyond the diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target index 77c5018e02..4b03ef602e 100644 --- a/tests/tcg/aarch64/Makefile.softmmu-target +++ b/tests/tcg/aarch64/Makefile.softmmu-target @@ -45,7 +45,8 @@ TESTS+=memory-sve # Running QEMU_BASE_MACHINE=-M virt -cpu max -display none -QEMU_OPTS+=$(QEMU_BASE_MACHINE) -semihosting-config enable=on,target=native,chardev=output -kernel +QEMU_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output +QEMU_OPTS+=$(QEMU_BASE_MACHINE) $(QEMU_BASE_ARGS) -kernel # console test is manual only QEMU_SEMIHOST=-serial none -chardev stdio,mux=on,id=stdio0 -semihosting-config enable=on,chardev=stdio0 -mon chardev=stdio0,mode=readline @@ -56,6 +57,10 @@ run-semiconsole: semiconsole run-plugin-semiconsole-with-%: semiconsole $(call skip-test, $<, "MANUAL ONLY") +# vtimer test needs EL2 +QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4 +run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel + # Simple Record/Replay Test .PHONY: memory-record run-memory-record: memory-record memory diff --git a/tests/tcg/aarch64/system/vtimer.c b/tests/tcg/aarch64/system/vtimer.c new file mode 100644 index 0000000000..42f2f7796c --- /dev/null +++ b/tests/tcg/aarch64/system/vtimer.c @@ -0,0 +1,48 @@ +/* + * Simple Virtual Timer Test + * + * Copyright (c) 2020 Linaro Ltd + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include +#include + +/* grabbed from Linux */ +#define __stringify_1(x...) #x +#define __stringify(x...) __stringify_1(x) + +#define read_sysreg(r) ({ \ + uint64_t __val; \ + asm volatile("mrs %0, " __stringify(r) : "=r" (__val)); \ + __val; \ +}) + +#define write_sysreg(r, v) do { \ + uint64_t __val = (uint64_t)(v); \ + asm volatile("msr " __stringify(r) ", %x0" \ + : : "rZ" (__val)); \ +} while (0) + +int main(void) +{ + int i; + + ml_printf("VTimer Test\n"); + + write_sysreg(cntvoff_el2, 1); + write_sysreg(cntv_cval_el0, -1); + write_sysreg(cntv_ctl_el0, 1); + + ml_printf("cntvoff_el2=%lx\n", read_sysreg(cntvoff_el2)); + ml_printf("cntv_cval_el0=%lx\n", read_sysreg(cntv_cval_el0)); + ml_printf("cntv_ctl_el0=%lx\n", read_sysreg(cntv_ctl_el0)); + + /* Now read cval a few times */ + for (i = 0; i < 10; i++) { + ml_printf("%d: cntv_cval_el0=%lx\n", i, read_sysreg(cntv_cval_el0)); + } + + return 0; +} From 75d0e6b5c6deb08dd6cc184adba3668055680e7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 24 Nov 2023 19:33:24 +0100 Subject: [PATCH 03/13] hw/net/can/xlnx-zynqmp: Avoid underflow while popping TX FIFOs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Message-Format Message Format The same message format is used for RXFIFO, TXFIFO, and TXHPB. Each message includes four words (16 bytes). Software must read and write all four words regardless of the actual number of data bytes and valid fields in the message. There is no mention in this reference manual about what the hardware does when not all four words are written. To fix the reported underflow behavior when DATA2 register is written, I choose to fill the data with the previous content of the ID / DLC / DATA1 registers, which is how I expect hardware would do. Note there is no hardware flag raised under such condition. Reported-by: Qiang Liu Reviewed-by: Francisco Iglesias Reviewed-by: Vikram Garhwal Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231124183325.95392-2-philmd@linaro.org Fixes: 98e5d7a2b7 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1425 Reviewed-by: Francisco Iglesias Reviewed-by: Vikram Garhwal Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Peter Maydell Reviewed-by: Peter Maydell --- hw/net/can/xlnx-zynqmp-can.c | 50 +++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c index e93e6c5e19..1f1c686479 100644 --- a/hw/net/can/xlnx-zynqmp-can.c +++ b/hw/net/can/xlnx-zynqmp-can.c @@ -434,6 +434,52 @@ static bool tx_ready_check(XlnxZynqMPCANState *s) return true; } +static void read_tx_frame(XlnxZynqMPCANState *s, Fifo32 *fifo, uint32_t *data) +{ + unsigned used = fifo32_num_used(fifo); + bool is_txhpb = fifo == &s->txhpb_fifo; + + assert(used > 0); + used %= CAN_FRAME_SIZE; + + /* + * Frame Message Format + * + * Each frame includes four words (16 bytes). Software must read and write + * all four words regardless of the actual number of data bytes and valid + * fields in the message. + * If software misbehave (not writing all four words), we use the previous + * registers content to initialize each missing word. + * + * If used is 1 then ID, DLC and DATA1 are missing. + * if used is 2 then ID and DLC are missing. + * if used is 3 then only ID is missing. + */ + if (used > 0) { + data[0] = s->regs[is_txhpb ? R_TXHPB_ID : R_TXFIFO_ID]; + } else { + data[0] = fifo32_pop(fifo); + } + if (used == 1 || used == 2) { + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC]; + } else { + data[1] = fifo32_pop(fifo); + } + if (used == 1) { + data[2] = s->regs[is_txhpb ? R_TXHPB_DATA1 : R_TXFIFO_DATA1]; + } else { + data[2] = fifo32_pop(fifo); + } + /* DATA2 triggered the transfer thus is always available */ + data[3] = fifo32_pop(fifo); + + if (used) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Incomplete CAN frame (only %u/%u slots used)\n", + TYPE_XLNX_ZYNQMP_CAN, used, CAN_FRAME_SIZE); + } +} + static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo) { qemu_can_frame frame; @@ -451,9 +497,7 @@ static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo) } while (!fifo32_is_empty(fifo)) { - for (i = 0; i < CAN_FRAME_SIZE; i++) { - data[i] = fifo32_pop(fifo); - } + read_tx_frame(s, fifo, data); if (ARRAY_FIELD_EX32(s->regs, STATUS_REGISTER, LBACK)) { /* From 8729856c1904c11a9b016cba600767f814e237b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 24 Nov 2023 19:33:25 +0100 Subject: [PATCH 04/13] hw/net/can/xlnx-zynqmp: Avoid underflow while popping RX FIFO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Message-Format Message Format The same message format is used for RXFIFO, TXFIFO, and TXHPB. Each message includes four words (16 bytes). Software must read and write all four words regardless of the actual number of data bytes and valid fields in the message. There is no mention in this reference manual about what the hardware does when not all four words are read. To fix the reported underflow behavior, I choose to fill the 4 frame data registers when the first register (ID) is accessed, which is how I expect hardware would do. Reported-by: Qiang Liu Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Francisco Iglesias Reviewed-by: Vikram Garhwal Message-id: 20231124183325.95392-3-philmd@linaro.org Fixes: 98e5d7a2b7 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1427 Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Francisco Iglesias Reviewed-by: Vikram Garhwal Signed-off-by: Peter Maydell Reviewed-by: Peter Maydell --- hw/net/can/xlnx-zynqmp-can.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c index 1f1c686479..f60e480c3a 100644 --- a/hw/net/can/xlnx-zynqmp-can.c +++ b/hw/net/can/xlnx-zynqmp-can.c @@ -778,14 +778,18 @@ static void update_rx_fifo(XlnxZynqMPCANState *s, const qemu_can_frame *frame) } } -static uint64_t can_rxfifo_pre_read(RegisterInfo *reg, uint64_t val) +static uint64_t can_rxfifo_post_read_id(RegisterInfo *reg, uint64_t val) { XlnxZynqMPCANState *s = XLNX_ZYNQMP_CAN(reg->opaque); + unsigned used = fifo32_num_used(&s->rx_fifo); - if (!fifo32_is_empty(&s->rx_fifo)) { - val = fifo32_pop(&s->rx_fifo); - } else { + if (used < CAN_FRAME_SIZE) { ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXUFLW, 1); + } else { + val = s->regs[R_RXFIFO_ID] = fifo32_pop(&s->rx_fifo); + s->regs[R_RXFIFO_DLC] = fifo32_pop(&s->rx_fifo); + s->regs[R_RXFIFO_DATA1] = fifo32_pop(&s->rx_fifo); + s->regs[R_RXFIFO_DATA2] = fifo32_pop(&s->rx_fifo); } can_update_irq(s); @@ -946,14 +950,11 @@ static const RegisterAccessInfo can_regs_info[] = { .post_write = can_tx_post_write, },{ .name = "RXFIFO_ID", .addr = A_RXFIFO_ID, .ro = 0xffffffff, - .post_read = can_rxfifo_pre_read, + .post_read = can_rxfifo_post_read_id, },{ .name = "RXFIFO_DLC", .addr = A_RXFIFO_DLC, .rsvd = 0xfff0000, - .post_read = can_rxfifo_pre_read, },{ .name = "RXFIFO_DATA1", .addr = A_RXFIFO_DATA1, - .post_read = can_rxfifo_pre_read, },{ .name = "RXFIFO_DATA2", .addr = A_RXFIFO_DATA2, - .post_read = can_rxfifo_pre_read, },{ .name = "AFR", .addr = A_AFR, .rsvd = 0xfffffff0, .post_write = can_filter_enable_post_write, From 837053a7f491b445088eac647abe7f462c50f59a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:46 +0100 Subject: [PATCH 05/13] hw/virtio: Add VirtioPCIDeviceTypeInfo::instance_finalize field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The VirtioPCIDeviceTypeInfo structure, added in commit a4ee4c8baa ("virtio: Helper for registering virtio device types") got extended in commit 8ea90ee690 ("virtio: add class_size") with the @class_size field. Do similarly with the @instance_finalize field. Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231121174051.63038-2-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/virtio/virtio-pci.c | 1 + include/hw/virtio/virtio-pci.h | 1 + 2 files changed, 2 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 205dbf24fb..e433879542 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2391,6 +2391,7 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t) .parent = t->parent ? t->parent : TYPE_VIRTIO_PCI, .instance_size = t->instance_size, .instance_init = t->instance_init, + .instance_finalize = t->instance_finalize, .class_size = t->class_size, .abstract = true, .interfaces = t->interfaces, diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h index 5a3f182f99..59d88018c1 100644 --- a/include/hw/virtio/virtio-pci.h +++ b/include/hw/virtio/virtio-pci.h @@ -246,6 +246,7 @@ typedef struct VirtioPCIDeviceTypeInfo { size_t instance_size; size_t class_size; void (*instance_init)(Object *obj); + void (*instance_finalize)(Object *obj); void (*class_init)(ObjectClass *klass, void *data); InterfaceInfo *interfaces; } VirtioPCIDeviceTypeInfo; From c9a4aa06dfce0fde1e279e1ea0c1945582ec0d16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:47 +0100 Subject: [PATCH 06/13] hw/virtio: Free VirtIOIOMMUPCI::vdev.reserved_regions[] on finalize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 0be6bfac62 ("qdev: Implement variable length array properties") added the DEFINE_PROP_ARRAY() macro with the following comment: * It is the responsibility of the device deinit code to free the * @_arrayfield memory. Commit 8077b8e549 added: DEFINE_PROP_ARRAY("reserved-regions", VirtIOIOMMUPCI, vdev.nb_reserved_regions, vdev.reserved_regions, qdev_prop_reserved_region, ReservedRegion), but forgot to free the 'vdev.reserved_regions' array. Do it in the instance_finalize() handler. Cc: qemu-stable@nongnu.org Fixes: 8077b8e549 ("virtio-iommu-pci: Add array of Interval properties") # v5.1.0+ Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Eric Auger Message-id: 20231121174051.63038-3-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/virtio/virtio-iommu-pci.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c index 9459fbf6ed..cbdfe4c591 100644 --- a/hw/virtio/virtio-iommu-pci.c +++ b/hw/virtio/virtio-iommu-pci.c @@ -95,10 +95,18 @@ static void virtio_iommu_pci_instance_init(Object *obj) TYPE_VIRTIO_IOMMU); } +static void virtio_iommu_pci_instance_finalize(Object *obj) +{ + VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(obj); + + g_free(dev->vdev.prop_resv_regions); +} + static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = { .generic_name = TYPE_VIRTIO_IOMMU_PCI, .instance_size = sizeof(VirtIOIOMMUPCI), .instance_init = virtio_iommu_pci_instance_init, + .instance_finalize = virtio_iommu_pci_instance_finalize, .class_init = virtio_iommu_pci_class_init, }; From 896dd6ff7b9f2575f1a908a07f26a70b58d8b675 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:48 +0100 Subject: [PATCH 07/13] hw/misc/mps2-scc: Free MPS2SCC::oscclk[] array on finalize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 0be6bfac62 ("qdev: Implement variable length array properties") added the DEFINE_PROP_ARRAY() macro with the following comment: * It is the responsibility of the device deinit code to free the * @_arrayfield memory. Commit 4fb013afcc added: DEFINE_PROP_ARRAY("oscclk", MPS2SCC, num_oscclk, oscclk_reset, qdev_prop_uint32, uint32_t), but forgot to free the 'oscclk_reset' array. Do it in the instance_finalize() handler. Cc: qemu-stable@nongnu.org Fixes: 4fb013afcc ("hw/misc/mps2-scc: Support configurable number of OSCCLK values") # v6.0.0+ Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231121174051.63038-4-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/misc/mps2-scc.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c index b3b42a792c..fe5034db14 100644 --- a/hw/misc/mps2-scc.c +++ b/hw/misc/mps2-scc.c @@ -329,6 +329,13 @@ static void mps2_scc_realize(DeviceState *dev, Error **errp) s->oscclk = g_new0(uint32_t, s->num_oscclk); } +static void mps2_scc_finalize(Object *obj) +{ + MPS2SCC *s = MPS2_SCC(obj); + + g_free(s->oscclk_reset); +} + static const VMStateDescription mps2_scc_vmstate = { .name = "mps2-scc", .version_id = 3, @@ -385,6 +392,7 @@ static const TypeInfo mps2_scc_info = { .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(MPS2SCC), .instance_init = mps2_scc_init, + .instance_finalize = mps2_scc_finalize, .class_init = mps2_scc_class_init, }; From 49b3e28b7bdfe771150d05c4b5860aa7854a4232 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:49 +0100 Subject: [PATCH 08/13] hw/nvram/xlnx-efuse: Free XlnxEFuse::ro_bits[] array on finalize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 0be6bfac62 ("qdev: Implement variable length array properties") added the DEFINE_PROP_ARRAY() macro with the following comment: * It is the responsibility of the device deinit code to free the * @_arrayfield memory. Commit 68fbcc344e added: DEFINE_PROP_ARRAY("read-only", XlnxEFuse, ro_bits_cnt, ro_bits, qdev_prop_uint32, uint32_t), but forgot to free the 'ro_bits' array. Do it in the instance_finalize handler. Cc: qemu-stable@nongnu.org Fixes: 68fbcc344e ("hw/nvram: Introduce Xilinx eFuse QOM") # v6.2.0+ Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231121174051.63038-5-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/nvram/xlnx-efuse.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/nvram/xlnx-efuse.c b/hw/nvram/xlnx-efuse.c index 655c40b8d1..f7b849f7de 100644 --- a/hw/nvram/xlnx-efuse.c +++ b/hw/nvram/xlnx-efuse.c @@ -224,6 +224,13 @@ static void efuse_realize(DeviceState *dev, Error **errp) } } +static void efuse_finalize(Object *obj) +{ + XlnxEFuse *s = XLNX_EFUSE(obj); + + g_free(s->ro_bits); +} + static void efuse_prop_set_drive(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -280,6 +287,7 @@ static const TypeInfo efuse_info = { .name = TYPE_XLNX_EFUSE, .parent = TYPE_DEVICE, .instance_size = sizeof(XlnxEFuse), + .instance_finalize = efuse_finalize, .class_init = efuse_class_init, }; From 4f10c66077e39969940d928077560665e155cac8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:50 +0100 Subject: [PATCH 09/13] hw/nvram/xlnx-efuse-ctrl: Free XlnxVersalEFuseCtrl[] "pg0-lock" array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 0be6bfac62 ("qdev: Implement variable length array properties") added the DEFINE_PROP_ARRAY() macro with the following comment: * It is the responsibility of the device deinit code to free the * @_arrayfield memory. Commit 9e4aa1fafe added: DEFINE_PROP_ARRAY("pg0-lock", XlnxVersalEFuseCtrl, extra_pg0_lock_n16, extra_pg0_lock_spec, qdev_prop_uint16, uint16_t), but forgot to free the 'extra_pg0_lock_spec' array. Do it in the instance_finalize() handler. Cc: qemu-stable@nongnu.org Fixes: 9e4aa1fafe ("hw/nvram: Xilinx Versal eFuse device") # v6.2.0+ Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231121174051.63038-6-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/nvram/xlnx-versal-efuse-ctrl.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/nvram/xlnx-versal-efuse-ctrl.c b/hw/nvram/xlnx-versal-efuse-ctrl.c index beb5661c35..2480af35e1 100644 --- a/hw/nvram/xlnx-versal-efuse-ctrl.c +++ b/hw/nvram/xlnx-versal-efuse-ctrl.c @@ -726,6 +726,13 @@ static void efuse_ctrl_init(Object *obj) sysbus_init_irq(sbd, &s->irq_efuse_imr); } +static void efuse_ctrl_finalize(Object *obj) +{ + XlnxVersalEFuseCtrl *s = XLNX_VERSAL_EFUSE_CTRL(obj); + + g_free(s->extra_pg0_lock_spec); +} + static const VMStateDescription vmstate_efuse_ctrl = { .name = TYPE_XLNX_VERSAL_EFUSE_CTRL, .version_id = 1, @@ -764,6 +771,7 @@ static const TypeInfo efuse_ctrl_info = { .instance_size = sizeof(XlnxVersalEFuseCtrl), .class_init = efuse_ctrl_class_init, .instance_init = efuse_ctrl_init, + .instance_finalize = efuse_ctrl_finalize, }; static void efuse_ctrl_register_types(void) From 6e782ffd555808378f69dd606641f0c4b5ca6120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 21 Nov 2023 18:40:51 +0100 Subject: [PATCH 10/13] hw/input/stellaris_gamepad: Free StellarisGamepad::keycodes[] array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 0be6bfac62 ("qdev: Implement variable length array properties") added the DEFINE_PROP_ARRAY() macro with the following comment: * It is the responsibility of the device deinit code to free the * @_arrayfield memory. Commit a75f336b97 added: DEFINE_PROP_ARRAY("keycodes", StellarisGamepad, num_buttons, keycodes, qdev_prop_uint32, uint32_t), but forgot to free the 'keycodes' array. Do it in the instance_finalize handler. Fixes: a75f336b97 ("hw/input/stellaris_input: Convert to qdev") Signed-off-by: Philippe Mathieu-Daudé Message-id: 20231121174051.63038-7-philmd@linaro.org Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/input/stellaris_gamepad.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c index 06a0c0ce83..9dfa620e29 100644 --- a/hw/input/stellaris_gamepad.c +++ b/hw/input/stellaris_gamepad.c @@ -63,6 +63,13 @@ static void stellaris_gamepad_realize(DeviceState *dev, Error **errp) qemu_input_handler_register(dev, &stellaris_gamepad_handler); } +static void stellaris_gamepad_finalize(Object *obj) +{ + StellarisGamepad *s = STELLARIS_GAMEPAD(obj); + + g_free(s->keycodes); +} + static void stellaris_gamepad_reset_enter(Object *obj, ResetType type) { StellarisGamepad *s = STELLARIS_GAMEPAD(obj); @@ -92,6 +99,7 @@ static const TypeInfo stellaris_gamepad_info[] = { .name = TYPE_STELLARIS_GAMEPAD, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(StellarisGamepad), + .instance_finalize = stellaris_gamepad_finalize, .class_init = stellaris_gamepad_class_init, }, }; From 90bb6d676489b5cc063858ece263e1586795803f Mon Sep 17 00:00:00 2001 From: Frederic Konrad Date: Fri, 24 Nov 2023 14:35:03 +0000 Subject: [PATCH 11/13] hw/ssi/xilinx_spips: fix an out of bound access The spips, qspips, and zynqmp-qspips share the same realize function (xilinx_spips_realize) and initialize their io memory region with different mmio_ops passed through the class. The size of the memory region is set to the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write out of s->regs[addr] in xilinx_spips_write for spips and qspips. This fixes that wrong behavior. Reviewed-by: Luc Michel Signed-off-by: Frederic Konrad Reviewed-by: Francisco Iglesias Message-id: 20231124143505.1493184-2-fkonrad@amd.com Signed-off-by: Peter Maydell --- hw/ssi/xilinx_spips.c | 7 ++++++- include/hw/ssi/xilinx_spips.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c index a3955c6c50..0bdfad7e2e 100644 --- a/hw/ssi/xilinx_spips.c +++ b/hw/ssi/xilinx_spips.c @@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr, DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value); addr >>= 2; + assert(addr < XLNX_SPIPS_R_MAX); + switch (addr) { case R_CONFIG: mask = ~(R_CONFIG_RSVD | MAN_START_COM); @@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp) } memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s, - "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4); + "spi", xsc->reg_size); sysbus_init_mmio(sbd, &s->iomem); s->irqline = -1; @@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data) dc->realize = xilinx_qspips_realize; xsc->reg_ops = &qspips_ops; + xsc->reg_size = XLNX_SPIPS_R_MAX * 4; xsc->rx_fifo_size = RXFF_A_Q; xsc->tx_fifo_size = TXFF_A_Q; } @@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_xilinx_spips; xsc->reg_ops = &spips_ops; + xsc->reg_size = XLNX_SPIPS_R_MAX * 4; xsc->rx_fifo_size = RXFF_A; xsc->tx_fifo_size = TXFF_A; } @@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data) dc->vmsd = &vmstate_xlnx_zynqmp_qspips; device_class_set_props(dc, xilinx_zynqmp_qspips_properties); xsc->reg_ops = &xlnx_zynqmp_qspips_ops; + xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4; xsc->rx_fifo_size = RXFF_A_Q; xsc->tx_fifo_size = TXFF_A_Q; } diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h index 1386d5ac8f..7a754bf67a 100644 --- a/include/hw/ssi/xilinx_spips.h +++ b/include/hw/ssi/xilinx_spips.h @@ -33,7 +33,9 @@ typedef struct XilinxSPIPS XilinxSPIPS; +/* For SPIPS, QSPIPS. */ #define XLNX_SPIPS_R_MAX (0x100 / 4) +/* For ZYNQMP_QSPIPS. */ #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4) /* Bite off 4k chunks at a time */ @@ -125,6 +127,7 @@ struct XilinxSPIPSClass { SysBusDeviceClass parent_class; const MemoryRegionOps *reg_ops; + uint64_t reg_size; uint32_t rx_fifo_size; uint32_t tx_fifo_size; From a9bc470ec208bd27a82100abc9dccf1b69f41b45 Mon Sep 17 00:00:00 2001 From: Frederic Konrad Date: Fri, 24 Nov 2023 14:35:04 +0000 Subject: [PATCH 12/13] hw/misc, hw/ssi: Fix some URLs for AMD / Xilinx models It seems that the url changed a bit, and it triggers an error. Fix the URLs so the documentation can be reached again. Signed-off-by: Frederic Konrad Reviewed-by: Francisco Iglesias Message-id: 20231124143505.1493184-3-fkonrad@amd.com Signed-off-by: Peter Maydell --- hw/dma/xlnx_csu_dma.c | 2 +- include/hw/misc/xlnx-versal-cframe-reg.h | 2 +- include/hw/misc/xlnx-versal-cfu.h | 2 +- include/hw/misc/xlnx-versal-pmc-iou-slcr.h | 2 +- include/hw/ssi/xlnx-versal-ospi.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c index e89089821a..531013f35a 100644 --- a/hw/dma/xlnx_csu_dma.c +++ b/hw/dma/xlnx_csu_dma.c @@ -33,7 +33,7 @@ /* * Ref: UG1087 (v1.7) February 8, 2019 - * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html + * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers * CSUDMA Module section */ REG32(ADDR, 0x0) diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h index a14fbd7fe4..0091505246 100644 --- a/include/hw/misc/xlnx-versal-cframe-reg.h +++ b/include/hw/misc/xlnx-versal-cframe-reg.h @@ -12,7 +12,7 @@ * https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf * * [2] Versal ACAP Register Reference, - * https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html + * https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFRAME_REG-Module */ #ifndef HW_MISC_XLNX_VERSAL_CFRAME_REG_H #define HW_MISC_XLNX_VERSAL_CFRAME_REG_H diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h index 86fb841053..be62bab8c8 100644 --- a/include/hw/misc/xlnx-versal-cfu.h +++ b/include/hw/misc/xlnx-versal-cfu.h @@ -12,7 +12,7 @@ * https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf * * [2] Versal ACAP Register Reference, - * https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html + * https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFU_CSR-Module */ #ifndef HW_MISC_XLNX_VERSAL_CFU_APB_H #define HW_MISC_XLNX_VERSAL_CFU_APB_H diff --git a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h index f7d24c93c4..0c4a4fd66d 100644 --- a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h +++ b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h @@ -34,7 +34,7 @@ * https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf * * [2] Versal ACAP Register Reference, - * https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___pmc_iop_slcr.html + * https://docs.xilinx.com/r/en-US/am012-versal-register-reference/PMC_IOP_SLCR-Module * * QEMU interface: * + sysbus MMIO region 0: MemoryRegion for the device's registers diff --git a/include/hw/ssi/xlnx-versal-ospi.h b/include/hw/ssi/xlnx-versal-ospi.h index 5d131d351d..4ac975aa2f 100644 --- a/include/hw/ssi/xlnx-versal-ospi.h +++ b/include/hw/ssi/xlnx-versal-ospi.h @@ -34,7 +34,7 @@ * https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf * * [2] Versal ACAP Register Reference, - * https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___ospi.html + * https://docs.xilinx.com/r/en-US/am012-versal-register-reference/OSPI-Module * * * QEMU interface: From 1ee80592bf24eabef77e2260a86d9358b54c08fd Mon Sep 17 00:00:00 2001 From: Frederic Konrad Date: Fri, 24 Nov 2023 14:35:05 +0000 Subject: [PATCH 13/13] hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC DMA UG1087 states for the source channel that: if SIZE is programmed to 0, and the DMA is started, the interrupts DONE and MEM_DONE will be asserted. This implies that it is allowed for the guest to stop the source DMA by writing a size of 0 to the SIZE register, so remove the LOG_GUEST_ERROR in that case. While at it remove the comment marking the SIZE register as write-only. See: https://docs.xilinx.com/r/en-US/ug1087-zynq-ultrascale-registers/CSUDMA_SRC_SIZE-CSUDMA-Register Signed-off-by: Frederic Konrad Reviewed-by: Francisco Iglesias Message-id: 20231124143505.1493184-4-fkonrad@amd.com Signed-off-by: Peter Maydell --- hw/dma/xlnx_csu_dma.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c index 531013f35a..bc1505aade 100644 --- a/hw/dma/xlnx_csu_dma.c +++ b/hw/dma/xlnx_csu_dma.c @@ -39,7 +39,7 @@ REG32(ADDR, 0x0) FIELD(ADDR, ADDR, 2, 30) /* wo */ REG32(SIZE, 0x4) - FIELD(SIZE, SIZE, 2, 27) /* wo */ + FIELD(SIZE, SIZE, 2, 27) FIELD(SIZE, LAST_WORD, 0, 1) /* rw, only exists in SRC */ REG32(STATUS, 0x8) FIELD(STATUS, DONE_CNT, 13, 3) /* wtc */ @@ -335,10 +335,14 @@ static uint64_t addr_pre_write(RegisterInfo *reg, uint64_t val) static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val) { XlnxCSUDMA *s = XLNX_CSU_DMA(reg->opaque); + uint64_t size = val & R_SIZE_SIZE_MASK; if (s->regs[R_SIZE] != 0) { - qemu_log_mask(LOG_GUEST_ERROR, - "%s: Starting DMA while already running.\n", __func__); + if (size || s->is_dst) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Starting DMA while already running.\n", + __func__); + } } if (!s->is_dst) { @@ -346,7 +350,7 @@ static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val) } /* Size is word aligned */ - return val & R_SIZE_SIZE_MASK; + return size; } static uint64_t size_post_read(RegisterInfo *reg, uint64_t val)