[Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Philippe Mathieu-Daudé]

We already define SDCardModes/SDCardStates as enums. Declare
the mode/state as enums too, this make gdb debugging sessions
friendlier: instead of numbers, the mode/state name is displayed.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aaab15f3868..a66b3d5b45e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -96,8 +96,8 @@ struct SDState {
     BlockBackend *blk;
     bool spi;
 
-    uint32_t mode;    /* current card mode, one of SDCardModes */
-    int32_t state;    /* current card state, one of SDCardStates */
+    enum SDCardModes mode;
+    enum SDCardStates state;
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
@@ -1640,7 +1640,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
 
 int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response) {
-    int last_state;
+    enum SDCardStates last_state;
     sd_rsp_type_t rtype;
     int rsplen;
 
-- 
2.20.1

[Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Philippe Mathieu-Daudé]

We already define SDCardModes/SDCardStates as enums. Declare
the mode/state as enums too, this make gdb debugging sessions
friendlier: instead of numbers, the mode/state name is displayed.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aaab15f3868..a66b3d5b45e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -96,8 +96,8 @@ struct SDState {
     BlockBackend *blk;
     bool spi;
 
-    uint32_t mode;    /* current card mode, one of SDCardModes */
-    int32_t state;    /* current card state, one of SDCardStates */
+    enum SDCardModes mode;
+    enum SDCardStates state;
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
@@ -1640,7 +1640,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
 
 int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response) {
-    int last_state;
+    enum SDCardStates last_state;
     sd_rsp_type_t rtype;
     int rsplen;
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> We already define SDCardModes/SDCardStates as enums. Declare
> the mode/state as enums too, this make gdb debugging sessions
> friendlier: instead of numbers, the mode/state name is displayed.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sd/sd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f3868..a66b3d5b45e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -96,8 +96,8 @@ struct SDState {
>      BlockBackend *blk;
>      bool spi;
>  
> -    uint32_t mode;    /* current card mode, one of SDCardModes */
> -    int32_t state;    /* current card state, one of SDCardStates */
> +    enum SDCardModes mode;
> +    enum SDCardStates state;
>      uint32_t vhs;
>      bool wp_switch;
>      unsigned long *wp_groups;
> @@ -1640,7 +1640,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>  
>  int sd_do_command(SDState *sd, SDRequest *req,
>                    uint8_t *response) {
> -    int last_state;
> +    enum SDCardStates last_state;
>      sd_rsp_type_t rtype;
>      int rsplen;

These guys are part of the migration state:

   static const VMStateDescription sd_vmstate = {
       .name = "sd-card",
       .version_id = 1,
       .minimum_version_id = 1,
       .pre_load = sd_vmstate_pre_load,
       .fields = (VMStateField[]) {
           VMSTATE_UINT32(mode, SDState),
           VMSTATE_INT32(state, SDState),
   [...]

Juan, David, are VMSTATE_UINT32() and VMSTATE_INT32() safe to use with
enums?

Re: [Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Philippe Mathieu-Daudé]

On 5/6/19 10:55 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> We already define SDCardModes/SDCardStates as enums. Declare
>> the mode/state as enums too, this make gdb debugging sessions
>> friendlier: instead of numbers, the mode/state name is displayed.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/sd/sd.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index aaab15f3868..a66b3d5b45e 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -96,8 +96,8 @@ struct SDState {
>>      BlockBackend *blk;
>>      bool spi;
>>  
>> -    uint32_t mode;    /* current card mode, one of SDCardModes */
>> -    int32_t state;    /* current card state, one of SDCardStates */
>> +    enum SDCardModes mode;
>> +    enum SDCardStates state;
>>      uint32_t vhs;
>>      bool wp_switch;
>>      unsigned long *wp_groups;
>> @@ -1640,7 +1640,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>>  
>>  int sd_do_command(SDState *sd, SDRequest *req,
>>                    uint8_t *response) {
>> -    int last_state;
>> +    enum SDCardStates last_state;
>>      sd_rsp_type_t rtype;
>>      int rsplen;
> 
> These guys are part of the migration state:
> 
>    static const VMStateDescription sd_vmstate = {
>        .name = "sd-card",
>        .version_id = 1,
>        .minimum_version_id = 1,
>        .pre_load = sd_vmstate_pre_load,
>        .fields = (VMStateField[]) {
>            VMSTATE_UINT32(mode, SDState),
>            VMSTATE_INT32(state, SDState),
>    [...]

Oh good point, I missed that.

> Juan, David, are VMSTATE_UINT32() and VMSTATE_INT32() safe to use with
> enums?

I'll wait Juan/Dave to enlighten us, else I can use anonymous union to
ease my debugging sessions:

-- >8 --
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
@@ -96,7 +96,10 @@ struct SDState {
     BlockBackend *blk;
     bool spi;

-    uint32_t mode;    /* current card mode, one of SDCardModes */
+    union {
+        uint32_t migratable_mode;
+        enum SDCardModes mode;
+    };
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t vhs;
     bool wp_switch;
@@ -659,7 +662,7 @@ static const VMStateDescription sd_vmstate = {
     .minimum_version_id = 1,
     .pre_load = sd_vmstate_pre_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(mode, SDState),
+        VMSTATE_UINT32(migratable_mode, SDState),
         VMSTATE_INT32(state, SDState),
         VMSTATE_UINT8_ARRAY(cid, SDState, 16),
         VMSTATE_UINT8_ARRAY(csd, SDState, 16),
---

Thanks for the review!

Phil.

Re: [Qemu-devel] [PATCH] hw/sd/sdcard: Use the available enums

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > We already define SDCardModes/SDCardStates as enums. Declare
> > the mode/state as enums too, this make gdb debugging sessions
> > friendlier: instead of numbers, the mode/state name is displayed.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  hw/sd/sd.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index aaab15f3868..a66b3d5b45e 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -96,8 +96,8 @@ struct SDState {
> >      BlockBackend *blk;
> >      bool spi;
> >  
> > -    uint32_t mode;    /* current card mode, one of SDCardModes */
> > -    int32_t state;    /* current card state, one of SDCardStates */
> > +    enum SDCardModes mode;
> > +    enum SDCardStates state;
> >      uint32_t vhs;
> >      bool wp_switch;
> >      unsigned long *wp_groups;
> > @@ -1640,7 +1640,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
> >  
> >  int sd_do_command(SDState *sd, SDRequest *req,
> >                    uint8_t *response) {
> > -    int last_state;
> > +    enum SDCardStates last_state;
> >      sd_rsp_type_t rtype;
> >      int rsplen;
> 
> These guys are part of the migration state:
> 
>    static const VMStateDescription sd_vmstate = {
>        .name = "sd-card",
>        .version_id = 1,
>        .minimum_version_id = 1,
>        .pre_load = sd_vmstate_pre_load,
>        .fields = (VMStateField[]) {
>            VMSTATE_UINT32(mode, SDState),
>            VMSTATE_INT32(state, SDState),
>    [...]
> 
> Juan, David, are VMSTATE_UINT32() and VMSTATE_INT32() safe to use with
> enums?

I think that is compiler dependent, so no.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK