Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB

[Laszlo Ersek <lersek@redhat.com>]

On 05/25/20 17:58, Philippe Mathieu-Daudé wrote:
> As of this commit, the biggest CFI01 NOR flash documented is
> the Micron PC28F00BP33EF. Its size is 2 GiB (256 MiB).

I don't understand what "2 GiB (256 MiB)" means; please clarify.

> 
> Actually this "2Gb device employs a virtual chip enable feature,
> which combines two 1Gb die with a common chip enable".
> 
> Since we do not want to model unrealistic hardware, cap the
> current model to this maximum. At least we have a datasheet
> to refer.
> 
> If a bigger flash is provided, the user get this warning:
> 
>   qemu-system-aarch64: Initialization of device cfi.pflash01 failed: Maximum supported CFI flash size is 16 MiB.
> 
> Note, the sbsa-ref ARM machine introduced in commit 64580903c2b
> already uses a pair of 256 MiB flash devices.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 11922c0f96..40f145dde7 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -37,6 +37,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/cutils.h"
>  #include "hw/block/block.h"
>  #include "hw/block/flash.h"
>  #include "hw/qdev-properties.h"
> @@ -68,6 +70,8 @@ do {                                                        \
>  #define PFLASH_BE          0
>  #define PFLASH_SECURE      1
>  
> +#define PFLASH_SIZE_MAX     (256 * MiB) /* Micron PC28F00BP33EF */
> +
>  struct PFlashCFI01 {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -717,6 +721,12 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      total_len = pfl->sector_len * pfl->nb_blocs;
> +    if (total_len > PFLASH_SIZE_MAX) {
> +        char *maxsz = size_to_str(PFLASH_SIZE_MAX);
> +        error_setg(errp, "Maximum supported CFI flash size is %s.", maxsz);
> +        g_free(maxsz);
> +        return;
> +    }
>  
>      /* These are only used to expose the parameters of each device
>       * in the cfi_table[].
> 

I'm unsure how strong the argument is, "we do not want to model
unrealistic hardware", but I do think the patch does what it says on the
tin (modulo the one part in the commit message that I don't understand).

With the commit message clarified

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I expect & hope people will provide better reviews than mine.)

Thanks
Laszlo