[PATCH 0/3] nvram: at24c: fix problems related to "rom-size"

[PATCH 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, mostly related to "rom-size". It fixes a segfault.

Wolfram Sang (3):
  nvram: at24c: remove doubled prefix for ERR
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"

 hw/nvram/eeprom_at24c.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, mostly related to "rom-size". It fixes a segfault.

Wolfram Sang (3):
  nvram: at24c: remove doubled prefix for ERR
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"

 hw/nvram/eeprom_at24c.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Wolfram Sang]

The ERR macro already has the TYPE_AT24C_EE prefix, no need to repeat in
the error message.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 5494351976..8507516b7e 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -60,8 +60,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
         if (ee->blk && ee->changed) {
             int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
             if (len != ee->rsize) {
-                ERR(TYPE_AT24C_EE
-                        " : failed to write backing file\n");
+                ERR("failed to write backing file\n");
             }
             DPRINTK("Wrote to backing file\n");
         }
@@ -127,7 +126,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
         int64_t len = blk_getlength(ee->blk);
 
         if (len != ee->rsize) {
-            ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+            ERR("Backing file size %lu != %u\n",
                     (unsigned long)len, (unsigned)ee->rsize);
             exit(1);
         }
@@ -135,8 +134,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
         if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                          BLK_PERM_ALL, &error_fatal) < 0)
         {
-            ERR(TYPE_AT24C_EE
-                    " : Backing file incorrect permission\n");
+            ERR("Backing file incorrect permission\n");
             exit(1);
         }
     }
@@ -158,8 +156,7 @@ void at24c_eeprom_reset(DeviceState *state)
         int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
         if (len != ee->rsize) {
-            ERR(TYPE_AT24C_EE
-                    " : Failed initial sync with backing file\n");
+            ERR("Failed initial sync with backing file\n");
         }
         DPRINTK("Reset read backing file\n");
     }
-- 
2.11.0

[Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Wolfram Sang]

The ERR macro already has the TYPE_AT24C_EE prefix, no need to repeat in
the error message.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 5494351976..8507516b7e 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -60,8 +60,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
         if (ee->blk && ee->changed) {
             int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
             if (len != ee->rsize) {
-                ERR(TYPE_AT24C_EE
-                        " : failed to write backing file\n");
+                ERR("failed to write backing file\n");
             }
             DPRINTK("Wrote to backing file\n");
         }
@@ -127,7 +126,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
         int64_t len = blk_getlength(ee->blk);
 
         if (len != ee->rsize) {
-            ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+            ERR("Backing file size %lu != %u\n",
                     (unsigned long)len, (unsigned)ee->rsize);
             exit(1);
         }
@@ -135,8 +134,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
         if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                          BLK_PERM_ALL, &error_fatal) < 0)
         {
-            ERR(TYPE_AT24C_EE
-                    " : Backing file incorrect permission\n");
+            ERR("Backing file incorrect permission\n");
             exit(1);
         }
     }
@@ -158,8 +156,7 @@ void at24c_eeprom_reset(DeviceState *state)
         int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
         if (len != ee->rsize) {
-            ERR(TYPE_AT24C_EE
-                    " : Failed initial sync with backing file\n");
+            ERR("Failed initial sync with backing file\n");
         }
         DPRINTK("Reset read backing file\n");
     }
-- 
2.11.0

