Hi Peter,
On Fri, Mar 20, 2020 at 4:46 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Thu, 12 Mar 2020 at 16:45, Peter Maydell <peter.mayd...@linaro.org>
> wrote:
> >
> > From: Niek Linnenbank <nieklinnenb...@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?
>
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
>
--
Niek Linnenbank
