diff mbox

[for-2.9,14/30] aspeed/smc: rework the prototype of the AspeedSMCFlash helper routines

Message ID 1480434248-27138-15-git-send-email-clg@kaod.org
State New
Headers show

Commit Message

Cédric Le Goater Nov. 29, 2016, 3:43 p.m. UTC 
Change the routines prototype to use a 'AspeedSMCFlash *' instead of
'AspeedSMCState *'. The result will help in making future changes
clearer.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c         | 36 ++++++++++++++++++++++++------------
 include/hw/ssi/aspeed_smc.h |  2 +-
 2 files changed, 25 insertions(+), 13 deletions(-)

Comments

Peter Maydell Dec. 14, 2016, 5:09 p.m. UTC | #1 
On 29 November 2016 at 15:43, Cédric Le Goater <clg@kaod.org> wrote:
> Change the routines prototype to use a 'AspeedSMCFlash *' instead of
> 'AspeedSMCState *'. The result will help in making future changes
> clearer.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

This patch breaks 'make check' because the palmetto-bmc
model now segfaults on startup:

gdb --args ./build/x86/arm-softmmu/qemu-system-arm -M palmetto-bmc
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
[...]
(gdb) r
Starting program:
/home/petmay01/linaro/qemu-from-laptop/qemu/build/x86/arm-softmmu/qemu-system-arm
-M palmetto-bmc
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7fffd1270700 (LWP 30296)]
[New Thread 0x7fffcfb97700 (LWP 30297)]
[New Thread 0x7fffcf396700 (LWP 30298)]
[New Thread 0x7fffceb95700 (LWP 30299)]
[New Thread 0x7fffbf709700 (LWP 30300)]

Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
0x0000555555b2fce8 in aspeed_smc_flash_update_cs (fl=0x0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:413
413         AspeedSMCState *s = fl->controller;
(gdb) bt
#0  0x0000555555b2fce8 in aspeed_smc_flash_update_cs (fl=0x0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:413
#1  0x0000555555b2fd6a in aspeed_smc_update_cs (s=0x7fffcc3924c0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:422
#2  0x0000555555b2febc in aspeed_smc_reset (d=0x7fffcc3924c0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:447
#3  0x0000555555b304a6 in aspeed_smc_realize (dev=0x7fffcc3924c0,
errp=0x7fffffffdcf0) at
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:556
#4  0x0000555555a19329 in device_set_realized (obj=0x7fffcc3924c0,
value=true, errp=0x7fffffffde98) at
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/core/qdev.c:918
#5  0x0000555555c17e50 in property_set_bool (obj=0x7fffcc3924c0, v=
    0x555556ea9d90, name=0x555555d639e7 "realized",
opaque=0x5555569cafb0, errp=0x7fffffffde98) at
/home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1854
#6  0x0000555555c1635b in object_property_set (obj=0x7fffcc3924c0, v=
    0x555556ea9d90, name=0x555555d639e7 "realized", errp=0x7fffffffde98)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1088
#7  0x0000555555c19183 in object_property_set_qobject
(obj=0x7fffcc3924c0, value=0x555556ea9c80, name=0x555555d639e7
"realized", errp=0x7fffffffde98)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/qom-qobject.c:27
#8  0x0000555555c165fe in object_property_set_bool
(obj=0x7fffcc3924c0, value=true, name=0x555555d639e7 "realized",
errp=0x7fffffffde98)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1157
#9  0x00005555558da673 in aspeed_soc_realize (dev=0x7fffcc371010,
errp=0x7fffffffdf20) at
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed_soc.c:256
#10 0x0000555555a19329 in device_set_realized (obj=0x7fffcc371010,
value=true, errp=0x555556849518 <error_abort>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/core/qdev.c:918
#11 0x0000555555c17e50 in property_set_bool (obj=0x7fffcc371010, v=
    0x5555569cc5b0, name=0x555555d63b5c "realized",
opaque=0x5555569be320, errp=0x555556849518 <error_abort>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1854
#12 0x0000555555c1635b in object_property_set (obj=0x7fffcc371010, v=
    0x5555569cc5b0, name=0x555555d63b5c "realized",
