Re: [PULL 22/36] hw/arm/allwinner-h3: add SDRAM controller device

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 12 Mar 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Niek Linnenbank <nieklinnenbank@gmail.com>
>
> In the Allwinner H3 SoC the SDRAM controller is responsible
> for interfacing with the external Synchronous Dynamic Random
> Access Memory (SDRAM). Types of memory that the SDRAM controller
> supports are DDR2/DDR3 and capacities of up to 2GiB. This commit
> adds emulation support of the Allwinner H3 SDRAM controller.

Hi; Coverity has flagged a possible issue with this patch
(CID 1421912):

> +static void allwinner_h3_dramc_map_rows(AwH3DramCtlState *s, uint8_t row_bits,
> +                                        uint8_t bank_bits, uint16_t page_size)
> +{
> +    /*
> +     * This function simulates row addressing behavior when bootloader
> +     * software attempts to detect the amount of available SDRAM. In U-Boot
> +     * the controller is configured with the widest row addressing available.
> +     * Then a pattern is written to RAM at an offset on the row boundary size.
> +     * If the value read back equals the value read back from the
> +     * start of RAM, the bootloader knows the amount of row bits.
> +     *
> +     * This function inserts a mirrored memory region when the configured row
> +     * bits are not matching the actual emulated memory, to simulate the
> +     * same behavior on hardware as expected by the bootloader.
> +     */
> +    uint8_t row_bits_actual = 0;
> +
> +    /* Calculate the actual row bits using the ram_size property */
> +    for (uint8_t i = 8; i < 12; i++) {
> +        if (1 << i == s->ram_size) {
> +            row_bits_actual = i + 3;
> +            break;
> +        }
> +    }
> +
> +    if (s->ram_size == (1 << (row_bits - 3))) {
> +        /* When row bits is the expected value, remove the mirror */
> +        memory_region_set_enabled(&s->row_mirror_alias, false);
> +        trace_allwinner_h3_dramc_rowmirror_disable();
> +
> +    } else if (row_bits_actual) {
> +        /* Row bits not matching ram_size, install the rows mirror */
> +        hwaddr row_mirror = s->ram_addr + ((1 << (row_bits_actual +
> +                                                  bank_bits)) * page_size);

In this calculation we do the multiply as a signed 32-bit operation,
which then gets sign-extended to 64 bits for the addition; that
means that if the multiply result is greater than 0x7fffffff then
the upper bits of the result will all be 1s. Is this a "can't happen"
situation, or should we be using "1ULL" to force a 64-bit multiply?

thanks
-- PMM