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
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
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
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 >
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
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 >
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
© 2016 - 2024 Red Hat, Inc.