Re: [PULL 06/12] aspeed/smc: Add DMA calibration settings

[Peter Maydell]

On Fri, 13 Sept 2019 at 16:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Cédric Le Goater <clg@kaod.org>
>
> When doing calibration, the SPI clock rate in the CE0 Control Register
> and the read delay cycles in the Read Timing Compensation Register are
> set using bit[11:4] of the DMA Control Register.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Acked-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Message-id: 20190904070506.1052-7-clg@kaod.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi; this is an old patch, but Coverity has suddenly decided
it doesn't like it (CID 1547822):

> +static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask)
> +{
> +    /* HCLK/1 .. HCLK/16 */
> +    const uint8_t hclk_divisors[] = {
> +        15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
> +        if (hclk_mask == hclk_divisors[i]) {
> +            return i + 1;
> +        }
> +    }
> +
> +    qemu_log_mask(LOG_GUEST_ERROR, "invalid HCLK mask %x", hclk_mask);
> +    return 0;
> +}
> +
> +/*
> + * When doing calibration, the SPI clock rate in the CE0 Control
> + * Register and the read delay cycles in the Read Timing Compensation
> + * Register are set using bit[11:4] of the DMA Control Register.
> + */
> +static void aspeed_smc_dma_calibration(AspeedSMCState *s)
> +{
> +    uint8_t delay =
> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & DMA_CTRL_DELAY_MASK;
> +    uint8_t hclk_mask =
> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
> +    uint8_t hclk_div = aspeed_smc_hclk_divisor(hclk_mask);
> +    uint32_t hclk_shift = (hclk_div - 1) << 2;

The code of aspeeed_smc_hclk_divisor() has a codepath where it
can return 0, and this callsite doesn't check for 0, and so
Coverity thinks that we might end up shifting -1 by 2 to get
the hclk_shift here, which means we overflow the value, which
it thinks is probably not what we meant to do.

In fact this can't happen, because we always pass aspeed_smc_hclk_divisor()
a value between 0 and 15, and if we do that then we always get back
a value between 1 and 16. So I think the right fix would be
to change the qemu_log_mask()/return 0 to be g_assert_not_reached().

thanks
-- PMM