Re: [PATCH v5 11/17] irqchip/riscv-imsic: Add S-mode MSI address list

[Andrew Jones <andrew.jones@oss.qualcomm.com>]

A Sashiko review raised the following questions:

> +		if (!local->msi_va) {
> +			kfree(smode_msi_pa);
> +			return -ENODEV;
>
> Does this strict requirement introduce a regression on systems with partial
> IMSIC coverage?
>
> By returning -ENODEV here if any possible CPU lacks an IMSIC mapping, the
> error bubbles up to imsic_setup_state() and fatally tears down the entire
> driver. Previously, imsic_setup_state() allowed partial coverage as long as
> at least one CPU handler was found.

Partial IMSIC coverage is not functionally supported. The IPI and MSI
paths can select any online CPU and unconditionally use that CPU's
local->msi_va or local->msi_pa, so every possible CPU must have an
initialized IMSIC page. Returning -ENODEV during setup therefore
validates this invariant earlier.

>
> +		smode_msi_pa[cpu] = local->msi_pa;
>
> Could this assignment cause an out-of-bounds write on systems where the CPU
> mask is sparse?
>
> The smode_msi_pa array is allocated using num_possible_cpus() (the count of
> possible CPUs) but is indexed here by the logical CPU ID. If the CPU mask has
> gaps (for example, if logical CPUs 0 and 2 are present), num_possible_cpus()
> would evaluate to 2, but this loop would attempt to write to index 2,
> overflowing the bounds of the array. Would allocating based on nr_cpu_ids
> prevent this issue?

For RISC-V, num_possible_cpus() == nr_cpu_ids because RISC-V assigns
logical CPU IDs sequentially during both DT and ACPI discovery and
populates the possible mask from those contiguous mappings.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv