* [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
@ 2020-05-25 15:58 Philippe Mathieu-Daudé
2020-05-25 19:02 ` Laszlo Ersek
2020-05-25 20:59 ` Peter Maydell
0 siblings, 2 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-25 15:58 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, qemu-block,
Peter Maydell, Philippe Mathieu-Daudé,
Radoslaw Biernacki, Markus Armbruster, Max Reitz, Leif Lindholm,
Laszlo Ersek
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-05-25 15:58 [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB Philippe Mathieu-Daudé
@ 2020-05-25 19:02 ` Laszlo Ersek
2020-05-25 20:59 ` Peter Maydell
1 sibling, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-05-25 19:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Peter Maydell, Tanmay Jagdale, qemu-block,
Hongbo Zhang, Radoslaw Biernacki, Markus Armbruster, Max Reitz,
Leif Lindholm
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-05-25 15:58 [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB Philippe Mathieu-Daudé
2020-05-25 19:02 ` Laszlo Ersek
@ 2020-05-25 20:59 ` Peter Maydell
2020-06-04 15:16 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-05-25 20:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, Qemu-block,
Markus Armbruster, Radoslaw Biernacki, QEMU Developers,
Max Reitz, Leif Lindholm, Laszlo Ersek
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-05-25 20:59 ` Peter Maydell
@ 2020-06-04 15:16 ` Philippe Mathieu-Daudé
2020-06-04 15:30 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-04 15:16 UTC (permalink / raw)
To: Peter Maydell
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, Qemu-block,
Markus Armbruster, Radoslaw Biernacki, QEMU Developers,
Max Reitz, Leif Lindholm, Laszlo Ersek
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-06-04 15:16 ` Philippe Mathieu-Daudé
@ 2020-06-04 15:30 ` Peter Maydell
2020-06-04 15:55 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-06-04 15:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, Qemu-block,
Markus Armbruster, Radoslaw Biernacki, QEMU Developers,
Max Reitz, Leif Lindholm, Laszlo Ersek
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-06-04 15:30 ` Peter Maydell
@ 2020-06-04 15:55 ` Philippe Mathieu-Daudé
2020-06-04 16:03 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-04 15:55 UTC (permalink / raw)
To: Peter Maydell
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, Qemu-block,
Markus Armbruster, Radoslaw Biernacki, QEMU Developers,
Max Reitz, Leif Lindholm, Laszlo Ersek
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB
2020-06-04 15:55 ` Philippe Mathieu-Daudé
@ 2020-06-04 16:03 ` Peter Maydell
0 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-06-04 16:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Hongbo Zhang, Tanmay Jagdale, Qemu-block,
Markus Armbruster, Radoslaw Biernacki, QEMU Developers,
Max Reitz, Leif Lindholm, Laszlo Ersek
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-04 16:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-25 15:58 [PATCH] hw/block/pflash_cfi01: Limit maximum flash size to 256 MiB Philippe Mathieu-Daudé
2020-05-25 19:02 ` Laszlo Ersek
2020-05-25 20:59 ` Peter Maydell
2020-06-04 15:16 ` Philippe Mathieu-Daudé
2020-06-04 15:30 ` Peter Maydell
2020-06-04 15:55 ` Philippe Mathieu-Daudé
2020-06-04 16:03 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.