[PATCH 2/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense as well.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 8507516b7e..d82710e1df 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -120,6 +120,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
     EEPROMState *ee = AT24C_EE(i2c);
 
+    if (!ee->rsize) {
+        ERR("rom-size not allowed to be 0\n");
+        exit(1);
+    }
+
     ee->mem = g_malloc0(ee->rsize);
 
     if (ee->blk) {
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense as well.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 8507516b7e..d82710e1df 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -120,6 +120,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
     EEPROMState *ee = AT24C_EE(i2c);
 
+    if (!ee->rsize) {
+        ERR("rom-size not allowed to be 0\n");
+        exit(1);
+    }
+
     ee->mem = g_malloc0(ee->rsize);
 
     if (ee->blk) {
-- 
2.11.0

[PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" doesn't make much sense, let's use the smallest 24cXX
which has 128 byte.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index d82710e1df..de988f8d07 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -168,7 +168,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
     DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
     DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
     DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" doesn't make much sense, let's use the smallest 24cXX
which has 128 byte.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 hw/nvram/eeprom_at24c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index d82710e1df..de988f8d07 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -168,7 +168,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
     DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
     DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
     DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Philippe Mathieu-Daudé]

Hi Wolfram,

On 03/12/2018 10:42 PM, Wolfram Sang wrote:
> 0 as "rom-size" doesn't make much sense, let's use the smallest 24cXX
> which has 128 byte.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  hw/nvram/eeprom_at24c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index d82710e1df..de988f8d07 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -168,7 +168,7 @@ void at24c_eeprom_reset(DeviceState *state)
>  }
>  
>  static Property at24c_eeprom_props[] = {
> -    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
> +    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),

This patch should goes before your 2/3 in your series.

Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.

With a #define you can add:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
>      DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
>      DEFINE_PROP_END_OF_LIST()
> 

Re: [Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Philippe Mathieu-Daudé]

Hi Wolfram,

On 03/12/2018 10:42 PM, Wolfram Sang wrote:
> The ERR macro already has the TYPE_AT24C_EE prefix, no need to repeat in
> the error message.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  hw/nvram/eeprom_at24c.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 5494351976..8507516b7e 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -60,8 +60,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>          if (ee->blk && ee->changed) {
>              int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
>              if (len != ee->rsize) {
> -                ERR(TYPE_AT24C_EE
> -                        " : failed to write backing file\n");
> +                ERR("failed to write backing file\n");

printf/fprintf are deprecated, since you are modifying this file can you
use a newer API, "qemu/error-report.h" for example.

>              }
>              DPRINTK("Wrote to backing file\n");

DPRINTK() can be converted to tracepoint.

>          }
> @@ -127,7 +126,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
>          int64_t len = blk_getlength(ee->blk);
>  
>          if (len != ee->rsize) {
> -            ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
> +            ERR("Backing file size %lu != %u\n",
>                      (unsigned long)len, (unsigned)ee->rsize);
>              exit(1);
>          }
> @@ -135,8 +134,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
>          if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>                           BLK_PERM_ALL, &error_fatal) < 0)
>          {
> -            ERR(TYPE_AT24C_EE
> -                    " : Backing file incorrect permission\n");
> +            ERR("Backing file incorrect permission\n");
>              exit(1);
>          }
>      }
> @@ -158,8 +156,7 @@ void at24c_eeprom_reset(DeviceState *state)
>          int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
>  
>          if (len != ee->rsize) {
> -            ERR(TYPE_AT24C_EE
> -                    " : Failed initial sync with backing file\n");
> +            ERR("Failed initial sync with backing file\n");
>          }
>          DPRINTK("Reset read backing file\n");
>      }
> 

Re: [Qemu-devel] [PATCH 2/3] nvram: at24c: prevent segfault by checking "rom-size"

[Philippe Mathieu-Daudé]

On 03/12/2018 10:42 PM, Wolfram Sang wrote:
> The value for "rom-size" is used as a divisor, so it must not be 0 or it
> will segfault. A size of 0 wouldn't make sense as well.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  hw/nvram/eeprom_at24c.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 8507516b7e..d82710e1df 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -120,6 +120,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
>  {
>      EEPROMState *ee = AT24C_EE(i2c);
>  
> +    if (!ee->rsize) {
> +        ERR("rom-size not allowed to be 0\n");

This might be more useful:

           error_report("Minimum rom-size is %u", AT24C_ROMSIZE_MIN);

> +        exit(1);
> +    }
> +
>      ee->mem = g_malloc0(ee->rsize);
>  
>      if (ee->blk) {
> 

Re: [Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]


> > -                ERR(TYPE_AT24C_EE
> > -                        " : failed to write backing file\n");
> > +                ERR("failed to write backing file\n");
> 
> printf/fprintf are deprecated, since you are modifying this file can you
> use a newer API, "qemu/error-report.h" for example.

Sure, I'll have a look.

> >              }
> >              DPRINTK("Wrote to backing file\n");
> 
> DPRINTK() can be converted to tracepoint.

I'd leave that for another incremental change.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

Hi Philippe,

> >  static Property at24c_eeprom_props[] = {
> > -    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
> > +    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
> 
> This patch should goes before your 2/3 in your series.

I don't mind much, but why? My reasoning was "let's first fix the cause
and then the symptom"?

> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.

Can do, of course. But won't that give room for regressions because
people are already using it with lower values?

Ideally, we would have a "model" variable. The model type would define
the size of the memory. The "rom-size" variable could then be kept as is
(except for the 0 bugfix) or deprecated?

Thanks for the review,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Philippe Mathieu-Daudé]

[-- Attachment #1.1: Type: text/plain, Size: 1375 bytes --]

Hi Wolfram,

On 03/13/2018 09:16 PM, Wolfram Sang wrote:
> Hi Philippe,
> 
>>>  static Property at24c_eeprom_props[] = {
>>> -    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
>>> +    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
>>
>> This patch should goes before your 2/3 in your series.
> 
> I don't mind much, but why? My reasoning was "let's first fix the cause
> and then the symptom"?

The '0' case is worst than incorrect, it segfaults, so you are right :)

> 
>> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> 
> Can do, of course. But won't that give room for regressions because
> people are already using it with lower values?

Your patch already introduce the regression :)

I prefer self-explanatory #defines than magic value, but I see your
point, so if we can not decide a value, can you add a comment to explain
the magic value? I think the clearer is to add a #define with a comment.

> 
> Ideally, we would have a "model" variable. The model type would define
> the size of the memory. The "rom-size" variable could then be kept as is
> (except for the 0 bugfix) or deprecated?

IMO there are too many AT24C eeproms to model, so the "rom-size"
variable is the easiest way. Your patch #2 is simple enough to avoid the
#DIV/0!

> 
> Thanks for the review,
> 
>    Wolfram
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

Hi Wolfram,

On 03/13/2018 09:16 PM, Wolfram Sang wrote:
> Hi Philippe,
> 
>>>  static Property at24c_eeprom_props[] = {
>>> -    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
>>> +    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
>>
>> This patch should goes before your 2/3 in your series.
> 
> I don't mind much, but why? My reasoning was "let's first fix the cause
> and then the symptom"?

The '0' case is worst than incorrect, it segfaults, so you are right :)

> 
>> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> 
> Can do, of course. But won't that give room for regressions because
> people are already using it with lower values?

Your patch already introduce the regression :)

I prefer self-explanatory #defines than magic value, but I see your
point, so if we can not decide a value, can you add a comment to explain
the magic value? I think the clearer is to add a #define with a comment.

> 
> Ideally, we would have a "model" variable. The model type would define
> the size of the memory. The "rom-size" variable could then be kept as is
> (except for the 0 bugfix) or deprecated?

IMO there are too many AT24C eeproms to model, so the "rom-size"
variable is the easiest way. Your patch #2 is simple enough to avoid the
#DIV/0!

> 
> Thanks for the review,
> 
>    Wolfram
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Hi Philippe,

> > I don't mind much, but why? My reasoning was "let's first fix the cause
> > and then the symptom"?
> 
> The '0' case is worst than incorrect, it segfaults, so you are right :)

Ok, thanks.

> >> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> > 
> > Can do, of course. But won't that give room for regressions because
> > people are already using it with lower values?
> 
> Your patch already introduce the regression :)

Not really. The current default setting (0) segfaults. This is how I
discovered it. So, there can't be any active user of that.

> I prefer self-explanatory #defines than magic value, but I see your
> point, so if we can not decide a value, can you add a comment to explain
> the magic value? I think the clearer is to add a #define with a comment.

I wouldn't mind a macro AT24C_ROMSIZE_DEFAULT and a comment explaining
why we chose this value. This is totally fine with me.

It was just the MIN value I saw potential usage regressions with.

> IMO there are too many AT24C eeproms to model, so the "rom-size"
> variable is the easiest way. Your patch #2 is simple enough to avoid the
> #DIV/0!

Fine with me, too. I will update my series accordingly.

Thanks,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]