On Thu, 12 Mar 2020 at 16:45, Peter Maydell <address@hidden> wrote:
>
> From: Niek Linnenbank <address@hidden>
>
> 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?
The allwinner_h3_dramcom_write() function invokes this function when the 'Control' register
is written. Basically allwinner_h3_dramc_map_rows() needs to insert a memory region such
that the bootloader can detect the amount of RAM. Currently U-Boot only searches up to page_size
values up to 8192, so in practise that would not trigger a result greater than 0x7fffffff.
However in theory, the Control register can indeed be written with larger page_size values.
So to be safe, I'll just make a small patch to replace the 1 with 1UL, thanks!
Regards,
Niek
thanks
-- PMM