errp=0x555556849518 <error_abort>) at
/home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1088
#13 0x0000555555c19183 in object_property_set_qobject
(obj=0x7fffcc371010, value=0x5555569cc3a0, name=0x555555d63b5c
"realized", errp=0x555556849518 <error_abort>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/qom-qobject.c:27
#14 0x0000555555c165fe in object_property_set_bool
(obj=0x7fffcc371010, value=true, name=0x555555d63b5c "realized",
errp=0x555556849518 <error_abort>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1157
#15 0x00005555558dadfe in aspeed_board_init (machine=0x55555693d8a0,
cfg=0x555556184fe0 <aspeed_boards>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed.c:152
#16 0x00005555558daf9c in palmetto_bmc_init (machine=0x55555693d8a0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed.c:182
#17 0x000055555596af75 in main (argc=3, argv=0x7fffffffe4c8,
envp=0x7fffffffe4e8)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/vl.c:4548

Calling reset from realize is probably a bad idea (reset gets
called later anyway), and calling qemu_set_irq() from reset
isn't recommended either. These may or may not be the cause
of the crash though.

thanks
-- PMM
Cédric Le Goater Dec. 15, 2016, 1:38 p.m. UTC | #2 
On 12/14/2016 06:09 PM, Peter Maydell wrote:
> On 29 November 2016 at 15:43, Cédric Le Goater <clg@kaod.org> wrote:
>> Change the routines prototype to use a 'AspeedSMCFlash *' instead of
>> 'AspeedSMCState *'. The result will help in making future changes
>> clearer.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> 
> This patch breaks 'make check' because the palmetto-bmc
> model now segfaults on startup:
> 
> gdb --args ./build/x86/arm-softmmu/qemu-system-arm -M palmetto-bmc
> GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
> [...]
> (gdb) r
> Starting program:
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/x86/arm-softmmu/qemu-system-arm
> -M palmetto-bmc
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> [New Thread 0x7fffd1270700 (LWP 30296)]
> [New Thread 0x7fffcfb97700 (LWP 30297)]
> [New Thread 0x7fffcf396700 (LWP 30298)]
> [New Thread 0x7fffceb95700 (LWP 30299)]
> [New Thread 0x7fffbf709700 (LWP 30300)]
> 
> Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
> 0x0000555555b2fce8 in aspeed_smc_flash_update_cs (fl=0x0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:413
> 413         AspeedSMCState *s = fl->controller;
> (gdb) bt
> #0  0x0000555555b2fce8 in aspeed_smc_flash_update_cs (fl=0x0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:413
> #1  0x0000555555b2fd6a in aspeed_smc_update_cs (s=0x7fffcc3924c0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:422
> #2  0x0000555555b2febc in aspeed_smc_reset (d=0x7fffcc3924c0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:447
> #3  0x0000555555b304a6 in aspeed_smc_realize (dev=0x7fffcc3924c0,
> errp=0x7fffffffdcf0) at
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ssi/aspeed_smc.c:556
> #4  0x0000555555a19329 in device_set_realized (obj=0x7fffcc3924c0,
> value=true, errp=0x7fffffffde98) at
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/core/qdev.c:918
> #5  0x0000555555c17e50 in property_set_bool (obj=0x7fffcc3924c0, v=
>     0x555556ea9d90, name=0x555555d639e7 "realized",
> opaque=0x5555569cafb0, errp=0x7fffffffde98) at
> /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1854
> #6  0x0000555555c1635b in object_property_set (obj=0x7fffcc3924c0, v=
>     0x555556ea9d90, name=0x555555d639e7 "realized", errp=0x7fffffffde98)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1088
> #7  0x0000555555c19183 in object_property_set_qobject
> (obj=0x7fffcc3924c0, value=0x555556ea9c80, name=0x555555d639e7
> "realized", errp=0x7fffffffde98)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/qom-qobject.c:27
> #8  0x0000555555c165fe in object_property_set_bool
> (obj=0x7fffcc3924c0, value=true, name=0x555555d639e7 "realized",
> errp=0x7fffffffde98)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1157
> #9  0x00005555558da673 in aspeed_soc_realize (dev=0x7fffcc371010,
> errp=0x7fffffffdf20) at
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed_soc.c:256
> #10 0x0000555555a19329 in device_set_realized (obj=0x7fffcc371010,
> value=true, errp=0x555556849518 <error_abort>)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/core/qdev.c:918
> #11 0x0000555555c17e50 in property_set_bool (obj=0x7fffcc371010, v=
>     0x5555569cc5b0, name=0x555555d63b5c "realized",
> opaque=0x5555569be320, errp=0x555556849518 <error_abort>)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1854
> #12 0x0000555555c1635b in object_property_set (obj=0x7fffcc371010, v=
>     0x5555569cc5b0, name=0x555555d63b5c "realized",
> errp=0x555556849518 <error_abort>) at
> /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1088
> #13 0x0000555555c19183 in object_property_set_qobject
> (obj=0x7fffcc371010, value=0x5555569cc3a0, name=0x555555d63b5c
> "realized", errp=0x555556849518 <error_abort>)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/qom-qobject.c:27
> #14 0x0000555555c165fe in object_property_set_bool
> (obj=0x7fffcc371010, value=true, name=0x555555d63b5c "realized",
> errp=0x555556849518 <error_abort>)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:1157
> #15 0x00005555558dadfe in aspeed_board_init (machine=0x55555693d8a0,
> cfg=0x555556184fe0 <aspeed_boards>)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed.c:152
> #16 0x00005555558daf9c in palmetto_bmc_init (machine=0x55555693d8a0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/aspeed.c:182
> #17 0x000055555596af75 in main (argc=3, argv=0x7fffffffe4c8,
> envp=0x7fffffffe4e8)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/vl.c:4548
> 
> Calling reset from realize is probably a bad idea (reset gets
> called later anyway), and calling qemu_set_irq() from reset
> isn't recommended either. These may or may not be the cause
> of the crash though.

