Re: [Qemu-devel] [PULL 01/14] ast2400: add a memory controller device model

[Peter Maydell]

On 6 September 2016 at 14:07, Peter Maydell <address@hidden> wrote:
> From: Cédric Le Goater <address@hidden>
>
> The uboot in the previous release of the SDK was using a hardcoded
> value for memory size. This is not true anymore, the value is now
> retrieved from the memory controller.
>
> Below is a model for this device, only supporting unlock and
> configuration. Without it, we endup running a guest with 64MB, which
> is a bit low nowdays. It uses a 'silicon-rev' property and ram_size to
> build a default value. Some bits should be linked to SCU strapping
> registers but it seems a bit complex to add for the current need.
>
> The model is ready for the AST2500 SOC.

> +static int ast2400_rambits(void)
> +{
> +    switch (ram_size >> 20) {
> +    case 64:
> +        return ASPEED_SDMC_DRAM_64MB;
> +    case 128:
> +        return ASPEED_SDMC_DRAM_128MB;
> +    case 256:
> +        return ASPEED_SDMC_DRAM_256MB;
> +    case 512:
> +        return ASPEED_SDMC_DRAM_512MB;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid RAM size: 0x%"
> +                      HWADDR_PRIx "\n", __func__, ram_size);
> +        break;

This doesn't compile on 32-bit systems, because ram_size
is not a hwaddr, it's a ram_addr_t, and it needs a different
format specifier.

I'll fix this up locally and resend the pullreq.

However, I notice looking at this code more closely that it's
using the global ram_size to determine the behaviour of the
device. That seems a bit dubious to me, we don't have other
devices that look at global config settings like that.
Can you look at doing a followup patch where the board level
code tells the memory controller how much RAM it has directly,
please?

(Also it seems wrong to call this a GUEST_ERROR, because the
thing setting the ram_size is the user running QEMU (possibly
in conjunction with any restrictions imposed by the board
model code), not the guest code.)

thanks
-- PMM