Ten arm-related bug fixes for 2.12... thanks -- PMM The following changes since commit 4c2c1015905fa1d616750dfe024b4c0b35875950: Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20180323' into staging (2018-03-23 10:20:54 +0000) are available in the Git repository at: git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20180323 for you to fetch changes up to 548f514cf89dd9ab39c0cb4c063097bccf141fdd: target/arm: Always set FAR to a known unknown value for debug exceptions (2018-03-23 18:26:46 +0000) ---------------------------------------------------------------- target-arm queue: * arm/translate-a64: don't lose interrupts after unmasking via write to DAIF * sdhci: fix incorrect use of Error * * hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses * hw/arm/bcm2836: Use the Cortex-A7 instead of Cortex-A15 * i.MX: Support serial RS-232 break properly * mach-virt: Set VM's SMBIOS system version to mc->name * target/arm: Honour MDCR_EL2.TDE when routing exceptions due to BKPT/BRK * target/arm: Factor out code to calculate FSR for debug exceptions * target/arm: Set FSR for BKPT, BRK when raising exception * target/arm: Always set FAR to a known unknown value for debug exceptions ---------------------------------------------------------------- Paolo Bonzini (1): sdhci: fix incorrect use of Error * Peter Maydell (6): hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses hw/arm/bcm2836: Use the Cortex-A7 instead of Cortex-A15 target/arm: Honour MDCR_EL2.TDE when routing exceptions due to BKPT/BRK target/arm: Factor out code to calculate FSR for debug exceptions target/arm: Set FSR for BKPT, BRK when raising exception target/arm: Always set FAR to a known unknown value for debug exceptions Trent Piepho (1): i.MX: Support serial RS-232 break properly Victor Kamensky (1): arm/translate-a64: treat DISAS_UPDATE as variant of DISAS_EXIT Wei Huang (1): mach-virt: Set VM's SMBIOS system version to mc->name include/hw/arm/virt.h | 1 + include/hw/char/imx_serial.h | 1 + target/arm/helper.h | 1 + target/arm/internals.h | 25 +++++++++++++++++++++++++ hw/arm/bcm2836.c | 2 +- hw/arm/raspi.c | 2 +- hw/arm/virt.c | 8 +++++++- hw/char/imx_serial.c | 5 ++++- hw/intc/arm_gicv3_cpuif.c | 6 +++--- hw/sd/sdhci.c | 4 ++-- target/arm/helper.c | 1 - target/arm/op_helper.c | 33 ++++++++++++++++++++++----------- target/arm/translate-a64.c | 21 ++++++++++++++++----- target/arm/translate.c | 19 ++++++++++++++----- 14 files changed, 98 insertions(+), 31 deletions(-) From: Victor Kamensky <kamensky@cisco.com> In OE project 4.15 linux kernel boot hang was observed under single cpu aarch64 qemu. Kernel code was in a loop waiting for vtimer arrival, spinning in TC generated blocks, while interrupt was pending unprocessed. This happened because when qemu tried to handle vtimer interrupt target had interrupts disabled, as result flag indicating TCG exit, cpu->icount_decr.u16.high, was cleared but arm_cpu_exec_interrupt function did not call arm_cpu_do_interrupt to process interrupt. Later when target reenabled interrupts, it happened without exit into main loop, so following code that waited for result of interrupt execution run in infinite loop. To solve the problem instructions that operate on CPU sys state (i.e enable/disable interrupt), and marked as DISAS_UPDATE, should be considered as DISAS_EXIT variant, and should be forced to exit back to main loop so qemu will have a chance processing pending CPU state updates, including pending interrupts. This change brings consistency with how DISAS_UPDATE is treated in aarch32 case. CC: Peter Maydell <peter.maydell@linaro.org> CC: Alex Bennée <alex.bennee@linaro.org> CC: qemu-stable@nongnu.org Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Victor Kamensky <kamensky@cisco.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 1521526368-1996-1-git-send-email-kamensky@cisco.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/translate-a64.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -XXX,XX +XXX,XX @@ static void aarch64_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu) case DISAS_UPDATE: gen_a64_set_pc_im(dc->pc); /* fall through */ - case DISAS_JUMP: - tcg_gen_lookup_and_goto_ptr(); - break; case DISAS_EXIT: tcg_gen_exit_tb(0); break; + case DISAS_JUMP: + tcg_gen_lookup_and_goto_ptr(); + break; case DISAS_NORETURN: case DISAS_SWI: break; -- 2.16.2 From: Paolo Bonzini <pbonzini@redhat.com> Detected by Coverity (CID 1386072, 1386073, 1386076, 1386077). local_err was unused, and this made the static analyzer unhappy. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-id: 20180320151355.25854-1-pbonzini@redhat.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/sd/sdhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -XXX,XX +XXX,XX @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp) Error *local_err = NULL; sdhci_initfn(s); - sdhci_common_realize(s, errp); + sdhci_common_realize(s, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -XXX,XX +XXX,XX @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp) SysBusDevice *sbd = SYS_BUS_DEVICE(dev); Error *local_err = NULL; - sdhci_common_realize(s, errp); + sdhci_common_realize(s, &local_err); if (local_err) { error_propagate(errp, local_err); return; -- 2.16.2 If the GIC has the security extension support enabled, then a non-secure access to ICC_PMR must take account of the non-secure view of interrupt priorities, where real priorities 0x00..0x7f are secure-only and not visible to the non-secure guest, and priorities 0x80..0xff are shown to the guest as if they were 0x00..0xff. We had the logic here wrong: * on reads, the priority is in the secure range if bit 7 is clear, not if it is set * on writes, we want to set bit 7, not mask everything else Our ICC_RPR read code had the same error as ICC_PMR. (Compare the GICv3 spec pseudocode functions ICC_RPR_EL1 and ICC_PMR_EL1.) Fixes: https://bugs.launchpad.net/qemu/+bug/1748434 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20180315133441.24149-1-peter.maydell@linaro.org --- hw/intc/arm_gicv3_cpuif.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -XXX,XX +XXX,XX @@ static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri) /* NS access and Group 0 is inaccessible to NS: return the * NS view of the current priority */ - if (value & 0x80) { + if ((value & 0x80) == 0) { /* Secure priorities not visible to NS */ value = 0; } else if (value != 0xff) { @@ -XXX,XX +XXX,XX @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, /* Current PMR in the secure range, don't allow NS to change it */ return; } - value = (value >> 1) & 0x80; + value = (value >> 1) | 0x80; } cs->icc_pmr_el1 = value; gicv3_cpuif_update(cs); @@ -XXX,XX +XXX,XX @@ static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri) if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env) && (env->cp15.scr_el3 & SCR_FIQ)) { /* NS GIC access and Group 0 is inaccessible to NS */ - if (prio & 0x80) { + if ((prio & 0x80) == 0) { /* NS mustn't see priorities in the Secure half of the range */ prio = 0; } else if (prio != 0xff) { -- 2.16.2 The BCM2836 uses a Cortex-A7, not a Cortex-A15. Update the device to use the correct CPU. https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf When the BCM2836 was introduced (bad5623690b) the Cortex-A7 was not available, so the very similar Cortex-A15 was used. Since dcf578ed8ce we can model the correct core. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alistair Francis <alistair@alistair23.me> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20180319110215.16755-1-peter.maydell@linaro.org --- hw/arm/bcm2836.c | 2 +- hw/arm/raspi.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -XXX,XX +XXX,XX @@ struct BCM283XInfo { static const BCM283XInfo bcm283x_socs[] = { { .name = TYPE_BCM2836, - .cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"), + .cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"), .clusterid = 0xf, }, #ifdef TARGET_AARCH64 diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -XXX,XX +XXX,XX @@ static void raspi2_machine_init(MachineClass *mc) mc->no_parallel = 1; mc->no_floppy = 1; mc->no_cdrom = 1; - mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"); + mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); mc->max_cpus = BCM283X_NCPUS; mc->min_cpus = BCM283X_NCPUS; mc->default_cpus = BCM283X_NCPUS; -- 2.16.2 From: Trent Piepho <tpiepho@impinj.com> Linux does not detect a break from this IMX serial driver as a magic sysrq. Nor does it note a break in the port error counts. The former is because the Linux driver uses the BRCD bit in the USR2 register to trigger the RS-232 break handler in the kernel, which is where sysrq hooks in. The emulated UART was not setting this status bit. The latter is because the Linux driver expects, in addition to the BRK bit, that the ERR bit is set when a break is read in the FIFO. A break should also count as a frame error, so add that bit too. Cc: Andrey Smirnov <andrew.smirnov@gmail.com> Signed-off-by: Trent Piepho <tpiepho@impinj.com> Message-id: 20180320013657.25038-1-tpiepho@impinj.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/char/imx_serial.h | 1 + hw/char/imx_serial.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/char/imx_serial.h +++ b/include/hw/char/imx_serial.h @@ -XXX,XX +XXX,XX @@ #define URXD_CHARRDY (1<<15) /* character read is valid */ #define URXD_ERR (1<<14) /* Character has error */ +#define URXD_FRMERR (1<<12) /* Character has frame error */ #define URXD_BRK (1<<11) /* Break received */ #define USR1_PARTYER (1<<15) /* Parity Error */ diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c index XXXXXXX..XXXXXXX 100644 --- a/hw/char/imx_serial.c +++ b/hw/char/imx_serial.c @@ -XXX,XX +XXX,XX @@ static void imx_put_data(void *opaque, uint32_t value) s->usr2 |= USR2_RDR; s->uts1 &= ~UTS1_RXEMPTY; s->readbuff = value; + if (value & URXD_BRK) { + s->usr2 |= USR2_BRCD; + } imx_update(s); } @@ -XXX,XX +XXX,XX @@ static void imx_receive(void *opaque, const uint8_t *buf, int size) static void imx_event(void *opaque, int event) { if (event == CHR_EVENT_BREAK) { - imx_put_data(opaque, URXD_BRK); + imx_put_data(opaque, URXD_BRK | URXD_FRMERR | URXD_ERR); } } -- 2.16.2 From: Wei Huang <wei@redhat.com> Instead of using "1.0" as the system version of SMBIOS, we should use mc->name for mach-virt machine type to be consistent other architectures. With this patch, "dmidecode -t 1" (e.g., "-M virt-2.12,accel=kvm") will show: Handle 0x0100, DMI type 1, 27 bytes System Information Manufacturer: QEMU Product Name: KVM Virtual Machine Version: virt-2.12 Serial Number: Not Specified ... instead of: Handle 0x0100, DMI type 1, 27 bytes System Information Manufacturer: QEMU Product Name: KVM Virtual Machine Version: 1.0 Serial Number: Not Specified ... For backward compatibility, we allow older machine types to keep "1.0" as the default system version. Signed-off-by: Wei Huang <wei@redhat.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20180322212318.7182-1-wei@redhat.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/virt.h | 1 + hw/arm/virt.c | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -XXX,XX +XXX,XX @@ typedef struct { bool no_its; bool no_pmu; bool claim_edge_triggered_timers; + bool smbios_old_sys_ver; } VirtMachineClass; typedef struct { 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 void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) static void virt_build_smbios(VirtMachineState *vms) { + MachineClass *mc = MACHINE_GET_CLASS(vms); + VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); uint8_t *smbios_tables, *smbios_anchor; size_t smbios_tables_len, smbios_anchor_len; const char *product = "QEMU Virtual Machine"; @@ -XXX,XX +XXX,XX @@ static void virt_build_smbios(VirtMachineState *vms) } smbios_set_defaults("QEMU", product, - "1.0", false, true, SMBIOS_ENTRY_POINT_30); + vmc->smbios_old_sys_ver ? "1.0" : mc->name, false, + true, SMBIOS_ENTRY_POINT_30); smbios_get_tables(NULL, 0, &smbios_tables, &smbios_tables_len, &smbios_anchor, &smbios_anchor_len); @@ -XXX,XX +XXX,XX @@ static void virt_2_11_instance_init(Object *obj) static void virt_machine_2_11_options(MachineClass *mc) { + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); + virt_machine_2_12_options(mc); SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11); + vmc->smbios_old_sys_ver = true; } DEFINE_VIRT_MACHINE(2, 11) -- 2.16.2 The MDCR_EL2.TDE bit allows the exception level targeted by debug exceptions to be set to EL2 for code executing at EL0. We handle this in the arm_debug_target_el() function, but this is only used for hardware breakpoint and watchpoint exceptions, not for the exception generated when the guest executes an AArch32 BKPT or AArch64 BRK instruction. We don't have enough information for a translate-time equivalent of arm_debug_target_el(), so instead make BKPT and BRK call a special purpose helper which can do the routing, rather than the generic exception_with_syndrome helper. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20180320134114.30418-2-peter.maydell@linaro.org --- target/arm/helper.h | 1 + target/arm/op_helper.c | 8 ++++++++ target/arm/translate-a64.c | 15 +++++++++++++-- target/arm/translate.c | 19 ++++++++++++++----- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/target/arm/helper.h b/target/arm/helper.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/helper.h +++ b/target/arm/helper.h @@ -XXX,XX +XXX,XX @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32) DEF_HELPER_2(exception_internal, void, env, i32) DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32) +DEF_HELPER_2(exception_bkpt_insn, void, env, i32) DEF_HELPER_1(setend, void, env) DEF_HELPER_2(wfi, void, env, i32) DEF_HELPER_1(wfe, void, env) diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -XXX,XX +XXX,XX @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp, raise_exception(env, excp, syndrome, target_el); } +/* Raise an EXCP_BKPT with the specified syndrome register value, + * targeting the correct exception level for debug exceptions. + */ +void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome) +{ + raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env)); +} + uint32_t HELPER(cpsr_read)(CPUARMState *env) { return cpsr_read(env) & ~(CPSR_EXEC | CPSR_RESERVED); diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -XXX,XX +XXX,XX @@ static void gen_exception_insn(DisasContext *s, int offset, int excp, s->base.is_jmp = DISAS_NORETURN; } +static void gen_exception_bkpt_insn(DisasContext *s, int offset, + uint32_t syndrome) +{ + TCGv_i32 tcg_syn; + + gen_a64_set_pc_im(s->pc - offset); + tcg_syn = tcg_const_i32(syndrome); + gen_helper_exception_bkpt_insn(cpu_env, tcg_syn); + tcg_temp_free_i32(tcg_syn); + s->base.is_jmp = DISAS_NORETURN; +} + static void gen_ss_advance(DisasContext *s) { /* If the singlestep state is Active-not-pending, advance to @@ -XXX,XX +XXX,XX @@ static void disas_exc(DisasContext *s, uint32_t insn) break; } /* BRK */ - gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16), - default_exception_el(s)); + gen_exception_bkpt_insn(s, 4, syn_aa64_bkpt(imm16)); break; case 2: if (op2_ll != 0) { diff --git a/target/arm/translate.c b/target/arm/translate.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -XXX,XX +XXX,XX @@ static void gen_exception_insn(DisasContext *s, int offset, int excp, s->base.is_jmp = DISAS_NORETURN; } +static void gen_exception_bkpt_insn(DisasContext *s, int offset, uint32_t syn) +{ + TCGv_i32 tcg_syn; + + gen_set_condexec(s); + gen_set_pc_im(s, s->pc - offset); + tcg_syn = tcg_const_i32(syn); + gen_helper_exception_bkpt_insn(cpu_env, tcg_syn); + tcg_temp_free_i32(tcg_syn); + s->base.is_jmp = DISAS_NORETURN; +} + /* Force a TB lookup after an instruction that changes the CPU state. */ static inline void gen_lookup_tb(DisasContext *s) { @@ -XXX,XX +XXX,XX @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) case 1: /* bkpt */ ARCH(5); - gen_exception_insn(s, 4, EXCP_BKPT, - syn_aa32_bkpt(imm16, false), - default_exception_el(s)); + gen_exception_bkpt_insn(s, 4, syn_aa32_bkpt(imm16, false)); break; case 2: /* Hypervisor call (v7) */ @@ -XXX,XX +XXX,XX @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn) { int imm8 = extract32(insn, 0, 8); ARCH(5); - gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true), - default_exception_el(s)); + gen_exception_bkpt_insn(s, 2, syn_aa32_bkpt(imm8, true)); break; } -- 2.16.2 When a debug exception is taken to AArch32, it appears as a Prefetch Abort, and the Instruction Fault Status Register (IFSR) must be set. The IFSR has two possible formats, depending on whether LPAE is in use. Factor out the code in arm_debug_excp_handler() which picks an FSR value into its own utility function, update it to use arm_fi_to_lfsc() and arm_fi_to_sfsc() rather than hard-coded constants, and use the correct condition to select long or short format. In particular this fixes a bug where we could select the short format because we're at EL0 and the EL1 translation regime is not using LPAE, but then route the debug exception to EL2 because of MDCR_EL2.TDE and hand EL2 the wrong format FSR. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20180320134114.30418-3-peter.maydell@linaro.org --- target/arm/internals.h | 25 +++++++++++++++++++++++++ target/arm/op_helper.c | 12 ++---------- 2 files changed, 27 insertions(+), 10 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 bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx) } } +/* Return the FSR value for a debug exception (watchpoint, hardware + * breakpoint or BKPT insn) targeting the specified exception level. + */ +static inline uint32_t arm_debug_exception_fsr(CPUARMState *env) +{ + ARMMMUFaultInfo fi = { .type = ARMFault_Debug }; + int target_el = arm_debug_target_el(env); + bool using_lpae = false; + + if (target_el == 2 || arm_el_is_aa64(env, target_el)) { + using_lpae = true; + } else { + if (arm_feature(env, ARM_FEATURE_LPAE) && + (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) { + using_lpae = true; + } + } + + if (using_lpae) { + return arm_fi_to_lfsc(&fi); + } else { + return arm_fi_to_sfsc(&fi); + } +} + #endif diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -XXX,XX +XXX,XX @@ void arm_debug_excp_handler(CPUState *cs) cs->watchpoint_hit = NULL; - if (extended_addresses_enabled(env)) { - env->exception.fsr = (1 << 9) | 0x22; - } else { - env->exception.fsr = 0x2; - } + env->exception.fsr = arm_debug_exception_fsr(env); env->exception.vaddress = wp_hit->hitaddr; raise_exception(env, EXCP_DATA_ABORT, syn_watchpoint(same_el, 0, wnr), @@ -XXX,XX +XXX,XX @@ void arm_debug_excp_handler(CPUState *cs) return; } - if (extended_addresses_enabled(env)) { - env->exception.fsr = (1 << 9) | 0x22; - } else { - env->exception.fsr = 0x2; - } + env->exception.fsr = arm_debug_exception_fsr(env); /* FAR is UNKNOWN, so doesn't need setting */ raise_exception(env, EXCP_PREFETCH_ABORT, syn_breakpoint(same_el), -- 2.16.2 Now that we have a helper function specifically for the BRK and BKPT instructions, we can set the exception.fsr there rather than in arm_cpu_do_interrupt_aarch32(). This allows us to use our new arm_debug_exception_fsr() helper. In particular this fixes a bug where we were hardcoding the short-form IFSR value, which is wrong if the target exception level has LPAE enabled. Fixes: https://bugs.launchpad.net/qemu/+bug/1756927 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20180320134114.30418-4-peter.maydell@linaro.org --- target/arm/helper.c | 1 - target/arm/op_helper.c | 2 ++ 2 files changed, 2 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 void arm_cpu_do_interrupt_aarch32(CPUState *cs) offset = 0; break; case EXCP_BKPT: - env->exception.fsr = 2; /* Fall through to prefetch abort. */ case EXCP_PREFETCH_ABORT: A32_BANKED_CURRENT_REG_SET(env, ifsr, env->exception.fsr); diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -XXX,XX +XXX,XX @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp, */ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome) { + /* FSR will only be used if the debug target EL is AArch32. */ + env->exception.fsr = arm_debug_exception_fsr(env); raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env)); } -- 2.16.2 For debug exceptions due to breakpoints or the BKPT instruction which are taken to AArch32, the Fault Address Register is architecturally UNKNOWN. We were using that as license to simply not set env->exception.vaddress, but this isn't correct, because it will expose to the guest whatever old value was in that field when arm_cpu_do_interrupt_aarch32() writes it to the guest IFSR. That old value might be a FAR for a previous guest EL2 or secure exception, in which case we shouldn't show it to an EL1 or non-secure exception handler. It might also be a non-deterministic value, which is bad for record-and-replay. Clear env->exception.vaddress before taking breakpoint debug exceptions, to avoid this minor information leak. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20180320134114.30418-5-peter.maydell@linaro.org --- target/arm/op_helper.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -XXX,XX +XXX,XX @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome) { /* FSR will only be used if the debug target EL is AArch32. */ env->exception.fsr = arm_debug_exception_fsr(env); + /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing + * values to the guest that it shouldn't be able to see at its + * exception/security level. + */ + env->exception.vaddress = 0; raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env)); } @@ -XXX,XX +XXX,XX @@ void arm_debug_excp_handler(CPUState *cs) } env->exception.fsr = arm_debug_exception_fsr(env); - /* FAR is UNKNOWN, so doesn't need setting */ + /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing + * values to the guest that it shouldn't be able to see at its + * exception/security level. + */ + env->exception.vaddress = 0; raise_exception(env, EXCP_PREFETCH_ABORT, syn_breakpoint(same_el), arm_debug_target_el(env)); -- 2.16.2 The following changes since commit b10d00d8811fa4eed4862963273d7353ce310c82: Merge remote-tracking branch 'remotes/kraxel/tags/seabios-20220118-pull-request' into staging (2022-01-19 18:46:28 +0000) are available in the Git repository at: https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20220120 for you to fetch changes up to 9705e3c1dcff96b0b3c7e594b6cd68d27d6c4ced: hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR (2022-01-20 11:47:54 +0000) ---------------------------------------------------------------- target-arm: * hw/intc/arm_gicv3_its: Fix various minor bugs * hw/arm/aspeed: Add the i3c device to the AST2600 SoC * hw/arm: kudo: add lm75s behind bus 1 switch at 75 * hw/arm/virt: Fix support for running guests on hosts with restricted IPA ranges * hw/intc/arm_gic: Allow reset of the running priority * hw/intc/arm_gic: Implement read of GICC_IIDR * hw/arm/virt: Support for virtio-mem-pci * hw/arm/virt: Support CPU cluster on ARM virt machine * docs/can: convert to restructuredText * hw/net: Move MV88W8618 network device out of hw/arm/ directory * hw/arm/virt: KVM: Enable PAuth when supported by the host ---------------------------------------------------------------- Gavin Shan (2): virtio-mem: Correct default THP size for ARM64 hw/arm/virt: Support for virtio-mem-pci Lucas Ramage (1): docs/can: convert to restructuredText Marc Zyngier (7): hw/arm/virt: KVM: Enable PAuth when supported by the host hw/arm/virt: Add a control for the the highmem PCIe MMIO hw/arm/virt: Add a control for the the highmem redistributors hw/arm/virt: Honor highmem setting when computing the memory map hw/arm/virt: Use the PA range to compute the memory map hw/arm/virt: Disable highmem devices that don't fit in the PA range hw/arm/virt: Drop superfluous checks against highmem Patrick Venture (1): hw/arm: kudo add lm75s behind bus 1 switch at 75 Peter Maydell (13): hw/intc/arm_gicv3_its: Fix event ID bounds checks hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value hw/intc/arm_gicv3_its: Don't use data if reading command failed hw/intc/arm_gicv3_its: Use enum for return value of process_* functions hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd() hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting hw/intc/arm_gicv3_its: Fix return codes in process_mapti() hw/intc/arm_gicv3_its: Fix return codes in process_mapc() hw/intc/arm_gicv3_its: Fix return codes in process_mapd() hw/intc/arm_gicv3_its: Factor out "find address of table entry" code hw/intc/arm_gicv3_its: Check indexes before use, not after hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table Petr Pavlu (2): hw/intc/arm_gic: Implement read of GICC_IIDR hw/intc/arm_gic: Allow reset of the running priority Philippe Mathieu-Daudé (4): hw: Move MARVELL_88W8618 Kconfig from audio/ to arm/ hw/arm/musicpal: Fix coding style of code related to MV88W8618 device hw/net: Move MV88W8618 network device out of hw/arm/ directory hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR Troy Lee (2): hw/misc/aspeed_i3c.c: Introduce a dummy AST2600 I3C model. hw/arm/aspeed: Add the i3c device to the AST2600 SoC Yanan Wang (6): hw/arm/virt: Support CPU cluster on ARM virt machine hw/arm/virt: Support cluster level in DT cpu-map hw/acpi/aml-build: Improve scalability of PPTT generation tests/acpi/bios-tables-test: Allow changes to virt/PPTT file hw/acpi/aml-build: Support cluster level in PPTT generation tests/acpi/bios-table-test: Update expected virt/PPTT file docs/system/arm/cpu-features.rst | 4 - docs/system/device-emulation.rst | 1 + docs/{can.txt => system/devices/can.rst} | 90 +++--- include/hw/arm/aspeed_soc.h | 3 + include/hw/arm/virt.h | 5 +- include/hw/misc/aspeed_i3c.h | 48 +++ include/hw/net/mv88w8618_eth.h | 12 + target/arm/cpu.h | 1 + hw/acpi/aml-build.c | 68 +++-- hw/arm/aspeed_ast2600.c | 16 + hw/arm/musicpal.c | 381 +----------------------- hw/arm/npcm7xx_boards.c | 10 +- hw/arm/virt-acpi-build.c | 10 +- hw/arm/virt.c | 184 ++++++++++-- hw/intc/arm_gic.c | 11 + hw/intc/arm_gicv3_its.c | 492 ++++++++++++++----------------- hw/intc/arm_gicv3_redist.c | 4 +- hw/misc/aspeed_i3c.c | 381 ++++++++++++++++++++++++ hw/net/mv88w8618_eth.c | 403 +++++++++++++++++++++++++ hw/virtio/virtio-mem.c | 36 ++- target/arm/cpu.c | 16 +- target/arm/cpu64.c | 31 +- target/arm/kvm64.c | 21 ++ MAINTAINERS | 2 + hw/arm/Kconfig | 4 + hw/audio/Kconfig | 3 - hw/misc/meson.build | 1 + hw/misc/trace-events | 6 + hw/net/meson.build | 1 + qemu-options.hx | 10 + tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes 31 files changed, 1473 insertions(+), 782 deletions(-) rename docs/{can.txt => system/devices/can.rst} (68%) create mode 100644 include/hw/misc/aspeed_i3c.h create mode 100644 include/hw/net/mv88w8618_eth.h create mode 100644 hw/misc/aspeed_i3c.c create mode 100644 hw/net/mv88w8618_eth.c From: Marc Zyngier <maz@kernel.org> Add basic support for Pointer Authentication when running a KVM guest and that the host supports it, loosely based on the SVE support. Although the feature is enabled by default when the host advertises it, it is possible to disable it by setting the 'pauth=off' CPU property. The 'pauth' comment is removed from cpu-features.rst, as it is now common to both TCG and KVM. Tested on an Apple M1 running 5.16-rc6. Cc: Eric Auger <eric.auger@redhat.com> Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220107150154.2490308-1-maz@kernel.org [PMM: fixed indentation] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/system/arm/cpu-features.rst | 4 ---- target/arm/cpu.h | 1 + target/arm/cpu.c | 16 +++++----------- target/arm/cpu64.c | 31 +++++++++++++++++++++++++++---- target/arm/kvm64.c | 21 +++++++++++++++++++++ 5 files changed, 54 insertions(+), 19 deletions(-) diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/system/arm/cpu-features.rst +++ b/docs/system/arm/cpu-features.rst @@ -XXX,XX +XXX,XX @@ TCG VCPU Features TCG VCPU features are CPU features that are specific to TCG. Below is the list of TCG VCPU features and their descriptions. - pauth Enable or disable ``FEAT_Pauth``, pointer - authentication. By default, the feature is - enabled with ``-cpu max``. - pauth-impdef When ``FEAT_Pauth`` is enabled, either the *impdef* (Implementation Defined) algorithm is enabled or the *architected* QARMA algorithm 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 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq); void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el, bool el0_a64); void aarch64_add_sve_properties(Object *obj); +void aarch64_add_pauth_properties(Object *obj); /* * SVE registers are encoded in KVM's memory in an endianness-invariant format. 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 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp) return; } - /* - * KVM does not support modifications to this feature. - * We have not registered the cpu properties when KVM - * is in use, so the user will not be able to set them. - */ - if (!kvm_enabled()) { - arm_cpu_pauth_finalize(cpu, &local_err); - if (local_err != NULL) { - error_propagate(errp, local_err); - return; - } + arm_cpu_pauth_finalize(cpu, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + return; } } @@ -XXX,XX +XXX,XX @@ static void arm_host_initfn(Object *obj) kvm_arm_set_cpu_features_from_host(cpu); if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { aarch64_add_sve_properties(obj); + aarch64_add_pauth_properties(obj); } #else hvf_arm_set_cpu_features_from_host(cpu); diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -XXX,XX +XXX,XX @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) int arch_val = 0, impdef_val = 0; uint64_t t; + /* Exit early if PAuth is enabled, and fall through to disable it */ + if (kvm_enabled() && cpu->prop_pauth) { + if (!cpu_isar_feature(aa64_pauth, cpu)) { + error_setg(errp, "'pauth' feature not supported by KVM on this host"); + } + + return; + } + /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */ if (cpu->prop_pauth) { if (cpu->prop_pauth_impdef) { @@ -XXX,XX +XXX,XX @@ static Property arm_cpu_pauth_property = static Property arm_cpu_pauth_impdef_property = DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false); +void aarch64_add_pauth_properties(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + + /* Default to PAUTH on, with the architected algorithm on TCG. */ + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property); + if (kvm_enabled()) { + /* + * Mirror PAuth support from the probed sysregs back into the + * property for KVM. Is it just a bit backward? Yes it is! + */ + cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu); + } else { + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property); + } +} + /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host); * otherwise, a CPU with as many features enabled as our emulation supports. * The version of '-cpu max' for qemu-system-arm is defined in cpu.c; @@ -XXX,XX +XXX,XX @@ static void aarch64_max_initfn(Object *obj) cpu->dcz_blocksize = 7; /* 512 bytes */ #endif - /* Default to PAUTH on, with the architected algorithm. */ - qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property); - qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property); - bitmap_fill(cpu->sve_vq_supported, ARM_MAX_VQ); } + aarch64_add_pauth_properties(obj); aarch64_add_sve_properties(obj); object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, cpu_max_set_sve_max_vq, NULL, NULL); diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index XXXXXXX..XXXXXXX 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -XXX,XX +XXX,XX @@ static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id) return ioctl(fd, KVM_GET_ONE_REG, &idreg); } +static bool kvm_arm_pauth_supported(void) +{ + return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) && + kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC)); +} + bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) { /* Identify the feature bits corresponding to the host CPU, and @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) */ struct kvm_vcpu_init init = { .target = -1, }; + /* + * Ask for Pointer Authentication if supported. We can't play the + * SVE trick of synthesising the ID reg as KVM won't tell us + * whether we have the architected or IMPDEF version of PAuth, so + * we have to use the actual ID regs. + */ + if (kvm_arm_pauth_supported()) { + init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | + 1 << KVM_ARM_VCPU_PTRAUTH_GENERIC); + } + if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { return false; } @@ -XXX,XX +XXX,XX @@ int kvm_arch_init_vcpu(CPUState *cs) assert(kvm_arm_sve_supported()); cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE; } + if (cpu_isar_feature(aa64_pauth, cpu)) { + cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | + 1 << KVM_ARM_VCPU_PTRAUTH_GENERIC); + } /* Do KVM_ARM_VCPU_INIT ioctl */ ret = kvm_arm_vcpu_init(cs); -- 2.25.1 From: Philippe Mathieu-Daudé <f4bug@amsat.org> The Marvell 88W8618 is a system-on-chip with an ARM core. We implement its audio codecs and network interface. Homogeneous SoC Kconfig are usually defined in the hw/$ARCH directory. Move it there. Suggested-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20220107184429.423572-2-f4bug@amsat.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/Kconfig | 3 +++ hw/audio/Kconfig | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -XXX,XX +XXX,XX @@ config MUSCA select SPLIT_IRQ select UNIMP +config MARVELL_88W8618 + bool + config MUSICPAL bool select OR_IRQ diff --git a/hw/audio/Kconfig b/hw/audio/Kconfig index XXXXXXX..XXXXXXX 100644 --- a/hw/audio/Kconfig +++ b/hw/audio/Kconfig @@ -XXX,XX +XXX,XX @@ config PL041 config CS4231 bool - -config MARVELL_88W8618 - bool -- 2.25.1 From: Philippe Mathieu-Daudé <f4bug@amsat.org> We are going to move this code, so fix its style first to avoid: ERROR: spaces required around that '/' (ctx:VxV) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20220107184429.423572-3-f4bug@amsat.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/musicpal.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/musicpal.c +++ b/hw/arm/musicpal.c @@ -XXX,XX +XXX,XX @@ static uint64_t mv88w8618_eth_read(void *opaque, hwaddr offset, return s->imr; case MP_ETH_FRDP0 ... MP_ETH_FRDP3: - return s->frx_queue[(offset - MP_ETH_FRDP0)/4]; + return s->frx_queue[(offset - MP_ETH_FRDP0) / 4]; case MP_ETH_CRDP0 ... MP_ETH_CRDP3: - return s->rx_queue[(offset - MP_ETH_CRDP0)/4]; + return s->rx_queue[(offset - MP_ETH_CRDP0) / 4]; case MP_ETH_CTDP0 ... MP_ETH_CTDP1: - return s->tx_queue[(offset - MP_ETH_CTDP0)/4]; + return s->tx_queue[(offset - MP_ETH_CTDP0) / 4]; default: return 0; @@ -XXX,XX +XXX,XX @@ static void mv88w8618_eth_write(void *opaque, hwaddr offset, break; case MP_ETH_FRDP0 ... MP_ETH_FRDP3: - s->frx_queue[(offset - MP_ETH_FRDP0)/4] = value; + s->frx_queue[(offset - MP_ETH_FRDP0) / 4] = value; break; case MP_ETH_CRDP0 ... MP_ETH_CRDP3: - s->rx_queue[(offset - MP_ETH_CRDP0)/4] = - s->cur_rx[(offset - MP_ETH_CRDP0)/4] = value; + s->rx_queue[(offset - MP_ETH_CRDP0) / 4] = + s->cur_rx[(offset - MP_ETH_CRDP0) / 4] = value; break; case MP_ETH_CTDP0 ... MP_ETH_CTDP1: - s->tx_queue[(offset - MP_ETH_CTDP0)/4] = value; + s->tx_queue[(offset - MP_ETH_CTDP0) / 4] = value; break; } } -- 2.25.1 From: Philippe Mathieu-Daudé <f4bug@amsat.org> The Marvell 88W8618 network device is hidden in the Musicpal machine. Move it into a new unit file under the hw/net/ directory. Acked-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20220107184429.423572-4-f4bug@amsat.org Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/net/mv88w8618_eth.h | 12 + hw/arm/musicpal.c | 381 +------------------------------ hw/net/mv88w8618_eth.c | 403 +++++++++++++++++++++++++++++++++ MAINTAINERS | 2 + hw/net/meson.build | 1 + 5 files changed, 419 insertions(+), 380 deletions(-) create mode 100644 include/hw/net/mv88w8618_eth.h create mode 100644 hw/net/mv88w8618_eth.c diff --git a/include/hw/net/mv88w8618_eth.h b/include/hw/net/mv88w8618_eth.h new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/include/hw/net/mv88w8618_eth.h @@ -XXX,XX +XXX,XX @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Marvell MV88W8618 / Freecom MusicPal emulation. + * + * Copyright (c) 2008-2021 QEMU contributors + */ +#ifndef HW_NET_MV88W8618_H +#define HW_NET_MV88W8618_H + +#define TYPE_MV88W8618_ETH "mv88w8618_eth" + +#endif diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/musicpal.c +++ b/hw/arm/musicpal.c @@ -XXX,XX +XXX,XX @@ #include "ui/pixel_ops.h" #include "qemu/cutils.h" #include "qom/object.h" +#include "hw/net/mv88w8618_eth.h" #define MP_MISC_BASE 0x80002000 #define MP_MISC_SIZE 0x00001000 #define MP_ETH_BASE 0x80008000 -#define MP_ETH_SIZE 0x00001000 #define MP_WLAN_BASE 0x8000C000 #define MP_WLAN_SIZE 0x00000800 @@ -XXX,XX +XXX,XX @@ /* Wolfson 8750 I2C address */ #define MP_WM_ADDR 0x1A -/* Ethernet register offsets */ -#define MP_ETH_SMIR 0x010 -#define MP_ETH_PCXR 0x408 -#define MP_ETH_SDCMR 0x448 -#define MP_ETH_ICR 0x450 -#define MP_ETH_IMR 0x458 -#define MP_ETH_FRDP0 0x480 -#define MP_ETH_FRDP1 0x484 -#define MP_ETH_FRDP2 0x488 -#define MP_ETH_FRDP3 0x48C -#define MP_ETH_CRDP0 0x4A0 -#define MP_ETH_CRDP1 0x4A4 -#define MP_ETH_CRDP2 0x4A8 -#define MP_ETH_CRDP3 0x4AC -#define MP_ETH_CTDP0 0x4E0 -#define MP_ETH_CTDP1 0x4E4 - -/* MII PHY access */ -#define MP_ETH_SMIR_DATA 0x0000FFFF -#define MP_ETH_SMIR_ADDR 0x03FF0000 -#define MP_ETH_SMIR_OPCODE (1 << 26) /* Read value */ -#define MP_ETH_SMIR_RDVALID (1 << 27) - -/* PHY registers */ -#define MP_ETH_PHY1_BMSR 0x00210000 -#define MP_ETH_PHY1_PHYSID1 0x00410000 -#define MP_ETH_PHY1_PHYSID2 0x00610000 - -#define MP_PHY_BMSR_LINK 0x0004 -#define MP_PHY_BMSR_AUTONEG 0x0008 - -#define MP_PHY_88E3015 0x01410E20 - -/* TX descriptor status */ -#define MP_ETH_TX_OWN (1U << 31) - -/* RX descriptor status */ -#define MP_ETH_RX_OWN (1U << 31) - -/* Interrupt cause/mask bits */ -#define MP_ETH_IRQ_RX_BIT 0 -#define MP_ETH_IRQ_RX (1 << MP_ETH_IRQ_RX_BIT) -#define MP_ETH_IRQ_TXHI_BIT 2 -#define MP_ETH_IRQ_TXLO_BIT 3 - -/* Port config bits */ -#define MP_ETH_PCXR_2BSM_BIT 28 /* 2-byte incoming suffix */ - -/* SDMA command bits */ -#define MP_ETH_CMD_TXHI (1 << 23) -#define MP_ETH_CMD_TXLO (1 << 22) - -typedef struct mv88w8618_tx_desc { - uint32_t cmdstat; - uint16_t res; - uint16_t bytes; - uint32_t buffer; - uint32_t next; -} mv88w8618_tx_desc; - -typedef struct mv88w8618_rx_desc { - uint32_t cmdstat; - uint16_t bytes; - uint16_t buffer_size; - uint32_t buffer; - uint32_t next; -} mv88w8618_rx_desc; - -#define TYPE_MV88W8618_ETH "mv88w8618_eth" -OBJECT_DECLARE_SIMPLE_TYPE(mv88w8618_eth_state, MV88W8618_ETH) - -struct mv88w8618_eth_state { - /*< private >*/ - SysBusDevice parent_obj; - /*< public >*/ - - MemoryRegion iomem; - qemu_irq irq; - MemoryRegion *dma_mr; - AddressSpace dma_as; - uint32_t smir; - uint32_t icr; - uint32_t imr; - int mmio_index; - uint32_t vlan_header; - uint32_t tx_queue[2]; - uint32_t rx_queue[4]; - uint32_t frx_queue[4]; - uint32_t cur_rx[4]; - NICState *nic; - NICConf conf; -}; - -static void eth_rx_desc_put(AddressSpace *dma_as, uint32_t addr, - mv88w8618_rx_desc *desc) -{ - cpu_to_le32s(&desc->cmdstat); - cpu_to_le16s(&desc->bytes); - cpu_to_le16s(&desc->buffer_size); - cpu_to_le32s(&desc->buffer); - cpu_to_le32s(&desc->next); - dma_memory_write(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); -} - -static void eth_rx_desc_get(AddressSpace *dma_as, uint32_t addr, - mv88w8618_rx_desc *desc) -{ - dma_memory_read(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); - le32_to_cpus(&desc->cmdstat); - le16_to_cpus(&desc->bytes); - le16_to_cpus(&desc->buffer_size); - le32_to_cpus(&desc->buffer); - le32_to_cpus(&desc->next); -} - -static ssize_t eth_receive(NetClientState *nc, const uint8_t *buf, size_t size) -{ - mv88w8618_eth_state *s = qemu_get_nic_opaque(nc); - uint32_t desc_addr; - mv88w8618_rx_desc desc; - int i; - - for (i = 0; i < 4; i++) { - desc_addr = s->cur_rx[i]; - if (!desc_addr) { - continue; - } - do { - eth_rx_desc_get(&s->dma_as, desc_addr, &desc); - if ((desc.cmdstat & MP_ETH_RX_OWN) && desc.buffer_size >= size) { - dma_memory_write(&s->dma_as, desc.buffer + s->vlan_header, - buf, size, MEMTXATTRS_UNSPECIFIED); - desc.bytes = size + s->vlan_header; - desc.cmdstat &= ~MP_ETH_RX_OWN; - s->cur_rx[i] = desc.next; - - s->icr |= MP_ETH_IRQ_RX; - if (s->icr & s->imr) { - qemu_irq_raise(s->irq); - } - eth_rx_desc_put(&s->dma_as, desc_addr, &desc); - return size; - } - desc_addr = desc.next; - } while (desc_addr != s->rx_queue[i]); - } - return size; -} - -static void eth_tx_desc_put(AddressSpace *dma_as, uint32_t addr, - mv88w8618_tx_desc *desc) -{ - cpu_to_le32s(&desc->cmdstat); - cpu_to_le16s(&desc->res); - cpu_to_le16s(&desc->bytes); - cpu_to_le32s(&desc->buffer); - cpu_to_le32s(&desc->next); - dma_memory_write(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); -} - -static void eth_tx_desc_get(AddressSpace *dma_as, uint32_t addr, - mv88w8618_tx_desc *desc) -{ - dma_memory_read(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); - le32_to_cpus(&desc->cmdstat); - le16_to_cpus(&desc->res); - le16_to_cpus(&desc->bytes); - le32_to_cpus(&desc->buffer); - le32_to_cpus(&desc->next); -} - -static void eth_send(mv88w8618_eth_state *s, int queue_index) -{ - uint32_t desc_addr = s->tx_queue[queue_index]; - mv88w8618_tx_desc desc; - uint32_t next_desc; - uint8_t buf[2048]; - int len; - - do { - eth_tx_desc_get(&s->dma_as, desc_addr, &desc); - next_desc = desc.next; - if (desc.cmdstat & MP_ETH_TX_OWN) { - len = desc.bytes; - if (len < 2048) { - dma_memory_read(&s->dma_as, desc.buffer, buf, len, - MEMTXATTRS_UNSPECIFIED); - qemu_send_packet(qemu_get_queue(s->nic), buf, len); - } - desc.cmdstat &= ~MP_ETH_TX_OWN; - s->icr |= 1 << (MP_ETH_IRQ_TXLO_BIT - queue_index); - eth_tx_desc_put(&s->dma_as, desc_addr, &desc); - } - desc_addr = next_desc; - } while (desc_addr != s->tx_queue[queue_index]); -} - -static uint64_t mv88w8618_eth_read(void *opaque, hwaddr offset, - unsigned size) -{ - mv88w8618_eth_state *s = opaque; - - switch (offset) { - case MP_ETH_SMIR: - if (s->smir & MP_ETH_SMIR_OPCODE) { - switch (s->smir & MP_ETH_SMIR_ADDR) { - case MP_ETH_PHY1_BMSR: - return MP_PHY_BMSR_LINK | MP_PHY_BMSR_AUTONEG | - MP_ETH_SMIR_RDVALID; - case MP_ETH_PHY1_PHYSID1: - return (MP_PHY_88E3015 >> 16) | MP_ETH_SMIR_RDVALID; - case MP_ETH_PHY1_PHYSID2: - return (MP_PHY_88E3015 & 0xFFFF) | MP_ETH_SMIR_RDVALID; - default: - return MP_ETH_SMIR_RDVALID; - } - } - return 0; - - case MP_ETH_ICR: - return s->icr; - - case MP_ETH_IMR: - return s->imr; - - case MP_ETH_FRDP0 ... MP_ETH_FRDP3: - return s->frx_queue[(offset - MP_ETH_FRDP0) / 4]; - - case MP_ETH_CRDP0 ... MP_ETH_CRDP3: - return s->rx_queue[(offset - MP_ETH_CRDP0) / 4]; - - case MP_ETH_CTDP0 ... MP_ETH_CTDP1: - return s->tx_queue[(offset - MP_ETH_CTDP0) / 4]; - - default: - return 0; - } -} - -static void mv88w8618_eth_write(void *opaque, hwaddr offset, - uint64_t value, unsigned size) -{ - mv88w8618_eth_state *s = opaque; - - switch (offset) { - case MP_ETH_SMIR: - s->smir = value; - break; - - case MP_ETH_PCXR: - s->vlan_header = ((value >> MP_ETH_PCXR_2BSM_BIT) & 1) * 2; - break; - - case MP_ETH_SDCMR: - if (value & MP_ETH_CMD_TXHI) { - eth_send(s, 1); - } - if (value & MP_ETH_CMD_TXLO) { - eth_send(s, 0); - } - if (value & (MP_ETH_CMD_TXHI | MP_ETH_CMD_TXLO) && s->icr & s->imr) { - qemu_irq_raise(s->irq); - } - break; - - case MP_ETH_ICR: - s->icr &= value; - break; - - case MP_ETH_IMR: - s->imr = value; - if (s->icr & s->imr) { - qemu_irq_raise(s->irq); - } - break; - - case MP_ETH_FRDP0 ... MP_ETH_FRDP3: - s->frx_queue[(offset - MP_ETH_FRDP0) / 4] = value; - break; - - case MP_ETH_CRDP0 ... MP_ETH_CRDP3: - s->rx_queue[(offset - MP_ETH_CRDP0) / 4] = - s->cur_rx[(offset - MP_ETH_CRDP0) / 4] = value; - break; - - case MP_ETH_CTDP0 ... MP_ETH_CTDP1: - s->tx_queue[(offset - MP_ETH_CTDP0) / 4] = value; - break; - } -} - -static const MemoryRegionOps mv88w8618_eth_ops = { - .read = mv88w8618_eth_read, - .write = mv88w8618_eth_write, - .endianness = DEVICE_NATIVE_ENDIAN, -}; - -static void eth_cleanup(NetClientState *nc) -{ - mv88w8618_eth_state *s = qemu_get_nic_opaque(nc); - - s->nic = NULL; -} - -static NetClientInfo net_mv88w8618_info = { - .type = NET_CLIENT_DRIVER_NIC, - .size = sizeof(NICState), - .receive = eth_receive, - .cleanup = eth_cleanup, -}; - -static void mv88w8618_eth_init(Object *obj) -{ - SysBusDevice *sbd = SYS_BUS_DEVICE(obj); - DeviceState *dev = DEVICE(sbd); - mv88w8618_eth_state *s = MV88W8618_ETH(dev); - - sysbus_init_irq(sbd, &s->irq); - memory_region_init_io(&s->iomem, obj, &mv88w8618_eth_ops, s, - "mv88w8618-eth", MP_ETH_SIZE); - sysbus_init_mmio(sbd, &s->iomem); -} - -static void mv88w8618_eth_realize(DeviceState *dev, Error **errp) -{ - mv88w8618_eth_state *s = MV88W8618_ETH(dev); - - if (!s->dma_mr) { - error_setg(errp, TYPE_MV88W8618_ETH " 'dma-memory' link not set"); - return; - } - - address_space_init(&s->dma_as, s->dma_mr, "emac-dma"); - s->nic = qemu_new_nic(&net_mv88w8618_info, &s->conf, - object_get_typename(OBJECT(dev)), dev->id, s); -} - -static const VMStateDescription mv88w8618_eth_vmsd = { - .name = "mv88w8618_eth", - .version_id = 1, - .minimum_version_id = 1, - .fields = (VMStateField[]) { - VMSTATE_UINT32(smir, mv88w8618_eth_state), - VMSTATE_UINT32(icr, mv88w8618_eth_state), - VMSTATE_UINT32(imr, mv88w8618_eth_state), - VMSTATE_UINT32(vlan_header, mv88w8618_eth_state), - VMSTATE_UINT32_ARRAY(tx_queue, mv88w8618_eth_state, 2), - VMSTATE_UINT32_ARRAY(rx_queue, mv88w8618_eth_state, 4), - VMSTATE_UINT32_ARRAY(frx_queue, mv88w8618_eth_state, 4), - VMSTATE_UINT32_ARRAY(cur_rx, mv88w8618_eth_state, 4), - VMSTATE_END_OF_LIST() - } -}; - -static Property mv88w8618_eth_properties[] = { - DEFINE_NIC_PROPERTIES(mv88w8618_eth_state, conf), - DEFINE_PROP_LINK("dma-memory", mv88w8618_eth_state, dma_mr, - TYPE_MEMORY_REGION, MemoryRegion *), - DEFINE_PROP_END_OF_LIST(), -}; - -static void mv88w8618_eth_class_init(ObjectClass *klass, void *data) -{ - DeviceClass *dc = DEVICE_CLASS(klass); - - dc->vmsd = &mv88w8618_eth_vmsd; - device_class_set_props(dc, mv88w8618_eth_properties); - dc->realize = mv88w8618_eth_realize; -} - -static const TypeInfo mv88w8618_eth_info = { - .name = TYPE_MV88W8618_ETH, - .parent = TYPE_SYS_BUS_DEVICE, - .instance_size = sizeof(mv88w8618_eth_state), - .instance_init = mv88w8618_eth_init, - .class_init = mv88w8618_eth_class_init, -}; - /* LCD register offsets */ #define MP_LCD_IRQCTRL 0x180 #define MP_LCD_IRQSTAT 0x184 @@ -XXX,XX +XXX,XX @@ static void musicpal_register_types(void) type_register_static(&mv88w8618_pic_info); type_register_static(&mv88w8618_pit_info); type_register_static(&mv88w8618_flashcfg_info); - type_register_static(&mv88w8618_eth_info); type_register_static(&mv88w8618_wlan_info); type_register_static(&musicpal_lcd_info); type_register_static(&musicpal_gpio_info); diff --git a/hw/net/mv88w8618_eth.c b/hw/net/mv88w8618_eth.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/hw/net/mv88w8618_eth.c @@ -XXX,XX +XXX,XX @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Marvell MV88W8618 / Freecom MusicPal emulation. + * + * Copyright (c) 2008 Jan Kiszka + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "hw/qdev-properties.h" +#include "hw/sysbus.h" +#include "hw/irq.h" +#include "hw/net/mv88w8618_eth.h" +#include "migration/vmstate.h" +#include "sysemu/dma.h" +#include "net/net.h" + +#define MP_ETH_SIZE 0x00001000 + +/* Ethernet register offsets */ +#define MP_ETH_SMIR 0x010 +#define MP_ETH_PCXR 0x408 +#define MP_ETH_SDCMR 0x448 +#define MP_ETH_ICR 0x450 +#define MP_ETH_IMR 0x458 +#define MP_ETH_FRDP0 0x480 +#define MP_ETH_FRDP1 0x484 +#define MP_ETH_FRDP2 0x488 +#define MP_ETH_FRDP3 0x48C +#define MP_ETH_CRDP0 0x4A0 +#define MP_ETH_CRDP1 0x4A4 +#define MP_ETH_CRDP2 0x4A8 +#define MP_ETH_CRDP3 0x4AC +#define MP_ETH_CTDP0 0x4E0 +#define MP_ETH_CTDP1 0x4E4 + +/* MII PHY access */ +#define MP_ETH_SMIR_DATA 0x0000FFFF +#define MP_ETH_SMIR_ADDR 0x03FF0000 +#define MP_ETH_SMIR_OPCODE (1 << 26) /* Read value */ +#define MP_ETH_SMIR_RDVALID (1 << 27) + +/* PHY registers */ +#define MP_ETH_PHY1_BMSR 0x00210000 +#define MP_ETH_PHY1_PHYSID1 0x00410000 +#define MP_ETH_PHY1_PHYSID2 0x00610000 + +#define MP_PHY_BMSR_LINK 0x0004 +#define MP_PHY_BMSR_AUTONEG 0x0008 + +#define MP_PHY_88E3015 0x01410E20 + +/* TX descriptor status */ +#define MP_ETH_TX_OWN (1U << 31) + +/* RX descriptor status */ +#define MP_ETH_RX_OWN (1U << 31) + +/* Interrupt cause/mask bits */ +#define MP_ETH_IRQ_RX_BIT 0 +#define MP_ETH_IRQ_RX (1 << MP_ETH_IRQ_RX_BIT) +#define MP_ETH_IRQ_TXHI_BIT 2 +#define MP_ETH_IRQ_TXLO_BIT 3 + +/* Port config bits */ +#define MP_ETH_PCXR_2BSM_BIT 28 /* 2-byte incoming suffix */ + +/* SDMA command bits */ +#define MP_ETH_CMD_TXHI (1 << 23) +#define MP_ETH_CMD_TXLO (1 << 22) + +typedef struct mv88w8618_tx_desc { + uint32_t cmdstat; + uint16_t res; + uint16_t bytes; + uint32_t buffer; + uint32_t next; +} mv88w8618_tx_desc; + +typedef struct mv88w8618_rx_desc { + uint32_t cmdstat; + uint16_t bytes; + uint16_t buffer_size; + uint32_t buffer; + uint32_t next; +} mv88w8618_rx_desc; + +OBJECT_DECLARE_SIMPLE_TYPE(mv88w8618_eth_state, MV88W8618_ETH) + +struct mv88w8618_eth_state { + /*< private >*/ + SysBusDevice parent_obj; + /*< public >*/ + + MemoryRegion iomem; + qemu_irq irq; + MemoryRegion *dma_mr; + AddressSpace dma_as; + uint32_t smir; + uint32_t icr; + uint32_t imr; + int mmio_index; + uint32_t vlan_header; + uint32_t tx_queue[2]; + uint32_t rx_queue[4]; + uint32_t frx_queue[4]; + uint32_t cur_rx[4]; + NICState *nic; + NICConf conf; +}; + +static void eth_rx_desc_put(AddressSpace *dma_as, uint32_t addr, + mv88w8618_rx_desc *desc) +{ + cpu_to_le32s(&desc->cmdstat); + cpu_to_le16s(&desc->bytes); + cpu_to_le16s(&desc->buffer_size); + cpu_to_le32s(&desc->buffer); + cpu_to_le32s(&desc->next); + dma_memory_write(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); +} + +static void eth_rx_desc_get(AddressSpace *dma_as, uint32_t addr, + mv88w8618_rx_desc *desc) +{ + dma_memory_read(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); + le32_to_cpus(&desc->cmdstat); + le16_to_cpus(&desc->bytes); + le16_to_cpus(&desc->buffer_size); + le32_to_cpus(&desc->buffer); + le32_to_cpus(&desc->next); +} + +static ssize_t eth_receive(NetClientState *nc, const uint8_t *buf, size_t size) +{ + mv88w8618_eth_state *s = qemu_get_nic_opaque(nc); + uint32_t desc_addr; + mv88w8618_rx_desc desc; + int i; + + for (i = 0; i < 4; i++) { + desc_addr = s->cur_rx[i]; + if (!desc_addr) { + continue; + } + do { + eth_rx_desc_get(&s->dma_as, desc_addr, &desc); + if ((desc.cmdstat & MP_ETH_RX_OWN) && desc.buffer_size >= size) { + dma_memory_write(&s->dma_as, desc.buffer + s->vlan_header, + buf, size, MEMTXATTRS_UNSPECIFIED); + desc.bytes = size + s->vlan_header; + desc.cmdstat &= ~MP_ETH_RX_OWN; + s->cur_rx[i] = desc.next; + + s->icr |= MP_ETH_IRQ_RX; + if (s->icr & s->imr) { + qemu_irq_raise(s->irq); + } + eth_rx_desc_put(&s->dma_as, desc_addr, &desc); + return size; + } + desc_addr = desc.next; + } while (desc_addr != s->rx_queue[i]); + } + return size; +} + +static void eth_tx_desc_put(AddressSpace *dma_as, uint32_t addr, + mv88w8618_tx_desc *desc) +{ + cpu_to_le32s(&desc->cmdstat); + cpu_to_le16s(&desc->res); + cpu_to_le16s(&desc->bytes); + cpu_to_le32s(&desc->buffer); + cpu_to_le32s(&desc->next); + dma_memory_write(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); +} + +static void eth_tx_desc_get(AddressSpace *dma_as, uint32_t addr, + mv88w8618_tx_desc *desc) +{ + dma_memory_read(dma_as, addr, desc, sizeof(*desc), MEMTXATTRS_UNSPECIFIED); + le32_to_cpus(&desc->cmdstat); + le16_to_cpus(&desc->res); + le16_to_cpus(&desc->bytes); + le32_to_cpus(&desc->buffer); + le32_to_cpus(&desc->next); +} + +static void eth_send(mv88w8618_eth_state *s, int queue_index) +{ + uint32_t desc_addr = s->tx_queue[queue_index]; + mv88w8618_tx_desc desc; + uint32_t next_desc; + uint8_t buf[2048]; + int len; + + do { + eth_tx_desc_get(&s->dma_as, desc_addr, &desc); + next_desc = desc.next; + if (desc.cmdstat & MP_ETH_TX_OWN) { + len = desc.bytes; + if (len < 2048) { + dma_memory_read(&s->dma_as, desc.buffer, buf, len, + MEMTXATTRS_UNSPECIFIED); + qemu_send_packet(qemu_get_queue(s->nic), buf, len); + } + desc.cmdstat &= ~MP_ETH_TX_OWN; + s->icr |= 1 << (MP_ETH_IRQ_TXLO_BIT - queue_index); + eth_tx_desc_put(&s->dma_as, desc_addr, &desc); + } + desc_addr = next_desc; + } while (desc_addr != s->tx_queue[queue_index]); +} + +static uint64_t mv88w8618_eth_read(void *opaque, hwaddr offset, + unsigned size) +{ + mv88w8618_eth_state *s = opaque; + + switch (offset) { + case MP_ETH_SMIR: + if (s->smir & MP_ETH_SMIR_OPCODE) { + switch (s->smir & MP_ETH_SMIR_ADDR) { + case MP_ETH_PHY1_BMSR: + return MP_PHY_BMSR_LINK | MP_PHY_BMSR_AUTONEG | + MP_ETH_SMIR_RDVALID; + case MP_ETH_PHY1_PHYSID1: + return (MP_PHY_88E3015 >> 16) | MP_ETH_SMIR_RDVALID; + case MP_ETH_PHY1_PHYSID2: + return (MP_PHY_88E3015 & 0xFFFF) | MP_ETH_SMIR_RDVALID; + default: + return MP_ETH_SMIR_RDVALID; + } + } + return 0; + + case MP_ETH_ICR: + return s->icr; + + case MP_ETH_IMR: + return s->imr; + + case MP_ETH_FRDP0 ... MP_ETH_FRDP3: + return s->frx_queue[(offset - MP_ETH_FRDP0) / 4]; + + case MP_ETH_CRDP0 ... MP_ETH_CRDP3: + return s->rx_queue[(offset - MP_ETH_CRDP0) / 4]; + + case MP_ETH_CTDP0 ... MP_ETH_CTDP1: + return s->tx_queue[(offset - MP_ETH_CTDP0) / 4]; + + default: + return 0; + } +} + +static void mv88w8618_eth_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + mv88w8618_eth_state *s = opaque; + + switch (offset) { + case MP_ETH_SMIR: + s->smir = value; + break; + + case MP_ETH_PCXR: + s->vlan_header = ((value >> MP_ETH_PCXR_2BSM_BIT) & 1) * 2; + break; + + case MP_ETH_SDCMR: + if (value & MP_ETH_CMD_TXHI) { + eth_send(s, 1); + } + if (value & MP_ETH_CMD_TXLO) { + eth_send(s, 0); + } + if (value & (MP_ETH_CMD_TXHI | MP_ETH_CMD_TXLO) && s->icr & s->imr) { + qemu_irq_raise(s->irq); + } + break; + + case MP_ETH_ICR: + s->icr &= value; + break; + + case MP_ETH_IMR: + s->imr = value; + if (s->icr & s->imr) { + qemu_irq_raise(s->irq); + } + break; + + case MP_ETH_FRDP0 ... MP_ETH_FRDP3: + s->frx_queue[(offset - MP_ETH_FRDP0) / 4] = value; + break; + + case MP_ETH_CRDP0 ... MP_ETH_CRDP3: + s->rx_queue[(offset - MP_ETH_CRDP0) / 4] = + s->cur_rx[(offset - MP_ETH_CRDP0) / 4] = value; + break; + + case MP_ETH_CTDP0 ... MP_ETH_CTDP1: + s->tx_queue[(offset - MP_ETH_CTDP0) / 4] = value; + break; + } +} + +static const MemoryRegionOps mv88w8618_eth_ops = { + .read = mv88w8618_eth_read, + .write = mv88w8618_eth_write, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void eth_cleanup(NetClientState *nc) +{ + mv88w8618_eth_state *s = qemu_get_nic_opaque(nc); + + s->nic = NULL; +} + +static NetClientInfo net_mv88w8618_info = { + .type = NET_CLIENT_DRIVER_NIC, + .size = sizeof(NICState), + .receive = eth_receive, + .cleanup = eth_cleanup, +}; + +static void mv88w8618_eth_init(Object *obj) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + DeviceState *dev = DEVICE(sbd); + mv88w8618_eth_state *s = MV88W8618_ETH(dev); + + sysbus_init_irq(sbd, &s->irq); + memory_region_init_io(&s->iomem, obj, &mv88w8618_eth_ops, s, + "mv88w8618-eth", MP_ETH_SIZE); + sysbus_init_mmio(sbd, &s->iomem); +} + +static void mv88w8618_eth_realize(DeviceState *dev, Error **errp) +{ + mv88w8618_eth_state *s = MV88W8618_ETH(dev); + + if (!s->dma_mr) { + error_setg(errp, TYPE_MV88W8618_ETH " 'dma-memory' link not set"); + return; + } + + address_space_init(&s->dma_as, s->dma_mr, "emac-dma"); + s->nic = qemu_new_nic(&net_mv88w8618_info, &s->conf, + object_get_typename(OBJECT(dev)), dev->id, s); +} + +static const VMStateDescription mv88w8618_eth_vmsd = { + .name = "mv88w8618_eth", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(smir, mv88w8618_eth_state), + VMSTATE_UINT32(icr, mv88w8618_eth_state), + VMSTATE_UINT32(imr, mv88w8618_eth_state), + VMSTATE_UINT32(vlan_header, mv88w8618_eth_state), + VMSTATE_UINT32_ARRAY(tx_queue, mv88w8618_eth_state, 2), + VMSTATE_UINT32_ARRAY(rx_queue, mv88w8618_eth_state, 4), + VMSTATE_UINT32_ARRAY(frx_queue, mv88w8618_eth_state, 4), + VMSTATE_UINT32_ARRAY(cur_rx, mv88w8618_eth_state, 4), + VMSTATE_END_OF_LIST() + } +}; + +static Property mv88w8618_eth_properties[] = { + DEFINE_NIC_PROPERTIES(mv88w8618_eth_state, conf), + DEFINE_PROP_LINK("dma-memory", mv88w8618_eth_state, dma_mr, + TYPE_MEMORY_REGION, MemoryRegion *), + DEFINE_PROP_END_OF_LIST(), +}; + +static void mv88w8618_eth_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &mv88w8618_eth_vmsd; + device_class_set_props(dc, mv88w8618_eth_properties); + dc->realize = mv88w8618_eth_realize; +} + +static const TypeInfo mv88w8618_eth_info = { + .name = TYPE_MV88W8618_ETH, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(mv88w8618_eth_state), + .instance_init = mv88w8618_eth_init, + .class_init = mv88w8618_eth_class_init, +}; + +static void musicpal_register_types(void) +{ + type_register_static(&mv88w8618_eth_info); +} + +type_init(musicpal_register_types) + diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ M: Peter Maydell <peter.maydell@linaro.org> L: qemu-arm@nongnu.org S: Odd Fixes F: hw/arm/musicpal.c +F: hw/net/mv88w8618_eth.c +F: include/hw/net/mv88w8618_eth.h F: docs/system/arm/musicpal.rst Nuvoton NPCM7xx diff --git a/hw/net/meson.build b/hw/net/meson.build index XXXXXXX..XXXXXXX 100644 --- a/hw/net/meson.build +++ b/hw/net/meson.build @@ -XXX,XX +XXX,XX @@ softmmu_ss.add(when: 'CONFIG_ALLWINNER_EMAC', if_true: files('allwinner_emac.c') softmmu_ss.add(when: 'CONFIG_ALLWINNER_SUN8I_EMAC', if_true: files('allwinner-sun8i-emac.c')) softmmu_ss.add(when: 'CONFIG_IMX_FEC', if_true: files('imx_fec.c')) softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-emac.c')) +softmmu_ss.add(when: 'CONFIG_MARVELL_88W8618', if_true: files('mv88w8618_eth.c')) softmmu_ss.add(when: 'CONFIG_CADENCE', if_true: files('cadence_gem.c')) softmmu_ss.add(when: 'CONFIG_STELLARIS_ENET', if_true: files('stellaris_enet.c')) -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> ARM64 machines like Kunpeng Family Server Chips have a level of hardware topology in which a group of CPU cores share L3 cache tag or L2 cache. For example, Kunpeng 920 typically has 6 or 8 clusters in each NUMA node (also represent range of CPU die), and each cluster has 4 CPU cores. All clusters share L3 cache data, but CPU cores in each cluster share a local L3 tag. Running a guest kernel with Cluster-Aware Scheduling on the Hosts which have physical clusters, if we can design a vCPU topology with cluster level for guest kernel and then have a dedicated vCPU pinning, the guest will gain scheduling performance improvement from cache affinity of CPU cluster. So let's enable the support for this new parameter on ARM virt machines. After this patch, we can define a 4-level CPU hierarchy like: cpus=*,maxcpus=*,sockets=*,clusters=*, cores=*,threads=*. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20220107083232.16256-2-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 1 + qemu-options.hx | 10 ++++++++++ 2 files changed, 11 insertions(+) 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 void virt_machine_class_init(ObjectClass *oc, void *data) hc->unplug_request = virt_machine_device_unplug_request_cb; hc->unplug = virt_machine_device_unplug_cb; mc->nvdimm_supported = true; + mc->smp_props.clusters_supported = true; mc->auto_enable_numa_with_memhp = true; mc->auto_enable_numa_with_memdev = true; mc->default_ram_id = "mach-virt.ram"; diff --git a/qemu-options.hx b/qemu-options.hx index XXXXXXX..XXXXXXX 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -XXX,XX +XXX,XX @@ SRST -smp 16,sockets=2,dies=2,cores=2,threads=2,maxcpus=16 + The following sub-option defines a CPU topology hierarchy (2 sockets + totally on the machine, 2 clusters per socket, 2 cores per cluster, + 2 threads per core) for ARM virt machines which support sockets/clusters + /cores/threads. Some members of the option can be omitted but their values + will be automatically computed: + + :: + + -smp 16,sockets=2,clusters=2,cores=2,threads=2,maxcpus=16 + Historically preference was given to the coarsest topology parameters when computing missing values (ie sockets preferred over cores, which were preferred over threads), however, this behaviour is considered -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> Support one cluster level between core and physical package in the cpu-map of Arm/virt devicetree. This is also consistent with Linux Doc "Documentation/devicetree/bindings/cpu/cpu-topology.txt". Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20220107083232.16256-3-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) 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 void fdt_add_cpu_nodes(const VirtMachineState *vms) * can contain several layers of clustering within a single physical * package and cluster nodes can be contained in parent cluster nodes. * - * Given that cluster is not yet supported in the vCPU topology, - * we currently generate one cluster node within each socket node - * by default. + * Note: currently we only support one layer of clustering within + * each physical package. */ qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map"); @@ -XXX,XX +XXX,XX @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms) if (ms->smp.threads > 1) { map_path = g_strdup_printf( - "/cpus/cpu-map/socket%d/cluster0/core%d/thread%d", - cpu / (ms->smp.cores * ms->smp.threads), + "/cpus/cpu-map/socket%d/cluster%d/core%d/thread%d", + cpu / (ms->smp.clusters * ms->smp.cores * ms->smp.threads), + (cpu / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters, (cpu / ms->smp.threads) % ms->smp.cores, cpu % ms->smp.threads); } else { map_path = g_strdup_printf( - "/cpus/cpu-map/socket%d/cluster0/core%d", - cpu / ms->smp.cores, + "/cpus/cpu-map/socket%d/cluster%d/core%d", + cpu / (ms->smp.clusters * ms->smp.cores), + (cpu / ms->smp.cores) % ms->smp.clusters, cpu % ms->smp.cores); } qemu_fdt_add_path(ms->fdt, map_path); -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> Use g_queue APIs to reduce the nested loops and code indentation with the processor hierarchy levels increasing. Consenquently, it's more scalable to add new topology level to build_pptt. No functional change intended. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20220107083232.16256-4-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index XXXXXXX..XXXXXXX 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -XXX,XX +XXX,XX @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, const char *oem_id, const char *oem_table_id) { - int pptt_start = table_data->len; + GQueue *list = g_queue_new(); + guint pptt_start = table_data->len; + guint parent_offset; + guint length, i; int uid = 0; int socket; AcpiTable table = { .sig = "PPTT", .rev = 2, @@ -XXX,XX +XXX,XX @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, acpi_table_begin(&table, table_data); for (socket = 0; socket < ms->smp.sockets; socket++) { - uint32_t socket_offset = table_data->len - pptt_start; - int core; - + g_queue_push_tail(list, + GUINT_TO_POINTER(table_data->len - pptt_start)); build_processor_hierarchy_node( table_data, /* @@ -XXX,XX +XXX,XX @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, */ (1 << 0), 0, socket, NULL, 0); + } + length = g_queue_get_length(list); + for (i = 0; i < length; i++) { + int core; + + parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list)); for (core = 0; core < ms->smp.cores; core++) { - uint32_t core_offset = table_data->len - pptt_start; - int thread; - if (ms->smp.threads > 1) { + g_queue_push_tail(list, + GUINT_TO_POINTER(table_data->len - pptt_start)); build_processor_hierarchy_node( table_data, (0 << 0), /* not a physical package */ - socket_offset, core, NULL, 0); - - for (thread = 0; thread < ms->smp.threads; thread++) { - build_processor_hierarchy_node( - table_data, - (1 << 1) | /* ACPI Processor ID valid */ - (1 << 2) | /* Processor is a Thread */ - (1 << 3), /* Node is a Leaf */ - core_offset, uid++, NULL, 0); - } + parent_offset, core, NULL, 0); } else { build_processor_hierarchy_node( table_data, (1 << 1) | /* ACPI Processor ID valid */ (1 << 3), /* Node is a Leaf */ - socket_offset, uid++, NULL, 0); + parent_offset, uid++, NULL, 0); } } } + length = g_queue_get_length(list); + for (i = 0; i < length; i++) { + int thread; + + parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list)); + for (thread = 0; thread < ms->smp.threads; thread++) { + build_processor_hierarchy_node( + table_data, + (1 << 1) | /* ACPI Processor ID valid */ + (1 << 2) | /* Processor is a Thread */ + (1 << 3), /* Node is a Leaf */ + parent_offset, uid++, NULL, 0); + } + } + + g_queue_free(list); acpi_table_end(linker, &table); } -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> List test/data/acpi/virt/PPTT as the expected files allowed to be changed in tests/qtest/bios-tables-test-allowed-diff.h Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Acked-by: Ani Sinha <ani@anisinha.ca> Message-id: 20220107083232.16256-5-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- tests/qtest/bios-tables-test-allowed-diff.h | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,2 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/virt/PPTT", -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> Support CPU cluster topology level in generation of ACPI Processor Properties Topology Table (PPTT). Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Message-id: 20220107083232.16256-6-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/acpi/aml-build.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index XXXXXXX..XXXXXXX 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -XXX,XX +XXX,XX @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, const char *oem_id, const char *oem_table_id) { + MachineClass *mc = MACHINE_GET_CLASS(ms); GQueue *list = g_queue_new(); guint pptt_start = table_data->len; guint parent_offset; @@ -XXX,XX +XXX,XX @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, 0, socket, NULL, 0); } + if (mc->smp_props.clusters_supported) { + length = g_queue_get_length(list); + for (i = 0; i < length; i++) { + int cluster; + + parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list)); + for (cluster = 0; cluster < ms->smp.clusters; cluster++) { + g_queue_push_tail(list, + GUINT_TO_POINTER(table_data->len - pptt_start)); + build_processor_hierarchy_node( + table_data, + (0 << 0), /* not a physical package */ + parent_offset, cluster, NULL, 0); + } + } + } + length = g_queue_get_length(list); for (i = 0; i < length; i++) { int core; -- 2.25.1 From: Yanan Wang <wangyanan55@huawei.com> Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory to update PPTT binary. Also empty bios-tables-test-allowed-diff.h. The disassembled differences between actual and expected PPTT: /* * Intel ACPI Component Architecture * AML/ASL+ Disassembler version 20200528 (64-bit version) * Copyright (c) 2000 - 2020 Intel Corporation * - * Disassembly of tests/data/acpi/virt/PPTT, Tue Jan 4 12:51:11 2022 + * Disassembly of /tmp/aml-2ZGOF1, Tue Jan 4 12:51:11 2022 * * ACPI Data Table [PPTT] * * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue */ [000h 0000 4] Signature : "PPTT" [Processor Properties Topology Table] -[004h 0004 4] Table Length : 0000004C +[004h 0004 4] Table Length : 00000060 [008h 0008 1] Revision : 02 -[009h 0009 1] Checksum : A8 +[009h 0009 1] Checksum : 48 [00Ah 0010 6] Oem ID : "BOCHS " [010h 0016 8] Oem Table ID : "BXPC " [018h 0024 4] Oem Revision : 00000001 [01Ch 0028 4] Asl Compiler ID : "BXPC" [020h 0032 4] Asl Compiler Revision : 00000001 [024h 0036 1] Subtable Type : 00 [Processor Hierarchy Node] [025h 0037 1] Length : 14 [026h 0038 2] Reserved : 0000 [028h 0040 4] Flags (decoded below) : 00000001 Physical package : 1 ACPI Processor ID valid : 0 Processor is a thread : 0 Node is a leaf : 0 Identical Implementation : 0 [02Ch 0044 4] Parent : 00000000 [030h 0048 4] ACPI Processor ID : 00000000 [034h 0052 4] Private Resource Number : 00000000 [038h 0056 1] Subtable Type : 00 [Processor Hierarchy Node] [039h 0057 1] Length : 14 [03Ah 0058 2] Reserved : 0000 -[03Ch 0060 4] Flags (decoded below) : 0000000A +[03Ch 0060 4] Flags (decoded below) : 00000000 Physical package : 0 - ACPI Processor ID valid : 1 + ACPI Processor ID valid : 0 Processor is a thread : 0 - Node is a leaf : 1 + Node is a leaf : 0 Identical Implementation : 0 [040h 0064 4] Parent : 00000024 [044h 0068 4] ACPI Processor ID : 00000000 [048h 0072 4] Private Resource Number : 00000000 -Raw Table Data: Length 76 (0x4C) +[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy Node] +[04Dh 0077 1] Length : 14 +[04Eh 0078 2] Reserved : 0000 +[050h 0080 4] Flags (decoded below) : 0000000A + Physical package : 0 + ACPI Processor ID valid : 1 + Processor is a thread : 0 + Node is a leaf : 1 + Identical Implementation : 0 +[054h 0084 4] Parent : 00000038 +[058h 0088 4] ACPI Processor ID : 00000000 +[05Ch 0092 4] Private Resource Number : 00000000 + +Raw Table Data: Length 96 (0x60) - 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 // PPTTL.....BOCHS + 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 // PPTT`....HBOCHS 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC 0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 // ................ - 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 // ................ - 0040: 24 00 00 00 00 00 00 00 00 00 00 00 // $........... + 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 // ................ + 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 // $............... + 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 // ....8........... Signed-off-by: Yanan Wang <wangyanan55@huawei.com> Reviewed-by: Ani Sinha <ani@anisinha.ca> Message-id: 20220107083232.16256-7-wangyanan55@huawei.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- tests/qtest/bios-tables-test-allowed-diff.h | 1 - tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes 2 files changed, 1 deletion(-) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,2 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/virt/PPTT", diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT index XXXXXXX..XXXXXXX 100644 GIT binary patch delta 53 pcmeZC;0g!`2}xjJU|{l?$YrDgWH5jU5Ca567#O&Klm(arApowi1QY-O delta 32 fcmYfB;R*-{3GrcIU|?D?k;`ae01J-_kOKn%ZFdCM -- 2.25.1 From: Lucas Ramage <lucas.ramage@infinite-omicron.com> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/527 Signed-off-by: Lucas Ramage <lucas.ramage@infinite-omicron.com> Message-id: 20220105205628.5491-1-oxr463@gmx.us Reviewed-by: Peter Maydell <peter.maydell@linaro.org> [PMM: Move to docs/system/devices/ rather than top-level; fix a pre-existing typo in passing] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/system/device-emulation.rst | 1 + docs/{can.txt => system/devices/can.rst} | 90 +++++++++++------------- 2 files changed, 41 insertions(+), 50 deletions(-) rename docs/{can.txt => system/devices/can.rst} (68%) diff --git a/docs/system/device-emulation.rst b/docs/system/device-emulation.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/system/device-emulation.rst +++ b/docs/system/device-emulation.rst @@ -XXX,XX +XXX,XX @@ Emulated Devices .. toctree:: :maxdepth: 1 + devices/can.rst devices/ivshmem.rst devices/net.rst devices/nvme.rst diff --git a/docs/can.txt b/docs/system/devices/can.rst similarity index 68% rename from docs/can.txt rename to docs/system/devices/can.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/can.txt +++ b/docs/system/devices/can.rst @@ -XXX,XX +XXX,XX @@ -QEMU CAN bus emulation support -============================== - +CAN Bus Emulation Support +========================= The CAN bus emulation provides mechanism to connect multiple emulated CAN controller chips together by one or multiple CAN busses (the controller device "canbus" parameter). The individual busses @@ -XXX,XX +XXX,XX @@ emulated environment for testing and RTEMS GSoC slot has been donated to work on CAN hardware emulation on QEMU. Examples how to use CAN emulation for SJA1000 based boards -========================================================== - +---------------------------------------------------------- When QEMU with CAN PCI support is compiled then one of the next CAN boards can be selected - (1) CAN bus Kvaser PCI CAN-S (single SJA1000 channel) boad. QEMU startup options +(1) CAN bus Kvaser PCI CAN-S (single SJA1000 channel) board. QEMU startup options:: + -object can-bus,id=canbus0 -device kvaser_pci,canbus=canbus0 - Add "can-host-socketcan" object to connect device to host system CAN bus + +Add "can-host-socketcan" object to connect device to host system CAN bus:: + -object can-host-socketcan,id=canhost0,if=can0,canbus=canbus0 - (2) CAN bus PCM-3680I PCI (dual SJA1000 channel) emulation +(2) CAN bus PCM-3680I PCI (dual SJA1000 channel) emulation:: + -object can-bus,id=canbus0 -device pcm3680_pci,canbus0=canbus0,canbus1=canbus0 - another example: +Another example:: + -object can-bus,id=canbus0 -object can-bus,id=canbus1 -device pcm3680_pci,canbus0=canbus0,canbus1=canbus1 - (3) CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation +(3) CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation:: + -device mioe3680_pci,canbus0=canbus0 - The ''kvaser_pci'' board/device model is compatible with and has been tested with -''kvaser_pci'' driver included in mainline Linux kernel. +the ''kvaser_pci'' driver included in mainline Linux kernel. The tested setup was Linux 4.9 kernel on the host and guest side. -Example for qemu-system-x86_64: + +Example for qemu-system-x86_64:: qemu-system-x86_64 -accel kvm -kernel /boot/vmlinuz-4.9.0-4-amd64 \ -initrd ramdisk.cpio \ @@ -XXX,XX +XXX,XX @@ Example for qemu-system-x86_64: -device kvaser_pci,canbus=canbus0 \ -nographic -append "console=ttyS0" -Example for qemu-system-arm: +Example for qemu-system-arm:: qemu-system-arm -cpu arm1176 -m 256 -M versatilepb \ -kernel kernel-qemu-arm1176-versatilepb \ @@ -XXX,XX +XXX,XX @@ Example for qemu-system-arm: The CAN interface of the host system has to be configured for proper bitrate and set up. Configuration is not propagated from emulated devices through bus to the physical host device. Example configuration -for 1 Mbit/s +for 1 Mbit/s:: ip link set can0 type can bitrate 1000000 ip link set can0 up Virtual (host local only) can interface can be used on the host -side instead of physical interface +side instead of physical interface:: ip link add dev can0 type vcan The CAN interface on the host side can be used to analyze CAN -traffic with "candump" command which is included in "can-utils". +traffic with "candump" command which is included in "can-utils":: candump can0 CTU CAN FD support examples -=========================== - +--------------------------- This open-source core provides CAN FD support. CAN FD drames are delivered even to the host systems when SocketCAN interface is found CAN FD capable. @@ -XXX,XX +XXX,XX @@ on the board. Example how to connect the canbus0-bus (virtual wire) to the host Linux system (SocketCAN used) and to both CTU CAN FD cores emulated on the corresponding PCI card expects that host system CAN bus -is setup according to the previous SJA1000 section. +is setup according to the previous SJA1000 section:: qemu-system-x86_64 -enable-kvm -kernel /boot/vmlinuz-4.19.52+ \ -initrd ramdisk.cpio \ @@ -XXX,XX +XXX,XX @@ is setup according to the previous SJA1000 section. -device ctucan_pci,canbus0=canbus0-bus,canbus1=canbus0-bus \ -nographic -Setup of CTU CAN FD controller in a guest Linux system +Setup of CTU CAN FD controller in a guest Linux system:: insmod ctucanfd.ko || modprobe ctucanfd insmod ctucanfd_pci.ko || modprobe ctucanfd_pci @@ -XXX,XX +XXX,XX @@ Setup of CTU CAN FD controller in a guest Linux system /bin/ip link set $ifc up done -The test can run for example +The test can run for example:: candump can1 -in the guest system and next commands in the host system for basic CAN +in the guest system and next commands in the host system for basic CAN:: cangen can0 -for CAN FD without bitrate switch +for CAN FD without bitrate switch:: cangen can0 -f -and with bitrate switch +and with bitrate switch:: cangen can0 -b @@ -XXX,XX +XXX,XX @@ The test can be run viceversa, generate messages in the guest system and capture in the host one and much more combinations. Links to other resources -======================== +------------------------ - (1) CAN related projects at Czech Technical University, Faculty of Electrical Engineering - http://canbus.pages.fel.cvut.cz/ - (2) Repository with development can-pci branch at Czech Technical University - https://gitlab.fel.cvut.cz/canbus/qemu-canbus - (3) RTEMS page describing project - https://devel.rtems.org/wiki/Developer/Simulators/QEMU/CANEmulation - (4) RTLWS 2015 article about the project and its use with CANopen emulation - http://cmp.felk.cvut.cz/~pisa/can/doc/rtlws-17-pisa-qemu-can.pdf - (5) GNU/Linux, CAN and CANopen in Real-time Control Applications - Slides from LinuxDays 2017 (include updated RTLWS 2015 content) - https://www.linuxdays.cz/2017/video/Pavel_Pisa-CAN_canopen.pdf - (6) Linux SocketCAN utilities - https://github.com/linux-can/can-utils/ - (7) CTU CAN FD project including core VHDL design, Linux driver, - test utilities etc. - https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core - (8) CTU CAN FD Core Datasheet Documentation - http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf - (9) CTU CAN FD Core System Architecture Documentation - http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/ctu_can_fd_architecture.pdf - (10) CTU CAN FD Driver Documentation - http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/driver_doc/ctucanfd-driver.html - (11) Integration with PCIe interfacing for Intel/Altera Cyclone IV based board - https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd + (1) `CAN related projects at Czech Technical University, Faculty of Electrical Engineering <http://canbus.pages.fel.cvut.cz>`_ + (2) `Repository with development can-pci branch at Czech Technical University <https://gitlab.fel.cvut.cz/canbus/qemu-canbus>`_ + (3) `RTEMS page describing project <https://devel.rtems.org/wiki/Developer/Simulators/QEMU/CANEmulation>`_ + (4) `RTLWS 2015 article about the project and its use with CANopen emulation <http://cmp.felk.cvut.cz/~pisa/can/doc/rtlws-17-pisa-qemu-can.pdf>`_ + (5) `GNU/Linux, CAN and CANopen in Real-time Control Applications Slides from LinuxDays 2017 (include updated RTLWS 2015 content) <https://www.linuxdays.cz/2017/video/Pavel_Pisa-CAN_canopen.pdf>`_ + (6) `Linux SocketCAN utilities <https://github.com/linux-can/can-utils>`_ + (7) `CTU CAN FD project including core VHDL design, Linux driver, test utilities etc. <https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core>`_ + (8) `CTU CAN FD Core Datasheet Documentation <http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf>`_ + (9) `CTU CAN FD Core System Architecture Documentation <http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/ctu_can_fd_architecture.pdf>`_ + (10) `CTU CAN FD Driver Documentation <http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/driver_doc/ctucanfd-driver.html>`_ + (11) `Integration with PCIe interfacing for Intel/Altera Cyclone IV based board <https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd>`_ -- 2.25.1 From: Gavin Shan <gshan@redhat.com> The default block size is same as to the THP size, which is either retrieved from "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size" or hardcoded to 2MB. There are flaws in both mechanisms and this intends to fix them up. * When "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size" is used to getting the THP size, 32MB and 512MB are valid values when we have 16KB and 64KB page size on ARM64. * When the hardcoded THP size is used, 2MB, 32MB and 512MB are valid values when we have 4KB, 16KB and 64KB page sizes on ARM64. Co-developed-by: David Hildenbrand <david@redhat.com> Signed-off-by: Gavin Shan <gshan@redhat.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Reviewed-by: David Hildenbrand <david@redhat.com> Message-id: 20220111063329.74447-2-gshan@redhat.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/virtio/virtio-mem.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -XXX,XX +XXX,XX @@ */ #define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)(1 * MiB)) -#if defined(__x86_64__) || defined(__arm__) || defined(__aarch64__) || \ - defined(__powerpc64__) -#define VIRTIO_MEM_DEFAULT_THP_SIZE ((uint32_t)(2 * MiB)) -#else - /* fallback to 1 MiB (e.g., the THP size on s390x) */ -#define VIRTIO_MEM_DEFAULT_THP_SIZE VIRTIO_MEM_MIN_BLOCK_SIZE +static uint32_t virtio_mem_default_thp_size(void) +{ + uint32_t default_thp_size = VIRTIO_MEM_MIN_BLOCK_SIZE; + +#if defined(__x86_64__) || defined(__arm__) || defined(__powerpc64__) + default_thp_size = 2 * MiB; +#elif defined(__aarch64__) + if (qemu_real_host_page_size == 4 * KiB) { + default_thp_size = 2 * MiB; + } else if (qemu_real_host_page_size == 16 * KiB) { + default_thp_size = 32 * MiB; + } else if (qemu_real_host_page_size == 64 * KiB) { + default_thp_size = 512 * MiB; + } #endif + return default_thp_size; +} + /* * We want to have a reasonable default block size such that * 1. We avoid splitting THPs when unplugging memory, which degrades @@ -XXX,XX +XXX,XX @@ static uint32_t virtio_mem_thp_size(void) if (g_file_get_contents(HPAGE_PMD_SIZE_PATH, &content, NULL, NULL) && !qemu_strtou64(content, &endptr, 0, &tmp) && (!endptr || *endptr == '\n')) { - /* - * Sanity-check the value, if it's too big (e.g., aarch64 with 64k base - * pages) or weird, fallback to something smaller. - */ - if (!tmp || !is_power_of_2(tmp) || tmp > 16 * MiB) { + /* Sanity-check the value and fallback to something reasonable. */ + if (!tmp || !is_power_of_2(tmp)) { warn_report("Read unsupported THP size: %" PRIx64, tmp); } else { thp_size = tmp; @@ -XXX,XX +XXX,XX @@ static uint32_t virtio_mem_thp_size(void) } if (!thp_size) { - thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE; + thp_size = virtio_mem_default_thp_size(); warn_report("Could not detect THP size, falling back to %" PRIx64 " MiB.", thp_size / MiB); } -- 2.25.1 From: Gavin Shan <gshan@redhat.com> This supports virtio-mem-pci device on "virt" platform, by simply following the implementation on x86. * This implements the hotplug handlers to support virtio-mem-pci device hot-add, while the hot-remove isn't supported as we have on x86. * The block size is 512MB on ARM64 instead of 128MB on x86. * It has been passing the tests with various combinations like 64KB and 4KB page sizes on host and guest, different memory device backends like normal, transparent huge page and HugeTLB, plus migration. Co-developed-by: David Hildenbrand <david@redhat.com> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Gavin Shan <gshan@redhat.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Reviewed-by: David Hildenbrand <david@redhat.com> Message-id: 20220111063329.74447-3-gshan@redhat.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 70 ++++++++++++++++++++++++++++++++++++++++++ hw/virtio/virtio-mem.c | 4 ++- hw/arm/Kconfig | 1 + 3 files changed, 74 insertions(+), 1 deletion(-) 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 @@ #include "hw/arm/smmuv3.h" #include "hw/acpi/acpi.h" #include "target/arm/internals.h" +#include "hw/mem/memory-device.h" #include "hw/mem/pc-dimm.h" #include "hw/mem/nvdimm.h" #include "hw/acpi/generic_event_device.h" +#include "hw/virtio/virtio-mem-pci.h" #include "hw/virtio/virtio-iommu.h" #include "hw/char/pl011.h" #include "qemu/guest-random.h" @@ -XXX,XX +XXX,XX @@ static void virt_memory_plug(HotplugHandler *hotplug_dev, dev, &error_abort); } +static void virt_virtio_md_pci_pre_plug(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev); + Error *local_err = NULL; + + if (!hotplug_dev2 && dev->hotplugged) { + /* + * Without a bus hotplug handler, we cannot control the plug/unplug + * order. We should never reach this point when hotplugging on ARM. + * However, it's nice to add a safety net, similar to what we have + * on x86. + */ + error_setg(errp, "hotplug of virtio based memory devices not supported" + " on this bus."); + return; + } + /* + * First, see if we can plug this memory device at all. If that + * succeeds, branch of to the actual hotplug handler. + */ + memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev), NULL, + &local_err); + if (!local_err && hotplug_dev2) { + hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err); + } + error_propagate(errp, local_err); +} + +static void virt_virtio_md_pci_plug(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev); + Error *local_err = NULL; + + /* + * Plug the memory device first and then branch off to the actual + * hotplug handler. If that one fails, we can easily undo the memory + * device bits. + */ + memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev)); + if (hotplug_dev2) { + hotplug_handler_plug(hotplug_dev2, dev, &local_err); + if (local_err) { + memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev)); + } + } + error_propagate(errp, local_err); +} + +static void virt_virtio_md_pci_unplug_request(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + /* We don't support hot unplug of virtio based memory devices */ + error_setg(errp, "virtio based memory devices cannot be unplugged."); +} + + static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { @@ -XXX,XX +XXX,XX @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { virt_memory_pre_plug(hotplug_dev, dev, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) { + virt_virtio_md_pci_pre_plug(hotplug_dev, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { hwaddr db_start = 0, db_end = 0; char *resv_prop_str; @@ -XXX,XX +XXX,XX @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev, if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { virt_memory_plug(hotplug_dev, dev, errp); } + + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) { + virt_virtio_md_pci_plug(hotplug_dev, dev, errp); + } + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { PCIDevice *pdev = PCI_DEVICE(dev); @@ -XXX,XX +XXX,XX @@ static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, { if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { virt_dimm_unplug_request(hotplug_dev, dev, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) { + virt_virtio_md_pci_unplug_request(hotplug_dev, dev, errp); } else { error_setg(errp, "device unplug request for unsupported device" " type: %s", object_get_typename(OBJECT(dev))); @@ -XXX,XX +XXX,XX @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine, if (device_is_dynamic_sysbus(mc, dev) || object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || + object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI) || object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { return HOTPLUG_HANDLER(machine); } diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -XXX,XX +XXX,XX @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) * The memory block size corresponds mostly to the section size. * * This allows e.g., to add 20MB with a section size of 128MB on x86_64, and - * a section size of 1GB on arm64 (as long as the start address is properly + * a section size of 512MB on arm64 (as long as the start address is properly * aligned, similar to ordinary DIMMs). * * We can change this at any time and maybe even make it configurable if @@ -XXX,XX +XXX,XX @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) */ #if defined(TARGET_X86_64) || defined(TARGET_I386) #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB)) +#elif defined(TARGET_ARM) +#define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB)) #else #error VIRTIO_MEM_USABLE_EXTENT not defined #endif diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -XXX,XX +XXX,XX @@ config ARM_VIRT select ACPI_HW_REDUCED select ACPI_APEI select ACPI_VIOT + select VIRTIO_MEM_SUPPORTED config CHEETAH bool -- 2.25.1 From: Petr Pavlu <petr.pavlu@suse.com> Implement support for reading GICC_IIDR. This register is used by the Linux kernel to recognize that GICv2 with GICC_APRn is present. Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> Message-id: 20220113151916.17978-2-ppavlu@suse.cz Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gic.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -XXX,XX +XXX,XX @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset, } break; } + case 0xfc: + if (s->revision == REV_11MPCORE) { + /* Reserved on 11MPCore */ + *data = 0; + } else { + /* GICv1 or v2; Arm implementation */ + *data = (s->revision << 16) | 0x43b; + } + break; default: qemu_log_mask(LOG_GUEST_ERROR, "gic_cpu_read: Bad offset %x\n", (int)offset); -- 2.25.1 From: Petr Pavlu <petr.pavlu@suse.com> When running Linux on a machine with GICv2, the kernel can crash while processing an interrupt and can subsequently start a kdump kernel from the active interrupt handler. In such a case, the crashed kernel might not gracefully signal the end of interrupt to the GICv2 hardware. The kdump kernel will however try to reset the GIC state on startup to get the controller into a sane state, in particular the kernel writes ones to GICD_ICACTIVERn and wipes out GICC_APRn to make sure that no interrupt is active. The patch adds a logic to recalculate the running priority when GICC_APRn/GICC_NSAPRn is written which makes sure that the mentioned reset works with the GICv2 emulation in QEMU too and the kdump kernel starts receiving interrupts. The described scenario can be reproduced on an AArch64 QEMU virt machine with a kdump-enabled Linux system by using the softdog module. The kdump kernel will hang at some point because QEMU still thinks the running priority is that of the timer interrupt and asserts no new interrupts to the system: $ modprobe softdog soft_margin=10 soft_panic=1 $ cat > /dev/watchdog [Press Enter to start the watchdog, wait for its timeout and observe that the kdump kernel hangs on startup.] Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> Message-id: 20220113151916.17978-3-ppavlu@suse.cz Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gic.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -XXX,XX +XXX,XX @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset, } else { s->apr[regno][cpu] = value; } + s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu); break; } case 0xe0: case 0xe4: case 0xe8: case 0xec: @@ -XXX,XX +XXX,XX @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset, return MEMTX_OK; } s->nsapr[regno][cpu] = value; + s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu); break; } case 0x1000: -- 2.25.1 From: Marc Zyngier <maz@kernel.org> Just like we can control the enablement of the highmem PCIe ECAM region using highmem_ecam, let's add a control for the highmem PCIe MMIO region. Similarily to highmem_ecam, this region is disabled when highmem is off. Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Eric Auger <eric.auger@redhat.com> Message-id: 20220114140741.1358263-2-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/virt.h | 1 + hw/arm/virt-acpi-build.c | 10 ++++------ hw/arm/virt.c | 7 +++++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -XXX,XX +XXX,XX @@ struct VirtMachineState { bool secure; bool highmem; bool highmem_ecam; + bool highmem_mmio; bool its; bool tcg_its; bool virt; diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -XXX,XX +XXX,XX @@ static void acpi_dsdt_add_virtio(Aml *scope, } static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, - uint32_t irq, bool use_highmem, bool highmem_ecam, - VirtMachineState *vms) + uint32_t irq, VirtMachineState *vms) { - int ecam_id = VIRT_ECAM_ID(highmem_ecam); + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); struct GPEXConfig cfg = { .mmio32 = memmap[VIRT_PCIE_MMIO], .pio = memmap[VIRT_PCIE_PIO], @@ -XXX,XX +XXX,XX @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, .bus = vms->bus, }; - if (use_highmem) { + if (vms->highmem_mmio) { cfg.mmio64 = memmap[VIRT_HIGH_PCIE_MMIO]; } @@ -XXX,XX +XXX,XX @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]); acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO], (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS); - acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), - vms->highmem, vms->highmem_ecam, vms); + acpi_dsdt_add_pci(scope, memmap, irqmap[VIRT_PCIE] + ARM_SPI_BASE, vms); if (vms->acpi_dev) { build_ged_aml(scope, "\\_SB."GED_DEVICE, HOTPLUG_HANDLER(vms->acpi_dev), 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 void create_pcie(VirtMachineState *vms) mmio_reg, base_mmio, size_mmio); memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias); - if (vms->highmem) { + if (vms->highmem_mmio) { /* Map high MMIO space */ MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1); @@ -XXX,XX +XXX,XX @@ static void create_pcie(VirtMachineState *vms) qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", 2, base_ecam, 2, size_ecam); - if (vms->highmem) { + if (vms->highmem_mmio) { qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges", 1, FDT_PCI_RANGE_IOPORT, 2, 0, 2, base_pio, 2, size_pio, @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem); + vms->highmem_mmio &= vms->highmem; + create_gic(vms, sysmem); virt_cpu_post_init(vms, sysmem); @@ -XXX,XX +XXX,XX @@ static void virt_instance_init(Object *obj) vms->gic_version = VIRT_GIC_VERSION_NOSEL; vms->highmem_ecam = !vmc->no_highmem_ecam; + vms->highmem_mmio = true; if (vmc->no_its) { vms->its = false; -- 2.25.1 From: Marc Zyngier <maz@kernel.org> Just like we can control the enablement of the highmem PCIe region using highmem_ecam, let's add a control for the highmem GICv3 redistributor region. Similarily to highmem_ecam, these redistributors are disabled when highmem is off. Reviewed-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Eric Auger <eric.auger@redhat.com> Message-id: 20220114140741.1358263-3-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/virt.h | 4 +++- hw/arm/virt-acpi-build.c | 2 ++ hw/arm/virt.c | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -XXX,XX +XXX,XX @@ struct VirtMachineState { bool highmem; bool highmem_ecam; bool highmem_mmio; + bool highmem_redists; bool its; bool tcg_its; bool virt; @@ -XXX,XX +XXX,XX @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms) assert(vms->gic_version == VIRT_GIC_VERSION_3); - return MACHINE(vms)->smp.cpus > redist0_capacity ? 2 : 1; + return (MACHINE(vms)->smp.cpus > redist0_capacity && + vms->highmem_redists) ? 2 : 1; } #endif /* QEMU_ARM_VIRT_H */ diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -XXX,XX +XXX,XX @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) acpi_add_table(table_offsets, tables_blob); build_fadt_rev5(tables_blob, tables->linker, vms, dsdt); + vms->highmem_redists &= vms->highmem; + acpi_add_table(table_offsets, tables_blob); build_madt(tables_blob, tables->linker, vms); 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 void machvirt_init(MachineState *machine) virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem); vms->highmem_mmio &= vms->highmem; + vms->highmem_redists &= vms->highmem; create_gic(vms, sysmem); @@ -XXX,XX +XXX,XX @@ static void virt_instance_init(Object *obj) vms->highmem_ecam = !vmc->no_highmem_ecam; vms->highmem_mmio = true; + vms->highmem_redists = true; if (vmc->no_its) { vms->its = false; -- 2.25.1 From: Marc Zyngier <maz@kernel.org> Even when the VM is configured with highmem=off, the highest_gpa field includes devices that are above the 4GiB limit. Similarily, nothing seem to check that the memory is within the limit set by the highmem=off option. This leads to failures in virt_kvm_type() on systems that have a crippled IPA range, as the reported IPA space is larger than what it should be. Instead, honor the user-specified limit to only use the devices at the lowest end of the spectrum, and fail if we have memory crossing the 4GiB limit. Reviewed-by: Andrew Jones <drjones@redhat.com> Reviewed-by: Eric Auger <eric.auger@redhat.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Message-id: 20220114140741.1358263-4-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) static void virt_set_memmap(VirtMachineState *vms) { MachineState *ms = MACHINE(vms); - hwaddr base, device_memory_base, device_memory_size; + hwaddr base, device_memory_base, device_memory_size, memtop; int i; vms->memmap = extended_memmap; @@ -XXX,XX +XXX,XX @@ static void virt_set_memmap(VirtMachineState *vms) device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB; /* Base address of the high IO region */ - base = device_memory_base + ROUND_UP(device_memory_size, GiB); + memtop = base = device_memory_base + ROUND_UP(device_memory_size, GiB); + if (!vms->highmem && memtop > 4 * GiB) { + error_report("highmem=off, but memory crosses the 4GiB limit\n"); + exit(EXIT_FAILURE); + } if (base < device_memory_base) { error_report("maxmem/slots too huge"); exit(EXIT_FAILURE); @@ -XXX,XX +XXX,XX @@ static void virt_set_memmap(VirtMachineState *vms) vms->memmap[i].size = size; base += size; } - vms->highest_gpa = base - 1; + vms->highest_gpa = (vms->highmem ? base : memtop) - 1; if (device_memory_size > 0) { ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); ms->device_memory->base = device_memory_base; -- 2.25.1 From: Marc Zyngier <maz@kernel.org> The highmem attribute is nothing but another way to express the PA range of a VM. To support HW that has a smaller PA range then what QEMU assumes, pass this PA range to the virt_set_memmap() function, allowing it to correctly exclude highmem devices if they are outside of the PA range. Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Eric Auger <eric.auger@redhat.com> Message-id: 20220114140741.1358263-5-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 64 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 12 deletions(-) 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 uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) return arm_cpu_mp_affinity(idx, clustersz); } -static void virt_set_memmap(VirtMachineState *vms) +static void virt_set_memmap(VirtMachineState *vms, int pa_bits) { MachineState *ms = MACHINE(vms); hwaddr base, device_memory_base, device_memory_size, memtop; @@ -XXX,XX +XXX,XX @@ static void virt_set_memmap(VirtMachineState *vms) exit(EXIT_FAILURE); } + /* + * !highmem is exactly the same as limiting the PA space to 32bit, + * irrespective of the underlying capabilities of the HW. + */ + if (!vms->highmem) { + pa_bits = 32; + } + /* * We compute the base of the high IO region depending on the * amount of initial and device memory. The device memory start/size @@ -XXX,XX +XXX,XX @@ static void virt_set_memmap(VirtMachineState *vms) /* Base address of the high IO region */ memtop = base = device_memory_base + ROUND_UP(device_memory_size, GiB); - if (!vms->highmem && memtop > 4 * GiB) { - error_report("highmem=off, but memory crosses the 4GiB limit\n"); + if (memtop > BIT_ULL(pa_bits)) { + error_report("Addressing limited to %d bits, but memory exceeds it by %llu bytes\n", + pa_bits, memtop - BIT_ULL(pa_bits)); exit(EXIT_FAILURE); } if (base < device_memory_base) { @@ -XXX,XX +XXX,XX @@ static void virt_set_memmap(VirtMachineState *vms) vms->memmap[i].size = size; base += size; } - vms->highest_gpa = (vms->highmem ? base : memtop) - 1; + + /* + * If base fits within pa_bits, all good. If it doesn't, limit it + * to the end of RAM, which is guaranteed to fit within pa_bits. + */ + vms->highest_gpa = (base <= BIT_ULL(pa_bits) ? base : memtop) - 1; + if (device_memory_size > 0) { ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); ms->device_memory->base = device_memory_base; @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) unsigned int smp_cpus = machine->smp.cpus; unsigned int max_cpus = machine->smp.max_cpus; + if (!cpu_type_valid(machine->cpu_type)) { + error_report("mach-virt: CPU type %s not supported", machine->cpu_type); + exit(1); + } + + possible_cpus = mc->possible_cpu_arch_ids(machine); + /* * In accelerated mode, the memory map is computed earlier in kvm_type() * to create a VM with the right number of IPA bits. */ if (!vms->memmap) { - virt_set_memmap(vms); + Object *cpuobj; + ARMCPU *armcpu; + int pa_bits; + + /* + * Instanciate a temporary CPU object to find out about what + * we are about to deal with. Once this is done, get rid of + * the object. + */ + cpuobj = object_new(possible_cpus->cpus[0].type); + armcpu = ARM_CPU(cpuobj); + + if (object_property_get_bool(cpuobj, "aarch64", NULL)) { + pa_bits = arm_pamax(armcpu); + } else if (arm_feature(&armcpu->env, ARM_FEATURE_LPAE)) { + /* v7 with LPAE */ + pa_bits = 40; + } else { + /* Anything else */ + pa_bits = 32; + } + + object_unref(cpuobj); + + virt_set_memmap(vms, pa_bits); } /* We can probe only here because during property set @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) */ finalize_gic_version(vms); - if (!cpu_type_valid(machine->cpu_type)) { - error_report("mach-virt: CPU type %s not supported", machine->cpu_type); - exit(1); - } - if (vms->secure) { /* * The Secure view of the world is the same as the NonSecure, @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) create_fdt(vms); - possible_cpus = mc->possible_cpu_arch_ids(machine); assert(possible_cpus->len == max_cpus); for (n = 0; n < possible_cpus->len; n++) { Object *cpuobj; @@ -XXX,XX +XXX,XX @@ static int virt_kvm_type(MachineState *ms, const char *type_str) max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa); /* we freeze the memory map to compute the highest gpa */ - virt_set_memmap(vms); + virt_set_memmap(vms, max_vm_pa_size); requested_pa_size = 64 - clz64(vms->highest_gpa); -- 2.25.1 From: Marc Zyngier <maz@kernel.org> In order to only keep the highmem devices that actually fit in the PA range, check their location against the range and update highest_gpa if they fit. If they don't, mark them as disabled. Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Eric Auger <eric.auger@redhat.com> Message-id: 20220114140741.1358263-6-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) 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 void virt_set_memmap(VirtMachineState *vms, int pa_bits) base = vms->memmap[VIRT_MEM].base + LEGACY_RAMLIMIT_BYTES; } + /* We know for sure that at least the memory fits in the PA space */ + vms->highest_gpa = memtop - 1; + for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { hwaddr size = extended_memmap[i].size; + bool fits; base = ROUND_UP(base, size); vms->memmap[i].base = base; vms->memmap[i].size = size; + + /* + * Check each device to see if they fit in the PA space, + * moving highest_gpa as we go. + * + * For each device that doesn't fit, disable it. + */ + fits = (base + size) <= BIT_ULL(pa_bits); + if (fits) { + vms->highest_gpa = base + size - 1; + } + + switch (i) { + case VIRT_HIGH_GIC_REDIST2: + vms->highmem_redists &= fits; + break; + case VIRT_HIGH_PCIE_ECAM: + vms->highmem_ecam &= fits; + break; + case VIRT_HIGH_PCIE_MMIO: + vms->highmem_mmio &= fits; + break; + } + base += size; } - /* - * If base fits within pa_bits, all good. If it doesn't, limit it - * to the end of RAM, which is guaranteed to fit within pa_bits. - */ - vms->highest_gpa = (base <= BIT_ULL(pa_bits) ? base : memtop) - 1; - if (device_memory_size > 0) { ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); ms->device_memory->base = device_memory_base; -- 2.25.1 From: Marc Zyngier <maz@kernel.org> Now that the devices present in the extended memory map are checked against the available PA space and disabled when they don't fit, there is no need to keep the same checks against highmem, as highmem really is a shortcut for the PA space being 32bit. Reviewed-by: Eric Auger <eric.auger@redhat.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Message-id: 20220114140741.1358263-7-maz@kernel.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt-acpi-build.c | 2 -- hw/arm/virt.c | 5 +---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -XXX,XX +XXX,XX @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) acpi_add_table(table_offsets, tables_blob); build_fadt_rev5(tables_blob, tables->linker, vms, dsdt); - vms->highmem_redists &= vms->highmem; - acpi_add_table(table_offsets, tables_blob); build_madt(tables_blob, tables->linker, vms); 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 void machvirt_init(MachineState *machine) virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem); - vms->highmem_mmio &= vms->highmem; - vms->highmem_redists &= vms->highmem; - create_gic(vms, sysmem); virt_cpu_post_init(vms, sysmem); @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) machine->ram_size, "mach-virt.tag"); } - vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); + vms->highmem_ecam &= (!firmware_loaded || aarch64); create_rtc(vms); -- 2.25.1 From: Patrick Venture <venture@google.com> Reviewed-by: Hao Wu <wuhaotsh@google.com> Signed-off-by: Patrick Venture <venture@google.com> Message-id: 20220111172338.1525587-1-venture@google.com Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/npcm7xx_boards.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/npcm7xx_boards.c +++ b/hw/arm/npcm7xx_boards.c @@ -XXX,XX +XXX,XX @@ static void kudo_bmc_i2c_init(NPCM7xxState *soc) { I2CSlave *i2c_mux; - i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), TYPE_PCA9548, 0x75); + i2c_mux = i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), + TYPE_PCA9548, 0x75); + + /* tmp105 is compatible with the lm75 */ + i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 4), "tmp105", 0x5c); + i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 5), "tmp105", 0x5c); + i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 6), "tmp105", 0x5c); + i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 7), "tmp105", 0x5c); + i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), TYPE_PCA9548, 0x77); i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), TYPE_PCA9548, 0x77); -- 2.25.1 From: Troy Lee <troy_lee@aspeedtech.com> Aspeed 2600 SDK enables I3C support by default. The I3C driver will try to reset the device controller and set it up through device address table register. This dummy model responds to these registers with default values as listed in the ast2600v10 datasheet chapter 54.2. This avoids a guest machine kernel panic due to referencing an invalid kernel address if the device address table register isn't set correctly. Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> Reviewed-by: Graeme Gregory <quic_ggregory@quicinc.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Tested-by: Graeme Gregory <quic_ggregory@quicinc.com> Message-id: 20220111084546.4145785-2-troy_lee@aspeedtech.com [PMM: tidied commit message] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/misc/aspeed_i3c.h | 48 +++++ hw/misc/aspeed_i3c.c | 381 +++++++++++++++++++++++++++++++++++ hw/misc/meson.build | 1 + hw/misc/trace-events | 6 + 4 files changed, 436 insertions(+) create mode 100644 include/hw/misc/aspeed_i3c.h create mode 100644 hw/misc/aspeed_i3c.c diff --git a/include/hw/misc/aspeed_i3c.h b/include/hw/misc/aspeed_i3c.h new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/include/hw/misc/aspeed_i3c.h @@ -XXX,XX +XXX,XX @@ +/* + * ASPEED I3C Controller + * + * Copyright (C) 2021 ASPEED Technology Inc. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ + +#ifndef ASPEED_I3C_H +#define ASPEED_I3C_H + +#include "hw/sysbus.h" + +#define TYPE_ASPEED_I3C "aspeed.i3c" +#define TYPE_ASPEED_I3C_DEVICE "aspeed.i3c.device" +OBJECT_DECLARE_TYPE(AspeedI3CState, AspeedI3CClass, ASPEED_I3C) + +#define ASPEED_I3C_NR_REGS (0x70 >> 2) +#define ASPEED_I3C_DEVICE_NR_REGS (0x300 >> 2) +#define ASPEED_I3C_NR_DEVICES 6 + +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI3CDevice, ASPEED_I3C_DEVICE) +typedef struct AspeedI3CDevice { + /* <private> */ + SysBusDevice parent; + + /* <public> */ + MemoryRegion mr; + qemu_irq irq; + + uint8_t id; + uint32_t regs[ASPEED_I3C_DEVICE_NR_REGS]; +} AspeedI3CDevice; + +typedef struct AspeedI3CState { + /* <private> */ + SysBusDevice parent; + + /* <public> */ + MemoryRegion iomem; + MemoryRegion iomem_container; + qemu_irq irq; + + uint32_t regs[ASPEED_I3C_NR_REGS]; + AspeedI3CDevice devices[ASPEED_I3C_NR_DEVICES]; +} AspeedI3CState; +#endif /* ASPEED_I3C_H */ diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/hw/misc/aspeed_i3c.c @@ -XXX,XX +XXX,XX @@ +/* + * ASPEED I3C Controller + * + * Copyright (C) 2021 ASPEED Technology Inc. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qemu/log.h" +#include "qemu/error-report.h" +#include "hw/misc/aspeed_i3c.h" +#include "hw/registerfields.h" +#include "hw/qdev-properties.h" +#include "qapi/error.h" +#include "migration/vmstate.h" +#include "trace.h" + +/* I3C Controller Registers */ +REG32(I3C1_REG0, 0x10) +REG32(I3C1_REG1, 0x14) + FIELD(I3C1_REG1, I2C_MODE, 0, 1) + FIELD(I3C1_REG1, SA_EN, 15, 1) +REG32(I3C2_REG0, 0x20) +REG32(I3C2_REG1, 0x24) + FIELD(I3C2_REG1, I2C_MODE, 0, 1) + FIELD(I3C2_REG1, SA_EN, 15, 1) +REG32(I3C3_REG0, 0x30) +REG32(I3C3_REG1, 0x34) + FIELD(I3C3_REG1, I2C_MODE, 0, 1) + FIELD(I3C3_REG1, SA_EN, 15, 1) +REG32(I3C4_REG0, 0x40) +REG32(I3C4_REG1, 0x44) + FIELD(I3C4_REG1, I2C_MODE, 0, 1) + FIELD(I3C4_REG1, SA_EN, 15, 1) +REG32(I3C5_REG0, 0x50) +REG32(I3C5_REG1, 0x54) + FIELD(I3C5_REG1, I2C_MODE, 0, 1) + FIELD(I3C5_REG1, SA_EN, 15, 1) +REG32(I3C6_REG0, 0x60) +REG32(I3C6_REG1, 0x64) + FIELD(I3C6_REG1, I2C_MODE, 0, 1) + FIELD(I3C6_REG1, SA_EN, 15, 1) + +/* I3C Device Registers */ +REG32(DEVICE_CTRL, 0x00) +REG32(DEVICE_ADDR, 0x04) +REG32(HW_CAPABILITY, 0x08) +REG32(COMMAND_QUEUE_PORT, 0x0c) +REG32(RESPONSE_QUEUE_PORT, 0x10) +REG32(RX_TX_DATA_PORT, 0x14) +REG32(IBI_QUEUE_STATUS, 0x18) +REG32(IBI_QUEUE_DATA, 0x18) +REG32(QUEUE_THLD_CTRL, 0x1c) +REG32(DATA_BUFFER_THLD_CTRL, 0x20) +REG32(IBI_QUEUE_CTRL, 0x24) +REG32(IBI_MR_REQ_REJECT, 0x2c) +REG32(IBI_SIR_REQ_REJECT, 0x30) +REG32(RESET_CTRL, 0x34) +REG32(SLV_EVENT_CTRL, 0x38) +REG32(INTR_STATUS, 0x3c) +REG32(INTR_STATUS_EN, 0x40) +REG32(INTR_SIGNAL_EN, 0x44) +REG32(INTR_FORCE, 0x48) +REG32(QUEUE_STATUS_LEVEL, 0x4c) +REG32(DATA_BUFFER_STATUS_LEVEL, 0x50) +REG32(PRESENT_STATE, 0x54) +REG32(CCC_DEVICE_STATUS, 0x58) +REG32(DEVICE_ADDR_TABLE_POINTER, 0x5c) + FIELD(DEVICE_ADDR_TABLE_POINTER, DEPTH, 16, 16) + FIELD(DEVICE_ADDR_TABLE_POINTER, ADDR, 0, 16) +REG32(DEV_CHAR_TABLE_POINTER, 0x60) +REG32(VENDOR_SPECIFIC_REG_POINTER, 0x6c) +REG32(SLV_MIPI_PID_VALUE, 0x70) +REG32(SLV_PID_VALUE, 0x74) +REG32(SLV_CHAR_CTRL, 0x78) +REG32(SLV_MAX_LEN, 0x7c) +REG32(MAX_READ_TURNAROUND, 0x80) +REG32(MAX_DATA_SPEED, 0x84) +REG32(SLV_DEBUG_STATUS, 0x88) +REG32(SLV_INTR_REQ, 0x8c) +REG32(DEVICE_CTRL_EXTENDED, 0xb0) +REG32(SCL_I3C_OD_TIMING, 0xb4) +REG32(SCL_I3C_PP_TIMING, 0xb8) +REG32(SCL_I2C_FM_TIMING, 0xbc) +REG32(SCL_I2C_FMP_TIMING, 0xc0) +REG32(SCL_EXT_LCNT_TIMING, 0xc8) +REG32(SCL_EXT_TERMN_LCNT_TIMING, 0xcc) +REG32(BUS_FREE_TIMING, 0xd4) +REG32(BUS_IDLE_TIMING, 0xd8) +REG32(I3C_VER_ID, 0xe0) +REG32(I3C_VER_TYPE, 0xe4) +REG32(EXTENDED_CAPABILITY, 0xe8) +REG32(SLAVE_CONFIG, 0xec) + +static const uint32_t ast2600_i3c_device_resets[ASPEED_I3C_DEVICE_NR_REGS] = { + [R_HW_CAPABILITY] = 0x000e00bf, + [R_QUEUE_THLD_CTRL] = 0x01000101, + [R_I3C_VER_ID] = 0x3130302a, + [R_I3C_VER_TYPE] = 0x6c633033, + [R_DEVICE_ADDR_TABLE_POINTER] = 0x00080280, + [R_DEV_CHAR_TABLE_POINTER] = 0x00020200, + [A_VENDOR_SPECIFIC_REG_POINTER] = 0x000000b0, + [R_SLV_MAX_LEN] = 0x00ff00ff, +}; + +static uint64_t aspeed_i3c_device_read(void *opaque, hwaddr offset, + unsigned size) +{ + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(opaque); + uint32_t addr = offset >> 2; + uint64_t value; + + switch (addr) { + case R_COMMAND_QUEUE_PORT: + value = 0; + break; + default: + value = s->regs[addr]; + break; + } + + trace_aspeed_i3c_device_read(s->id, offset, value); + + return value; +} + +static void aspeed_i3c_device_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(opaque); + uint32_t addr = offset >> 2; + + trace_aspeed_i3c_device_write(s->id, offset, value); + + switch (addr) { + case R_HW_CAPABILITY: + case R_RESPONSE_QUEUE_PORT: + case R_IBI_QUEUE_DATA: + case R_QUEUE_STATUS_LEVEL: + case R_PRESENT_STATE: + case R_CCC_DEVICE_STATUS: + case R_DEVICE_ADDR_TABLE_POINTER: + case R_VENDOR_SPECIFIC_REG_POINTER: + case R_SLV_CHAR_CTRL: + case R_SLV_MAX_LEN: + case R_MAX_READ_TURNAROUND: + case R_I3C_VER_ID: + case R_I3C_VER_TYPE: + case R_EXTENDED_CAPABILITY: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: write to readonly register[%02lx] = %08lx\n", + __func__, offset, value); + break; + case R_RX_TX_DATA_PORT: + break; + case R_RESET_CTRL: + break; + default: + s->regs[addr] = value; + break; + } +} + +static const VMStateDescription aspeed_i3c_device_vmstate = { + .name = TYPE_ASPEED_I3C, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]){ + VMSTATE_UINT32_ARRAY(regs, AspeedI3CDevice, ASPEED_I3C_DEVICE_NR_REGS), + VMSTATE_END_OF_LIST(), + } +}; + +static const MemoryRegionOps aspeed_i3c_device_ops = { + .read = aspeed_i3c_device_read, + .write = aspeed_i3c_device_write, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + +static void aspeed_i3c_device_reset(DeviceState *dev) +{ + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(dev); + + memcpy(s->regs, ast2600_i3c_device_resets, sizeof(s->regs)); +} + +static void aspeed_i3c_device_realize(DeviceState *dev, Error **errp) +{ + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(dev); + g_autofree char *name = g_strdup_printf(TYPE_ASPEED_I3C_DEVICE ".%d", + s->id); + + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); + + memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i3c_device_ops, + s, name, ASPEED_I3C_DEVICE_NR_REGS << 2); +} + +static uint64_t aspeed_i3c_read(void *opaque, hwaddr addr, unsigned int size) +{ + AspeedI3CState *s = ASPEED_I3C(opaque); + uint64_t val = 0; + + val = s->regs[addr >> 2]; + + trace_aspeed_i3c_read(addr, val); + + return val; +} + +static void aspeed_i3c_write(void *opaque, + hwaddr addr, + uint64_t data, + unsigned int size) +{ + AspeedI3CState *s = ASPEED_I3C(opaque); + + trace_aspeed_i3c_write(addr, data); + + addr >>= 2; + + /* I3C controller register */ + switch (addr) { + case R_I3C1_REG1: + case R_I3C2_REG1: + case R_I3C3_REG1: + case R_I3C4_REG1: + case R_I3C5_REG1: + case R_I3C6_REG1: + if (data & R_I3C1_REG1_I2C_MODE_MASK) { + qemu_log_mask(LOG_UNIMP, + "%s: Not support I2C mode [%08lx]=%08lx", + __func__, addr << 2, data); + break; + } + if (data & R_I3C1_REG1_SA_EN_MASK) { + qemu_log_mask(LOG_UNIMP, + "%s: Not support slave mode [%08lx]=%08lx", + __func__, addr << 2, data); + break; + } + s->regs[addr] = data; + break; + default: + s->regs[addr] = data; + break; + } +} + +static const MemoryRegionOps aspeed_i3c_ops = { + .read = aspeed_i3c_read, + .write = aspeed_i3c_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 4, + } +}; + +static void aspeed_i3c_reset(DeviceState *dev) +{ + AspeedI3CState *s = ASPEED_I3C(dev); + memset(s->regs, 0, sizeof(s->regs)); +} + +static void aspeed_i3c_instance_init(Object *obj) +{ + AspeedI3CState *s = ASPEED_I3C(obj); + int i; + + for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) { + object_initialize_child(obj, "device[*]", &s->devices[i], + TYPE_ASPEED_I3C_DEVICE); + } +} + +static void aspeed_i3c_realize(DeviceState *dev, Error **errp) +{ + int i; + AspeedI3CState *s = ASPEED_I3C(dev); + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + + memory_region_init(&s->iomem_container, OBJECT(s), + TYPE_ASPEED_I3C ".container", 0x8000); + + sysbus_init_mmio(sbd, &s->iomem_container); + + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s, + TYPE_ASPEED_I3C ".regs", ASPEED_I3C_NR_REGS << 2); + + memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem); + + for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) { + Object *dev = OBJECT(&s->devices[i]); + + if (!object_property_set_uint(dev, "device-id", i, errp)) { + return; + } + + if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) { + return; + } + + /* + * Register Address of I3CX Device = + * (Base Address of Global Register) + (Offset of I3CX) + Offset + * X = 0, 1, 2, 3, 4, 5 + * Offset of I3C0 = 0x2000 + * Offset of I3C1 = 0x3000 + * Offset of I3C2 = 0x4000 + * Offset of I3C3 = 0x5000 + * Offset of I3C4 = 0x6000 + * Offset of I3C5 = 0x7000 + */ + memory_region_add_subregion(&s->iomem_container, + 0x2000 + i * 0x1000, &s->devices[i].mr); + } + +} + +static Property aspeed_i3c_device_properties[] = { + DEFINE_PROP_UINT8("device-id", AspeedI3CDevice, id, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +static void aspeed_i3c_device_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->desc = "Aspeed I3C Device"; + dc->realize = aspeed_i3c_device_realize; + dc->reset = aspeed_i3c_device_reset; + device_class_set_props(dc, aspeed_i3c_device_properties); +} + +static const TypeInfo aspeed_i3c_device_info = { + .name = TYPE_ASPEED_I3C_DEVICE, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(AspeedI3CDevice), + .class_init = aspeed_i3c_device_class_init, +}; + +static const VMStateDescription vmstate_aspeed_i3c = { + .name = TYPE_ASPEED_I3C, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(regs, AspeedI3CState, ASPEED_I3C_NR_REGS), + VMSTATE_STRUCT_ARRAY(devices, AspeedI3CState, ASPEED_I3C_NR_DEVICES, 1, + aspeed_i3c_device_vmstate, AspeedI3CDevice), + VMSTATE_END_OF_LIST(), + } +}; + +static void aspeed_i3c_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = aspeed_i3c_realize; + dc->reset = aspeed_i3c_reset; + dc->desc = "Aspeed I3C Controller"; + dc->vmsd = &vmstate_aspeed_i3c; +} + +static const TypeInfo aspeed_i3c_info = { + .name = TYPE_ASPEED_I3C, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_init = aspeed_i3c_instance_init, + .instance_size = sizeof(AspeedI3CState), + .class_init = aspeed_i3c_class_init, +}; + +static void aspeed_i3c_register_types(void) +{ + type_register_static(&aspeed_i3c_device_info); + type_register_static(&aspeed_i3c_info); +} + +type_init(aspeed_i3c_register_types); diff --git a/hw/misc/meson.build b/hw/misc/meson.build index XXXXXXX..XXXXXXX 100644 --- a/hw/misc/meson.build +++ b/hw/misc/meson.build @@ -XXX,XX +XXX,XX @@ softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c')) softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c')) softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files( 'aspeed_hace.c', + 'aspeed_i3c.c', 'aspeed_lpc.c', 'aspeed_scu.c', 'aspeed_sdmc.c', diff --git a/hw/misc/trace-events b/hw/misc/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/misc/trace-events +++ b/hw/misc/trace-events @@ -XXX,XX +XXX,XX @@ armsse_mhu_write(uint64_t offset, uint64_t data, unsigned size) "SSE-200 MHU wri # aspeed_xdma.c aspeed_xdma_write(uint64_t offset, uint64_t data) "XDMA write: offset 0x%" PRIx64 " data 0x%" PRIx64 +# aspeed_i3c.c +aspeed_i3c_read(uint64_t offset, uint64_t data) "I3C read: offset 0x%" PRIx64 " data 0x%" PRIx64 +aspeed_i3c_write(uint64_t offset, uint64_t data) "I3C write: offset 0x%" PRIx64 " data 0x%" PRIx64 +aspeed_i3c_device_read(uint32_t deviceid, uint64_t offset, uint64_t data) "I3C Dev[%u] read: offset 0x%" PRIx64 " data 0x%" PRIx64 +aspeed_i3c_device_write(uint32_t deviceid, uint64_t offset, uint64_t data) "I3C Dev[%u] write: offset 0x%" PRIx64 " data 0x%" PRIx64 + # bcm2835_property.c bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) "mbox property tag:0x%08x in_sz:%u out_sz:%zu" -- 2.25.1 From: Troy Lee <troy_lee@aspeedtech.com> Add the new i3c device to the AST2600 SoC. Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> Reviewed-by: Graeme Gregory <quic_ggregory@quicinc.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Tested-by: Graeme Gregory <quic_ggregory@quicinc.com> Message-id: 20220111084546.4145785-3-troy_lee@aspeedtech.com [PMM: tidied commit message] Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/aspeed_soc.h | 3 +++ hw/arm/aspeed_ast2600.c | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/arm/aspeed_soc.h +++ b/include/hw/arm/aspeed_soc.h @@ -XXX,XX +XXX,XX @@ #include "hw/timer/aspeed_timer.h" #include "hw/rtc/aspeed_rtc.h" #include "hw/i2c/aspeed_i2c.h" +#include "hw/misc/aspeed_i3c.h" #include "hw/ssi/aspeed_smc.h" #include "hw/misc/aspeed_hace.h" #include "hw/watchdog/wdt_aspeed.h" @@ -XXX,XX +XXX,XX @@ struct AspeedSoCState { AspeedRtcState rtc; AspeedTimerCtrlState timerctrl; AspeedI2CState i2c; + AspeedI3CState i3c; AspeedSCUState scu; AspeedHACEState hace; AspeedXDMAState xdma; @@ -XXX,XX +XXX,XX @@ enum { ASPEED_DEV_HACE, ASPEED_DEV_DPMCU, ASPEED_DEV_DP, + ASPEED_DEV_I3C, }; #endif /* ASPEED_SOC_H */ diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/aspeed_ast2600.c +++ b/hw/arm/aspeed_ast2600.c @@ -XXX,XX +XXX,XX @@ static const hwaddr aspeed_soc_ast2600_memmap[] = { [ASPEED_DEV_UART1] = 0x1E783000, [ASPEED_DEV_UART5] = 0x1E784000, [ASPEED_DEV_VUART] = 0x1E787000, + [ASPEED_DEV_I3C] = 0x1E7A0000, [ASPEED_DEV_SDRAM] = 0x80000000, }; @@ -XXX,XX +XXX,XX @@ static const int aspeed_soc_ast2600_irqmap[] = { [ASPEED_DEV_ETH4] = 33, [ASPEED_DEV_KCS] = 138, /* 138 -> 142 */ [ASPEED_DEV_DP] = 62, + [ASPEED_DEV_I3C] = 102, /* 102 -> 107 */ }; static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl) @@ -XXX,XX +XXX,XX @@ static void aspeed_soc_ast2600_init(Object *obj) snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname); object_initialize_child(obj, "hace", &s->hace, typename); + + object_initialize_child(obj, "i3c", &s->i3c, TYPE_ASPEED_I3C); } /* @@ -XXX,XX +XXX,XX @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) sysbus_mmio_map(SYS_BUS_DEVICE(&s->hace), 0, sc->memmap[ASPEED_DEV_HACE]); sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0, aspeed_soc_get_irq(s, ASPEED_DEV_HACE)); + + /* I3C */ + if (!sysbus_realize(SYS_BUS_DEVICE(&s->i3c), errp)) { + return; + } + sysbus_mmio_map(SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]); + for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) { + qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), + sc->irqmap[ASPEED_DEV_I3C] + i); + /* The AST2600 I3C controller has one IRQ per bus. */ + sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq); + } } static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) -- 2.25.1 In process_its_cmd() and process_mapti() we must check the event ID against a limit defined by the size field in the DTE, which specifies the number of ID bits minus one. Convert this code to our num_foo convention: * change the variable names * use uint64_t and 1ULL when calculating the number of valid event IDs, because DTE.SIZE is 5 bits and so num_eventids may be up to 2^32 * fix the off-by-one error in the comparison Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Message-id: 20220111171048.3545974-2-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, MemTxResult res = MEMTX_OK; bool dte_valid; uint64_t dte = 0; - uint32_t max_eventid; + uint64_t num_eventids; uint16_t icid = 0; uint32_t pIntid = 0; bool ite_valid = false; @@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, dte_valid = FIELD_EX64(dte, DTE, VALID); if (dte_valid) { - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); @@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, dte_valid ? "valid" : "invalid", ite_valid ? "valid" : "invalid", cte_valid ? "valid" : "invalid"); - } else if (eventid > max_eventid) { + } else if (eventid >= num_eventids) { qemu_log_mask(LOG_GUEST_ERROR, - "%s: invalid command attributes: eventid %d > %d\n", - __func__, eventid, max_eventid); + "%s: invalid command attributes: eventid %d >= %" + PRId64 "\n", + __func__, eventid, num_eventids); } else { /* * Current implementation only supports rdbase == procnum @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, AddressSpace *as = &s->gicv3->dma_as; uint32_t devid, eventid; uint32_t pIntid = 0; - uint32_t max_eventid, max_Intid; + uint64_t num_eventids; + uint32_t max_Intid; bool dte_valid; MemTxResult res = MEMTX_OK; uint16_t icid = 0; @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, return result; } dte_valid = FIELD_EX64(dte, DTE, VALID); - max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1); + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids) - || !dte_valid || (eventid > max_eventid) || + || !dte_valid || (eventid >= num_eventids) || (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) && (pIntid != INTID_SPURIOUS))) { qemu_log_mask(LOG_GUEST_ERROR, -- 2.25.1 The bounds check on the number of interrupt IDs is correct, but doesn't match our convention; change the variable name, initialize it to the 2^n value rather than (2^n)-1, and use >= instead of > in the comparison. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Message-id: 20220111171048.3545974-3-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, uint32_t devid, eventid; uint32_t pIntid = 0; uint64_t num_eventids; - uint32_t max_Intid; + uint32_t num_intids; bool dte_valid; MemTxResult res = MEMTX_OK; uint16_t icid = 0; @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, } dte_valid = FIELD_EX64(dte, DTE, VALID); num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); - max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1; + num_intids = 1ULL << (GICD_TYPER_IDBITS + 1); if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids) || !dte_valid || (eventid >= num_eventids) || - (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) && + (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) && (pIntid != INTID_SPURIOUS))) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes " -- 2.25.1 process_its_cmd() returns a bool, like all the other process_ functions. However we were putting its return value into 'res', not 'result', which meant we would ignore it when deciding whether to continue or stall the command queue. Fix the typo. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20220111171048.3545974-4-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s) switch (cmd) { case GITS_CMD_INT: - res = process_its_cmd(s, data, cq_offset, INTERRUPT); + result = process_its_cmd(s, data, cq_offset, INTERRUPT); break; case GITS_CMD_CLEAR: - res = process_its_cmd(s, data, cq_offset, CLEAR); + result = process_its_cmd(s, data, cq_offset, CLEAR); break; case GITS_CMD_SYNC: /* -- 2.25.1 In process_cmdq(), we read 64 bits of the command packet, which contain the command identifier, which we then switch() on to dispatch to an appropriate sub-function. However, if address_space_ldq_le() reports a memory transaction failure, we still read the command identifier out of the data and switch() on it. Restructure the code so that we stop immediately (stalling the command queue) in this case. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-5-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s) data = address_space_ldq_le(as, s->cq.base_addr + cq_offset, MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - result = false; + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); + qemu_log_mask(LOG_GUEST_ERROR, + "%s: could not read command at 0x%" PRIx64 "\n", + __func__, s->cq.base_addr + cq_offset); + break; } + cmd = (data & CMD_MASK); switch (cmd) { -- 2.25.1 When an ITS detects an error in a command, it has an implementation-defined (CONSTRAINED UNPREDICTABLE) choice of whether to ignore the command, proceeding to the next one in the queue, or to stall the ITS command queue, processing nothing further. The behaviour required when the read of the command packet from memory fails is less clearly documented, but the same set of choices as for command errors seem reasonable. The intention of the QEMU implementation, as documented in the comments, is that if we encounter a memory error reading the command packet or one of the various data tables then we should stall, but for command parameter errors we should ignore the queue and continue. However, we don't actually do this. To get the desired behaviour, the various process_* functions need to return true to cause process_cmdq() to advance to the next command and keep processing, and false to stall command processing. What they mostly do is return false for any kind of error. To make the code clearer, replace the 'bool' return from the process_ functions with an enum which may be either CMD_STALL or CMD_CONTINUE. In this commit no behaviour changes; in subsequent commits we will adjust the error-return paths for the process_ functions one by one. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 20220111171048.3545974-6-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 59 ++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ typedef struct { uint64_t itel; } IteEntry; +/* + * The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options + * if a command parameter is not correct. These include both "stall + * processing of the command queue" and "ignore this command, and + * keep processing the queue". In our implementation we choose that + * memory transaction errors reading the command packet provoke a + * stall, but errors in parameters cause us to ignore the command + * and continue processing. + * The process_* functions which handle individual ITS commands all + * return an ItsCmdResult which tells process_cmdq() whether it should + * stall or keep going. + */ +typedef enum ItsCmdResult { + CMD_STALL = 0, + CMD_CONTINUE = 1, +} ItsCmdResult; + static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) { uint64_t result = 0; @@ -XXX,XX +XXX,XX @@ static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res) * 3. handling of ITS CLEAR command * 4. handling of ITS DISCARD command */ -static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, - ItsCmdType cmd) +static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, + uint32_t offset, ItsCmdType cmd) { AddressSpace *as = &s->gicv3->dma_as; uint32_t devid, eventid; @@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, bool ite_valid = false; uint64_t cte = 0; bool cte_valid = false; - bool result = false; + ItsCmdResult result = CMD_STALL; uint64_t rdbase; if (cmd == NONE) { @@ -XXX,XX +XXX,XX @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset, if (cmd == DISCARD) { IteEntry ite = {}; /* remove mapping from interrupt translation table */ - result = update_ite(s, eventid, dte, ite); + result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; } } return result; } -static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, - bool ignore_pInt) +static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, + uint32_t offset, bool ignore_pInt) { AddressSpace *as = &s->gicv3->dma_as; uint32_t devid, eventid; @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, MemTxResult res = MEMTX_OK; uint16_t icid = 0; uint64_t dte = 0; - bool result = false; + ItsCmdResult result = CMD_STALL; devid = ((value & DEVID_MASK) >> DEVID_SHIFT); offset += NUM_BYTES_IN_DW; @@ -XXX,XX +XXX,XX @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset, ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid); - result = update_ite(s, eventid, dte, ite); + result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; } return result; @@ -XXX,XX +XXX,XX @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid, } } -static bool process_mapc(GICv3ITSState *s, uint32_t offset) +static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset) { AddressSpace *as = &s->gicv3->dma_as; uint16_t icid; uint64_t rdbase; bool valid; MemTxResult res = MEMTX_OK; - bool result = false; + ItsCmdResult result = CMD_STALL; uint64_t value; offset += NUM_BYTES_IN_DW; @@ -XXX,XX +XXX,XX @@ static bool process_mapc(GICv3ITSState *s, uint32_t offset) * command in the queue */ } else { - result = update_cte(s, icid, valid, rdbase); + result = update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL; } return result; @@ -XXX,XX +XXX,XX @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid, } } -static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset) +static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, + uint32_t offset) { AddressSpace *as = &s->gicv3->dma_as; uint32_t devid; @@ -XXX,XX +XXX,XX @@ static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset) uint64_t itt_addr; bool valid; MemTxResult res = MEMTX_OK; - bool result = false; + ItsCmdResult result = CMD_STALL; devid = ((value & DEVID_MASK) >> DEVID_SHIFT); @@ -XXX,XX +XXX,XX @@ static bool process_mapd(GICv3ITSState *s, uint64_t value, uint32_t offset) * command in the queue */ } else { - result = update_dte(s, devid, valid, size, itt_addr); + result = update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL; } return result; @@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s) uint64_t data; AddressSpace *as = &s->gicv3->dma_as; MemTxResult res = MEMTX_OK; - bool result = true; uint8_t cmd; int i; @@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s) } while (wr_offset != rd_offset) { + ItsCmdResult result = CMD_CONTINUE; + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE); data = address_space_ldq_le(as, s->cq.base_addr + cq_offset, MEMTXATTRS_UNSPECIFIED, &res); @@ -XXX,XX +XXX,XX @@ static void process_cmdq(GICv3ITSState *s) default: break; } - if (result) { + if (result == CMD_CONTINUE) { rd_offset++; rd_offset %= s->cq.num_entries; s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset); } else { - /* - * in this implementation, in case of dma read/write error - * we stall the command processing - */ + /* CMD_STALL */ s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1); qemu_log_mask(LOG_GUEST_ERROR, - "%s: %x cmd processing failed\n", __func__, cmd); + "%s: 0x%x cmd processing failed, stalling\n", + __func__, cmd); break; } } -- 2.25.1 Fix process_its_cmd() to consistently return CMD_STALL for memory errors and CMD_CONTINUE for parameter errors, as we claim in the comments that we do. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-7-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, bool ite_valid = false; uint64_t cte = 0; bool cte_valid = false; - ItsCmdResult result = CMD_STALL; uint64_t rdbase; if (cmd == NONE) { @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, } if (res != MEMTX_OK) { - return result; + return CMD_STALL; } eventid = (value & EVENTID_MASK); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, dte = get_dte(s, devid, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } dte_valid = FIELD_EX64(dte, DTE, VALID); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } if (ite_valid) { @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, } if (res != MEMTX_OK) { - return result; + return CMD_STALL; } } else { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: " "invalid dte: %"PRIx64" for %d (MEM_TX: %d)\n", __func__, dte, devid, res); - return result; + return CMD_CONTINUE; } @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: devid %d>=%d", __func__, devid, s->dt.num_ids); - + return CMD_CONTINUE; } else if (!dte_valid || !ite_valid || !cte_valid) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: " @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, dte_valid ? "valid" : "invalid", ite_valid ? "valid" : "invalid", cte_valid ? "valid" : "invalid"); + return CMD_CONTINUE; } else if (eventid >= num_eventids) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: eventid %d >= %" PRId64 "\n", __func__, eventid, num_eventids); + return CMD_CONTINUE; } else { /* * Current implementation only supports rdbase == procnum @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, rdbase = FIELD_EX64(cte, CTE, RDBASE); if (rdbase >= s->gicv3->num_cpu) { - return result; + return CMD_CONTINUE; } if ((cmd == CLEAR) || (cmd == DISCARD)) { @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, if (cmd == DISCARD) { IteEntry ite = {}; /* remove mapping from interrupt translation table */ - result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; + return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; } + return CMD_CONTINUE; } - - return result; } static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, -- 2.25.1 Refactor process_its_cmd() so that it consistently uses the structure do thing; if (error condition) { return early; } do next thing; rather than doing some of the work nested inside if (not error) code blocks. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-8-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 103 +++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 53 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, } dte_valid = FIELD_EX64(dte, DTE, VALID); - if (dte_valid) { - num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); - - ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); - - if (res != MEMTX_OK) { - return CMD_STALL; - } - - if (ite_valid) { - cte_valid = get_cte(s, icid, &cte, &res); - } - - if (res != MEMTX_OK) { - return CMD_STALL; - } - } else { + if (!dte_valid) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: " - "invalid dte: %"PRIx64" for %d (MEM_TX: %d)\n", - __func__, dte, devid, res); + "invalid dte: %"PRIx64" for %d\n", + __func__, dte, devid); return CMD_CONTINUE; } + num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); + + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); + if (res != MEMTX_OK) { + return CMD_STALL; + } + + if (!ite_valid) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes: invalid ITE\n", + __func__); + return CMD_CONTINUE; + } + + cte_valid = get_cte(s, icid, &cte, &res); + if (res != MEMTX_OK) { + return CMD_STALL; + } + if (!cte_valid) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes: " + "invalid cte: %"PRIx64"\n", + __func__, cte); + return CMD_CONTINUE; + } - /* - * In this implementation, in case of guest errors we ignore the - * command and move onto the next command in the queue. - */ if (devid >= s->dt.num_ids) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: devid %d>=%d", __func__, devid, s->dt.num_ids); return CMD_CONTINUE; - } else if (!dte_valid || !ite_valid || !cte_valid) { - qemu_log_mask(LOG_GUEST_ERROR, - "%s: invalid command attributes: " - "dte: %s, ite: %s, cte: %s\n", - __func__, - dte_valid ? "valid" : "invalid", - ite_valid ? "valid" : "invalid", - cte_valid ? "valid" : "invalid"); - return CMD_CONTINUE; - } else if (eventid >= num_eventids) { + } + if (eventid >= num_eventids) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes: eventid %d >= %" PRId64 "\n", __func__, eventid, num_eventids); return CMD_CONTINUE; - } else { - /* - * Current implementation only supports rdbase == procnum - * Hence rdbase physical address is ignored - */ - rdbase = FIELD_EX64(cte, CTE, RDBASE); + } - if (rdbase >= s->gicv3->num_cpu) { - return CMD_CONTINUE; - } + /* + * Current implementation only supports rdbase == procnum + * Hence rdbase physical address is ignored + */ + rdbase = FIELD_EX64(cte, CTE, RDBASE); - if ((cmd == CLEAR) || (cmd == DISCARD)) { - gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0); - } else { - gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1); - } - - if (cmd == DISCARD) { - IteEntry ite = {}; - /* remove mapping from interrupt translation table */ - return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; - } + if (rdbase >= s->gicv3->num_cpu) { return CMD_CONTINUE; } + + if ((cmd == CLEAR) || (cmd == DISCARD)) { + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0); + } else { + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1); + } + + if (cmd == DISCARD) { + IteEntry ite = {}; + /* remove mapping from interrupt translation table */ + return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; + } + return CMD_CONTINUE; } static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, -- 2.25.1 Fix process_mapti() to consistently return CMD_STALL for memory errors and CMD_CONTINUE for parameter errors, as we claim in the comments that we do. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-9-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, MemTxResult res = MEMTX_OK; uint16_t icid = 0; uint64_t dte = 0; - ItsCmdResult result = CMD_STALL; + IteEntry ite = {}; devid = ((value & DEVID_MASK) >> DEVID_SHIFT); offset += NUM_BYTES_IN_DW; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } eventid = (value & EVENTID_MASK); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } icid = value & ICID_MASK; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, dte = get_dte(s, devid, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } dte_valid = FIELD_EX64(dte, DTE, VALID); num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, * we ignore this command and move onto the next * command in the queue */ - } else { - /* add ite entry to interrupt translation table */ - IteEntry ite = {}; - ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid); - ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL); - ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid); - ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); - ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid); - - result = update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; + return CMD_CONTINUE; } - return result; + /* add ite entry to interrupt translation table */ + ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, dte_valid); + ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL); + ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid); + ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); + ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid); + + return update_ite(s, eventid, dte, ite) ? CMD_CONTINUE : CMD_STALL; } static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid, -- 2.25.1 Fix process_mapc() to consistently return CMD_STALL for memory errors and CMD_CONTINUE for parameter errors, as we claim in the comments that we do. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-10-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset) uint64_t rdbase; bool valid; MemTxResult res = MEMTX_OK; - ItsCmdResult result = CMD_STALL; uint64_t value; offset += NUM_BYTES_IN_DW; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset) MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } icid = value & ICID_MASK; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset) * we ignore this command and move onto the next * command in the queue */ - } else { - result = update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL; + return CMD_CONTINUE; } - return result; + return update_cte(s, icid, valid, rdbase) ? CMD_CONTINUE : CMD_STALL; } static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid, -- 2.25.1 Fix process_mapd() to consistently return CMD_STALL for memory errors and CMD_CONTINUE for parameter errors, as we claim in the comments that we do. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-id: 20220111171048.3545974-11-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, uint64_t itt_addr; bool valid; MemTxResult res = MEMTX_OK; - ItsCmdResult result = CMD_STALL; devid = ((value & DEVID_MASK) >> DEVID_SHIFT); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } size = (value & SIZE_MASK); @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, MEMTXATTRS_UNSPECIFIED, &res); if (res != MEMTX_OK) { - return result; + return CMD_STALL; } itt_addr = (value & ITTADDR_MASK) >> ITTADDR_SHIFT; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, * we ignore this command and move onto the next * command in the queue */ - } else { - result = update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL; + return CMD_CONTINUE; } - return result; + return update_dte(s, devid, valid, size, itt_addr) ? CMD_CONTINUE : CMD_STALL; } /* -- 2.25.1 The ITS has several tables which all share a similar format, described by the TableDesc struct: the guest may configure them to be a single-level table or a two-level table. Currently we open-code the process of finding the table entry in all the functions which read or write the device table or the collection table. Factor out the "get the address of the table entry" logic into a new function, so that the code which needs to read or write a table entry only needs to call table_entry_addr() and then perform a suitable load or store to that address. Note that the error handling is slightly complicated because we want to handle two cases differently: * failure to read the L1 table entry should end up causing a command stall, like other kinds of DMA error * an L1 table entry that says there is no L2 table for this index (ie whose valid bit is 0) must result in us treating the table entry as not-valid on read, and discarding writes (this is mandated by the spec) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Message-id: 20220111171048.3545974-12-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 212 +++++++++++++--------------------------- 1 file changed, 70 insertions(+), 142 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static uint64_t baser_base_addr(uint64_t value, uint32_t page_sz) return result; } +static uint64_t table_entry_addr(GICv3ITSState *s, TableDesc *td, + uint32_t idx, MemTxResult *res) +{ + /* + * Given a TableDesc describing one of the ITS in-guest-memory + * tables and an index into it, return the guest address + * corresponding to that table entry. + * If there was a memory error reading the L1 table of an + * indirect table, *res is set accordingly, and we return -1. + * If the L1 table entry is marked not valid, we return -1 with + * *res set to MEMTX_OK. + * + * The specification defines the format of level 1 entries of a + * 2-level table, but the format of level 2 entries and the format + * of flat-mapped tables is IMPDEF. + */ + AddressSpace *as = &s->gicv3->dma_as; + uint32_t l2idx; + uint64_t l2; + uint32_t num_l2_entries; + + *res = MEMTX_OK; + + if (!td->indirect) { + /* Single level table */ + return td->base_addr + idx * td->entry_sz; + } + + /* Two level table */ + l2idx = idx / (td->page_sz / L1TABLE_ENTRY_SIZE); + + l2 = address_space_ldq_le(as, + td->base_addr + (l2idx * L1TABLE_ENTRY_SIZE), + MEMTXATTRS_UNSPECIFIED, res); + if (*res != MEMTX_OK) { + return -1; + } + if (!(l2 & L2_TABLE_VALID_MASK)) { + return -1; + } + + num_l2_entries = td->page_sz / td->entry_sz; + return (l2 & ((1ULL << 51) - 1)) + (idx % num_l2_entries) * td->entry_sz; +} + static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte, MemTxResult *res) { AddressSpace *as = &s->gicv3->dma_as; - uint64_t l2t_addr; - uint64_t value; - bool valid_l2t; - uint32_t l2t_id; - uint32_t num_l2_entries; + uint64_t entry_addr = table_entry_addr(s, &s->ct, icid, res); - if (s->ct.indirect) { - l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); - - value = address_space_ldq_le(as, - s->ct.base_addr + - (l2t_id * L1TABLE_ENTRY_SIZE), - MEMTXATTRS_UNSPECIFIED, res); - - if (*res == MEMTX_OK) { - valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; - - if (valid_l2t) { - num_l2_entries = s->ct.page_sz / s->ct.entry_sz; - - l2t_addr = value & ((1ULL << 51) - 1); - - *cte = address_space_ldq_le(as, l2t_addr + - ((icid % num_l2_entries) * GITS_CTE_SIZE), - MEMTXATTRS_UNSPECIFIED, res); - } - } - } else { - /* Flat level table */ - *cte = address_space_ldq_le(as, s->ct.base_addr + - (icid * GITS_CTE_SIZE), - MEMTXATTRS_UNSPECIFIED, res); + if (entry_addr == -1) { + return false; /* not valid */ } + *cte = address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res); return FIELD_EX64(*cte, CTE, VALID); } @@ -XXX,XX +XXX,XX @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte, static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res) { AddressSpace *as = &s->gicv3->dma_as; - uint64_t l2t_addr; - uint64_t value; - bool valid_l2t; - uint32_t l2t_id; - uint32_t num_l2_entries; + uint64_t entry_addr = table_entry_addr(s, &s->dt, devid, res); - if (s->dt.indirect) { - l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); - - value = address_space_ldq_le(as, - s->dt.base_addr + - (l2t_id * L1TABLE_ENTRY_SIZE), - MEMTXATTRS_UNSPECIFIED, res); - - if (*res == MEMTX_OK) { - valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; - - if (valid_l2t) { - num_l2_entries = s->dt.page_sz / s->dt.entry_sz; - - l2t_addr = value & ((1ULL << 51) - 1); - - value = address_space_ldq_le(as, l2t_addr + - ((devid % num_l2_entries) * GITS_DTE_SIZE), - MEMTXATTRS_UNSPECIFIED, res); - } - } - } else { - /* Flat level table */ - value = address_space_ldq_le(as, s->dt.base_addr + - (devid * GITS_DTE_SIZE), - MEMTXATTRS_UNSPECIFIED, res); + if (entry_addr == -1) { + return 0; /* a DTE entry with the Valid bit clear */ } - - return value; + return address_space_ldq_le(as, entry_addr, MEMTXATTRS_UNSPECIFIED, res); } /* @@ -XXX,XX +XXX,XX @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid, uint64_t rdbase) { AddressSpace *as = &s->gicv3->dma_as; - uint64_t value; - uint64_t l2t_addr; - bool valid_l2t; - uint32_t l2t_id; - uint32_t num_l2_entries; + uint64_t entry_addr; uint64_t cte = 0; MemTxResult res = MEMTX_OK; @@ -XXX,XX +XXX,XX @@ static bool update_cte(GICv3ITSState *s, uint16_t icid, bool valid, cte = FIELD_DP64(cte, CTE, RDBASE, rdbase); } - /* - * The specification defines the format of level 1 entries of a - * 2-level table, but the format of level 2 entries and the format - * of flat-mapped tables is IMPDEF. - */ - if (s->ct.indirect) { - l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE); - - value = address_space_ldq_le(as, - s->ct.base_addr + - (l2t_id * L1TABLE_ENTRY_SIZE), - MEMTXATTRS_UNSPECIFIED, &res); - - if (res != MEMTX_OK) { - return false; - } - - valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; - - if (valid_l2t) { - num_l2_entries = s->ct.page_sz / s->ct.entry_sz; - - l2t_addr = value & ((1ULL << 51) - 1); - - address_space_stq_le(as, l2t_addr + - ((icid % num_l2_entries) * GITS_CTE_SIZE), - cte, MEMTXATTRS_UNSPECIFIED, &res); - } - } else { - /* Flat level table */ - address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE), - cte, MEMTXATTRS_UNSPECIFIED, &res); - } + entry_addr = table_entry_addr(s, &s->ct, icid, &res); if (res != MEMTX_OK) { + /* memory access error: stall */ return false; - } else { + } + if (entry_addr == -1) { + /* No L2 table for this index: discard write and continue */ return true; } + + address_space_stq_le(as, entry_addr, cte, MEMTXATTRS_UNSPECIFIED, &res); + return res == MEMTX_OK; } static ItsCmdResult process_mapc(GICv3ITSState *s, uint32_t offset) @@ -XXX,XX +XXX,XX @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid, uint8_t size, uint64_t itt_addr) { AddressSpace *as = &s->gicv3->dma_as; - uint64_t value; - uint64_t l2t_addr; - bool valid_l2t; - uint32_t l2t_id; - uint32_t num_l2_entries; + uint64_t entry_addr; uint64_t dte = 0; MemTxResult res = MEMTX_OK; @@ -XXX,XX +XXX,XX @@ static bool update_dte(GICv3ITSState *s, uint32_t devid, bool valid, return true; } - /* - * The specification defines the format of level 1 entries of a - * 2-level table, but the format of level 2 entries and the format - * of flat-mapped tables is IMPDEF. - */ - if (s->dt.indirect) { - l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE); - - value = address_space_ldq_le(as, - s->dt.base_addr + - (l2t_id * L1TABLE_ENTRY_SIZE), - MEMTXATTRS_UNSPECIFIED, &res); - - if (res != MEMTX_OK) { - return false; - } - - valid_l2t = (value & L2_TABLE_VALID_MASK) != 0; - - if (valid_l2t) { - num_l2_entries = s->dt.page_sz / s->dt.entry_sz; - - l2t_addr = value & ((1ULL << 51) - 1); - - address_space_stq_le(as, l2t_addr + - ((devid % num_l2_entries) * GITS_DTE_SIZE), - dte, MEMTXATTRS_UNSPECIFIED, &res); - } - } else { - /* Flat level table */ - address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE), - dte, MEMTXATTRS_UNSPECIFIED, &res); - } + entry_addr = table_entry_addr(s, &s->dt, devid, &res); if (res != MEMTX_OK) { + /* memory access error: stall */ return false; - } else { + } + if (entry_addr == -1) { + /* No L2 table for this index: discard write and continue */ return true; } + address_space_stq_le(as, entry_addr, dte, MEMTXATTRS_UNSPECIFIED, &res); + return res == MEMTX_OK; } static ItsCmdResult process_mapd(GICv3ITSState *s, uint64_t value, -- 2.25.1 In a few places in the ITS command handling functions, we were doing the range-check of an event ID or device ID only after using it as a table index; move the checks to before the uses. This misordering wouldn't have very bad effects because the tables are in guest memory anyway. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Message-id: 20220111171048.3545974-13-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 42 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, eventid = (value & EVENTID_MASK); + if (devid >= s->dt.num_ids) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes: devid %d>=%d", + __func__, devid, s->dt.num_ids); + return CMD_CONTINUE; + } + dte = get_dte(s, devid, &res); if (res != MEMTX_OK) { @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); + if (eventid >= num_eventids) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes: eventid %d >= %" + PRId64 "\n", + __func__, eventid, num_eventids); + return CMD_CONTINUE; + } + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res); if (res != MEMTX_OK) { return CMD_STALL; @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, return CMD_CONTINUE; } - if (devid >= s->dt.num_ids) { - qemu_log_mask(LOG_GUEST_ERROR, - "%s: invalid command attributes: devid %d>=%d", - __func__, devid, s->dt.num_ids); - return CMD_CONTINUE; - } - if (eventid >= num_eventids) { - qemu_log_mask(LOG_GUEST_ERROR, - "%s: invalid command attributes: eventid %d >= %" - PRId64 "\n", - __func__, eventid, num_eventids); - return CMD_CONTINUE; - } - /* * Current implementation only supports rdbase == procnum * Hence rdbase physical address is ignored @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, icid = value & ICID_MASK; + if (devid >= s->dt.num_ids) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid command attributes: devid %d>=%d", + __func__, devid, s->dt.num_ids); + return CMD_CONTINUE; + } + dte = get_dte(s, devid, &res); if (res != MEMTX_OK) { @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_mapti(GICv3ITSState *s, uint64_t value, num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1); num_intids = 1ULL << (GICD_TYPER_IDBITS + 1); - if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids) + if ((icid >= s->ct.num_ids) || !dte_valid || (eventid >= num_eventids) || (((pIntid < GICV3_LPI_INTID_START) || (pIntid >= num_intids)) && (pIntid != INTID_SPURIOUS))) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid command attributes " - "devid %d or icid %d or eventid %d or pIntid %d or" - "unmapped dte %d\n", __func__, devid, icid, eventid, + "icid %d or eventid %d or pIntid %d or" + "unmapped dte %d\n", __func__, icid, eventid, pIntid, dte_valid); /* * in this implementation, in case of error -- 2.25.1 In process_its_cmd(), we read an ICID out of the interrupt table entry, and then use it as an index into the collection table. Add a check that it is within range for the collection table first. This check is not strictly necessary, because: * we range check the ICID from the guest before writing it into the interrupt table entry, so the the only way to get an out of range ICID in process_its_cmd() is if a badly-behaved guest is writing directly to the interrupt table memory * the collection table is in guest memory, so QEMU won't fall over if we read off the end of it However, it seems clearer to include the check. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Message-id: 20220111171048.3545974-14-peter.maydell@linaro.org --- hw/intc/arm_gicv3_its.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -XXX,XX +XXX,XX @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value, return CMD_CONTINUE; } + if (icid >= s->ct.num_ids) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: invalid ICID 0x%x in ITE (table corrupted?)\n", + __func__, icid); + return CMD_CONTINUE; + } + cte_valid = get_cte(s, icid, &cte, &res); if (res != MEMTX_OK) { return CMD_STALL; -- 2.25.1 From: Philippe Mathieu-Daudé <philmd@redhat.com> Quoting Peter Maydell: "These MEMTX_* aren't from the memory transaction API functions; they're just being used by gicd_readl() and friends as a way to indicate a success/failure so that the actual MemoryRegionOps read/write fns like gicv3_dist_read() can log a guest error." We are going to introduce more MemTxResult bits, so it is safer to check for !MEMTX_OK rather than MEMTX_ERROR. Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: David Hildenbrand <david@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gicv3_redist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c index XXXXXXX..XXXXXXX 100644 --- a/hw/intc/arm_gicv3_redist.c +++ b/hw/intc/arm_gicv3_redist.c @@ -XXX,XX +XXX,XX @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data, break; } - if (r == MEMTX_ERROR) { + if (r != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid guest read at offset " TARGET_FMT_plx " size %u\n", __func__, offset, size); @@ -XXX,XX +XXX,XX @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data, break; } - if (r == MEMTX_ERROR) { + if (r != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid guest write at offset " TARGET_FMT_plx " size %u\n", __func__, offset, size); -- 2.25.1