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

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

[Philippe Mathieu-Daudé]

As of this commit, the biggest CFI01 NOR flash documented is
the Micron PC28F00BP33EF. Its size is 2 GiB (256 MiB).

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[].
-- 
2.21.3

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

[Laszlo Ersek]

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

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

[Peter Maydell]

On Mon, 25 May 2020 at 16:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> As of this commit, the biggest CFI01 NOR flash documented is
> the Micron PC28F00BP33EF. Its size is 2 GiB (256 MiB).
>
> 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.

What problem is this check solving? Is there some implementation
imposed limitation or a limit within the flash header format
that means larger sizes don't work? If so, what's the restriction?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

Hi Peter,

On 5/25/20 10:59 PM, Peter Maydell wrote:
> On Mon, 25 May 2020 at 16:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> As of this commit, the biggest CFI01 NOR flash documented is
>> the Micron PC28F00BP33EF. Its size is 2 GiB (256 MiB).

Oops, read as "2 Gbit (256 Mbyte)".

>>
>> 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.

Also here read "256 MiB" (I capped to 16MiB to test the patch).
>>
>> Note, the sbsa-ref ARM machine introduced in commit 64580903c2b
>> already uses a pair of 256 MiB flash devices.
> 
> What problem is this check solving? Is there some implementation
> imposed limitation or a limit within the flash header format
> that means larger sizes don't work? If so, what's the restriction?

I am not confident maintaining virtual device with no specifications.
If someone come with a datasheet for a pflash > 256MB then we can add
another commit to relax this restriction.
If someone is forced to use a >256MB and such hardware does not exist,
I'd rather have a document in docs/specs/pflash_cfi01.rst describing the
CFI fields.

IOW this is not a hardware limitation, but a maintenance protection.

I am worried with the recent EDK2 activity with the SBSA-ref, and I
don't want to give false hope to developers that they can create any
kind of pflash with the current device model.

If I reword this better in the commit description, is my argument
acceptable?

Thanks,

Phil.

> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On Thu, 4 Jun 2020 at 16:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 5/25/20 10:59 PM, Peter Maydell wrote:
> > What problem is this check solving? Is there some implementation
> > imposed limitation or a limit within the flash header format
> > that means larger sizes don't work? If so, what's the restriction?
>
> I am not confident maintaining virtual device with no specifications.

This isn't a virtual device, is it? The CFI spec exists and defines
what these flash devices behave like.

> If someone come with a datasheet for a pflash > 256MB then we can add
> another commit to relax this restriction.
> If someone is forced to use a >256MB and such hardware does not exist,
> I'd rather have a document in docs/specs/pflash_cfi01.rst describing the
> CFI fields.
>
> IOW this is not a hardware limitation, but a maintenance protection.
>
> I am worried with the recent EDK2 activity with the SBSA-ref, and I
> don't want to give false hope to developers that they can create any
> kind of pflash with the current device model.
>
> If I reword this better in the commit description, is my argument
> acceptable?

Not really; I think we should know what we're limiting against.
Currently you're checking total_len, but this is just sector_len * nb_blocs,
so if there's a problem with silly large values then it's probably
actually a problem with one of those being over-sized which would
still show up even if the total_len was less than 256MB.
(I suspect the underlying limit here is what the cfi_table entries
0x2D..0x30 impose on blocks_per_device and sector_len_per_device.)

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 6/4/20 5:30 PM, Peter Maydell wrote:
> On Thu, 4 Jun 2020 at 16:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 5/25/20 10:59 PM, Peter Maydell wrote:
>>> What problem is this check solving? Is there some implementation
>>> imposed limitation or a limit within the flash header format
>>> that means larger sizes don't work? If so, what's the restriction?
>>
>> I am not confident maintaining virtual device with no specifications.
> 
> This isn't a virtual device, is it? The CFI spec exists and defines
> what these flash devices behave like.
> 
>> If someone come with a datasheet for a pflash > 256MB then we can add
>> another commit to relax this restriction.
>> If someone is forced to use a >256MB and such hardware does not exist,
>> I'd rather have a document in docs/specs/pflash_cfi01.rst describing the
>> CFI fields.
>>
>> IOW this is not a hardware limitation, but a maintenance protection.
>>
>> I am worried with the recent EDK2 activity with the SBSA-ref, and I
>> don't want to give false hope to developers that they can create any
>> kind of pflash with the current device model.
>>
>> If I reword this better in the commit description, is my argument
>> acceptable?
> 
> Not really; I think we should know what we're limiting against.
> Currently you're checking total_len, but this is just sector_len * nb_blocs,
> so if there's a problem with silly large values then it's probably
> actually a problem with one of those being over-sized which would
> still show up even if the total_len was less than 256MB.
> (I suspect the underlying limit here is what the cfi_table entries
> 0x2D..0x30 impose on blocks_per_device and sector_len_per_device.)

What I'm working on is a whitelist of the few models our machines really
use, but it is taking time. Meanwhile I wanted to at least limit the
total size.

I'll try to finish my whitelist effort before someone try to use a
>256MB flash.

> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On Thu, 4 Jun 2020 at 16:55, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 6/4/20 5:30 PM, Peter Maydell wrote:
> > Not really; I think we should know what we're limiting against.
> > Currently you're checking total_len, but this is just sector_len * nb_blocs,
> > so if there's a problem with silly large values then it's probably
> > actually a problem with one of those being over-sized which would
> > still show up even if the total_len was less than 256MB.
> > (I suspect the underlying limit here is what the cfi_table entries
> > 0x2D..0x30 impose on blocks_per_device and sector_len_per_device.)
>
> What I'm working on is a whitelist of the few models our machines really
> use, but it is taking time. Meanwhile I wanted to at least limit the
> total size.

I don't see what we would be whitelisting, though. The only way
to create a flash device is from hand-written C code in the board
model. If a new board model does something weird we can catch that
in code review. Sanity checks on whether the properties supplied
by the board code make sense might be useful; randomly saying
"you can't have a flash device unless it's one we've seen before"
makes less sense to me, because it just means we'll end up adding
to the whitelist every time.

thanks
-- PMM