It it a bad split with the following patch so I will merge them in 
the next version and try to cleanup up the qemu_set_irq().

Thanks,

C.
diff mbox

Patch

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 78f5aed53247..66622f198a2f 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -328,19 +328,30 @@  static const MemoryRegionOps aspeed_smc_flash_default_ops = {
     },
 };
 
-static inline int aspeed_smc_flash_mode(const AspeedSMCState *s, int cs)
+static inline int aspeed_smc_flash_mode(const AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
+    AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CMD_MODE_MASK;
+}
+
+static inline bool aspeed_smc_is_usermode(const AspeedSMCFlash *fl)
+{
+    return aspeed_smc_flash_mode(fl) == CTRL_USERMODE;
 }
 
-static inline bool aspeed_smc_is_usermode(const AspeedSMCState *s, int cs)
+static inline bool aspeed_smc_is_ce_stop_active(const AspeedSMCFlash *fl)
 {
-    return aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE;
+    AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE;
 }
 
-static inline bool aspeed_smc_is_writable(const AspeedSMCState *s, int cs)
+static inline bool aspeed_smc_is_writable(const AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
+    AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->id));
 }
 
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
@@ -350,7 +361,7 @@  static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
     uint64_t ret = 0;
     int i;
 
-    if (aspeed_smc_is_usermode(s, fl->id)) {
+    if (aspeed_smc_is_usermode(fl)) {
         for (i = 0; i < size; i++) {
             ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
         }
@@ -370,13 +381,13 @@  static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
     const AspeedSMCState *s = fl->controller;
     int i;
 
-    if (!aspeed_smc_is_writable(s, fl->id)) {
+    if (!aspeed_smc_is_writable(fl)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
                       HWADDR_PRIx "\n", __func__, addr);
         return;
     }
 
-    if (!aspeed_smc_is_usermode(s, fl->id)) {
+    if (!aspeed_smc_is_usermode(fl)) {
         qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
                       __func__);
         return;
@@ -397,9 +408,10 @@  static const MemoryRegionOps aspeed_smc_flash_ops = {
     },
 };
 
-static bool aspeed_smc_is_ce_stop_active(const AspeedSMCState *s, int cs)
+static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_ctrl0 + cs] & CTRL_CE_STOP_ACTIVE;
+    AspeedSMCState *s = fl->controller;
+    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
 }
 
 static void aspeed_smc_update_cs(const AspeedSMCState *s)
@@ -407,7 +419,7 @@  static void aspeed_smc_update_cs(const AspeedSMCState *s)
     int i;
 
     for (i = 0; i < s->num_cs; ++i) {
-        qemu_set_irq(s->cs_lines[i], aspeed_smc_is_ce_stop_active(s, i));
+        aspeed_smc_flash_update_cs(&s->flashes[i]);
     }
 }
 
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index bdfbcc0ffa7d..c64954f6103f 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -47,7 +47,7 @@  typedef struct AspeedSMCController {
 } AspeedSMCController;
 
 typedef struct AspeedSMCFlash {
-    const struct AspeedSMCState *controller;
+    struct AspeedSMCState *controller;
 
     uint8_t id;
     uint32_t size;