Patchwork [V4,09/12] hw/sd.c: convert SD state to QOM object

login
register
mail settings
Submitter Mitsyanko Igor
Date July 27, 2012, 7:29 p.m.
Message ID <1343417387-13953-10-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/173768/
State New
Headers show

Comments

Mitsyanko Igor - July 27, 2012, 7:29 p.m.
A straightforward conversion of SD card implementation to a proper QEMU object.
Wrapper functions were introduced for SDClass methods in order to avoid SD card
users modification. Because of this, name change for several functions in hw/sd.c
was required.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   56 ++++++++++++++++++++++++++++++++++++++--------------
 hw/sd.h |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 100 insertions(+), 23 deletions(-)
Markus Armbruster - July 31, 2012, 9:45 a.m.
Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> A straightforward conversion of SD card implementation to a proper QEMU object.
> Wrapper functions were introduced for SDClass methods in order to avoid SD card
> users modification. Because of this, name change for several functions in hw/sd.c
> was required.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |   56 ++++++++++++++++++++++++++++++++++++++--------------
>  hw/sd.h |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 100 insertions(+), 23 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index f8ab045..fe2c138 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -75,6 +75,8 @@ enum {
>  };
>  
>  struct SDState {
> +    Object parent_obj;
> +
>      uint32_t mode;
>      int32_t state;
>      uint32_t ocr;
> @@ -489,11 +491,8 @@ static const VMStateDescription sd_vmstate = {
>     whether card should be in SSI or MMC/SD mode.  It is also up to the
>     board to ensure that ssi transfers only occur when the chip select
>     is asserted.  */
> -SDState *sd_init(BlockDriverState *bs, bool is_spi)
> +static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
>  {
> -    SDState *sd;
> -
> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>      sd->buf = qemu_blockalign(bs, 512);
>      sd->spi = is_spi;
>      sd->enable = true;
> @@ -503,10 +502,9 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
>          bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
>      }
>      vmstate_register(NULL, -1, &sd_vmstate, sd);
> -    return sd;
>  }
>  
> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> +static void sd_set_callbacks(SDState *sd, qemu_irq readonly, qemu_irq insert)
>  {
>      sd->readonly_cb = readonly;
>      sd->inserted_cb = insert;
> @@ -1334,7 +1332,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>      return sd_cmd_class[req->cmd] == 0 || sd_cmd_class[req->cmd] == 7;
>  }
>  
> -int sd_do_command(SDState *sd, SDRequest *req,
> +static int sd_send_command(SDState *sd, SDRequest *req,
>                    uint8_t *response) {
>      int last_state;
>      sd_rsp_type_t rtype;
> @@ -1502,7 +1500,7 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>  #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
>  #define APP_WRITE_BLOCK(a, len)
>  
> -void sd_write_data(SDState *sd, uint8_t value)
> +static void sd_write_card_data(SDState *sd, uint8_t value)
>  {
>      int i;
>  
> @@ -1510,7 +1508,7 @@ void sd_write_data(SDState *sd, uint8_t value)
>          return;
>  
>      if (sd->state != sd_receivingdata_state) {
> -        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
> +        fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n");

Outside this patch's scope, but here goes anyway: what kind of condition
is reported here?  Programming error that should never happen?  Guest
doing something weird?

Same for all the other fprintf(stderr, ...) in this file.

>          return;
>      }
>  
> @@ -1621,12 +1619,12 @@ void sd_write_data(SDState *sd, uint8_t value)
>          break;
>  
>      default:
> -        fprintf(stderr, "sd_write_data: unknown command\n");
> +        fprintf(stderr, "sd_write_card_data: unknown command\n");
>          break;
>      }
>  }
>  
> -uint8_t sd_read_data(SDState *sd)
> +static uint8_t sd_read_card_data(SDState *sd)
>  {
>      /* TODO: Append CRCs */
>      uint8_t ret;
> @@ -1636,7 +1634,7 @@ uint8_t sd_read_data(SDState *sd)
>          return 0x00;
>  
>      if (sd->state != sd_sendingdata_state) {
> -        fprintf(stderr, "sd_read_data: not in Sending-Data state\n");
> +        fprintf(stderr, "sd_read_card_data: not in Sending-Data state\n");
>          return 0x00;
>      }
>  
> @@ -1738,19 +1736,47 @@ uint8_t sd_read_data(SDState *sd)
>          break;
>  
>      default:
> -        fprintf(stderr, "sd_read_data: unknown command\n");
> +        fprintf(stderr, "sd_read_card_data: unknown command\n");
>          return 0x00;
>      }
>  
>      return ret;
>  }
>  
> -bool sd_data_ready(SDState *sd)
> +static bool sd_is_data_ready(SDState *sd)
>  {
>      return sd->state == sd_sendingdata_state;
>  }
>  
> -void sd_enable(SDState *sd, bool enable)
> +static void sd_enable_card(SDState *sd, bool enable)
>  {
>      sd->enable = enable;
>  }
> +
> +static void sd_class_init(ObjectClass *klass, void *data)
> +{
> +    SDClass *k = SD_CLASS(klass);
> +
> +    k->init = sd_init_card;
> +    k->set_cb = sd_set_callbacks;
> +    k->do_command = sd_send_command;
> +    k->data_ready = sd_is_data_ready;
> +    k->read_data = sd_read_card_data;
> +    k->write_data = sd_write_card_data;
> +    k->enable = sd_enable_card;
> +}
> +
> +static const TypeInfo sd_type_info = {
> +    .name = TYPE_SD_CARD,
> +    .parent = TYPE_OBJECT,

Possibly ignorant question: why TYPE_OBJECT, not TYPE_DEVICE?

> +    .instance_size = sizeof(SDState),
> +    .class_init = sd_class_init,
> +    .class_size = sizeof(SDClass)
> +};
> +
> +static void sd_register_types(void)
> +{
> +    type_register_static(&sd_type_info);
> +}
> +
> +type_init(sd_register_types)
> diff --git a/hw/sd.h b/hw/sd.h
> index 4eb9679..f84e863 100644
> --- a/hw/sd.h
> +++ b/hw/sd.h
> @@ -29,6 +29,9 @@
>  #ifndef __hw_sd_h
>  #define __hw_sd_h		1
>  
> +#include "qemu-common.h"
> +#include "qemu/object.h"
> +
>  #define OUT_OF_RANGE		(1 << 31)
>  #define ADDRESS_ERROR		(1 << 30)
>  #define BLOCK_LEN_ERROR		(1 << 29)
> @@ -67,13 +70,61 @@ typedef struct {
>  
>  typedef struct SDState SDState;
>  
> -SDState *sd_init(BlockDriverState *bs, bool is_spi);
> -int sd_do_command(SDState *sd, SDRequest *req,
> -                  uint8_t *response);
> -void sd_write_data(SDState *sd, uint8_t value);
> -uint8_t sd_read_data(SDState *sd);
> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
> -bool sd_data_ready(SDState *sd);
> -void sd_enable(SDState *sd, bool enable);
> +typedef struct SDClass {
> +    ObjectClass parent_class;
> +
> +    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
> +    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
> +    void (*write_data)(SDState *sd, uint8_t value);
> +    uint8_t (*read_data)(SDState *sd);
> +    void (*set_cb)(SDState *sd, qemu_irq readonly, qemu_irq insert);
> +    bool (*data_ready)(SDState *sd);
> +    void (*enable)(SDState *sd, bool enable);
> +} SDClass;
> +
> +#define TYPE_SD_CARD            "sd-card"
> +#define SD_CARD(obj)            \
> +     OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
> +#define SD_CLASS(klass)         \
> +     OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD_CARD)
> +#define SD_GET_CLASS(obj)       \
> +     OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD_CARD)
> +
> +static inline SDState *sd_init(BlockDriverState *bs, bool is_spi)
> +{
> +    SDState *sd = SD_CARD(object_new(TYPE_SD_CARD));
> +    SD_GET_CLASS(sd)->init(sd, bs, is_spi);
> +    return sd;

Shouldn't bs and spi be properties?  Oh, that's coming in PATCH
10-11/12.

> +}
> +
> +static inline int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response)
> +{
> +    return SD_GET_CLASS(sd)->do_command(sd, req, response);
> +}
> +
> +static inline uint8_t sd_read_data(SDState *sd)
> +{
> +    return SD_GET_CLASS(sd)->read_data(sd);
> +}
> +
> +static inline void sd_write_data(SDState *sd, uint8_t value)
> +{
> +    SD_GET_CLASS(sd)->write_data(sd, value);
> +}
> +
> +static inline bool sd_data_ready(SDState *sd)
> +{
> +    return SD_GET_CLASS(sd)->data_ready(sd);
> +}
> +
> +static inline void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> +{
> +    SD_GET_CLASS(sd)->set_cb(sd, readonly, insert);
> +}
> +
> +static inline void sd_enable(SDState *sd, bool enable)
> +{
> +    SD_GET_CLASS(sd)->enable(sd, enable);
> +}
>  
>  #endif	/* __hw_sd_h */
Peter Maydell - July 31, 2012, 9:59 a.m.
On 31 July 2012 10:45, Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>> @@ -1510,7 +1508,7 @@ void sd_write_data(SDState *sd, uint8_t value)
>>          return;
>>
>>      if (sd->state != sd_receivingdata_state) {
>> -        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
>> +        fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n");
>
> Outside this patch's scope, but here goes anyway: what kind of condition
> is reported here?  Programming error that should never happen?  Guest
> doing something weird?

This particular case I think is "SD card controller model tried
to do something wrong".

> Same for all the other fprintf(stderr, ...) in this file.

Various uses:
 * guest legitimately did something we feel like telling the user
   about (eg "Card force-erased by CMD42")
 * guest did something dubious but with defined semantics
   ("Unknown CMD", trying to do something when the card is locked)
 * guest did something legitimate but unimplemented
 * underlying block layer read/write failed (and we are about
   to throw away the error rather than reporting it anywhere else!)

These would all be worth tidying up some day when we have a
sensible logging infrastructure to tidy them up into.

-- PMM
Mitsyanko Igor - July 31, 2012, 2:48 p.m.
On 07/31/2012 01:45 PM, Markus Armbruster wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>
>> A straightforward conversion of SD card implementation to a proper QEMU object.
>> Wrapper functions were introduced for SDClass methods in order to avoid SD card
>> users modification. Because of this, name change for several functions in hw/sd.c
>> was required.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |   56 ++++++++++++++++++++++++++++++++++++++--------------
>>   hw/sd.h |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>   2 files changed, 100 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index f8ab045..fe2c138 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -75,6 +75,8 @@ enum {
>>   };
>>   
>>   struct SDState {
>> +    Object parent_obj;
>> +
>>       uint32_t mode;
>>       int32_t state;
>>       uint32_t ocr;
>> @@ -489,11 +491,8 @@ static const VMStateDescription sd_vmstate = {
>>      whether card should be in SSI or MMC/SD mode.  It is also up to the
>>      board to ensure that ssi transfers only occur when the chip select
>>      is asserted.  */
>> -SDState *sd_init(BlockDriverState *bs, bool is_spi)
>> +static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
>>   {
>> -    SDState *sd;
>> -
>> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>>       sd->buf = qemu_blockalign(bs, 512);
>>       sd->spi = is_spi;
>>       sd->enable = true;
>> @@ -503,10 +502,9 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
>>           bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
>>       }
>>       vmstate_register(NULL, -1, &sd_vmstate, sd);
>> -    return sd;
>>   }
>>   
>> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>> +static void sd_set_callbacks(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>   {
>>       sd->readonly_cb = readonly;
>>       sd->inserted_cb = insert;
>> @@ -1334,7 +1332,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>>       return sd_cmd_class[req->cmd] == 0 || sd_cmd_class[req->cmd] == 7;
>>   }
>>   
>> -int sd_do_command(SDState *sd, SDRequest *req,
>> +static int sd_send_command(SDState *sd, SDRequest *req,
>>                     uint8_t *response) {
>>       int last_state;
>>       sd_rsp_type_t rtype;
>> @@ -1502,7 +1500,7 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>>   #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
>>   #define APP_WRITE_BLOCK(a, len)
>>   
>> -void sd_write_data(SDState *sd, uint8_t value)
>> +static void sd_write_card_data(SDState *sd, uint8_t value)
>>   {
>>       int i;
>>   
>> @@ -1510,7 +1508,7 @@ void sd_write_data(SDState *sd, uint8_t value)
>>           return;
>>   
>>       if (sd->state != sd_receivingdata_state) {
>> -        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
>> +        fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n");
> Outside this patch's scope, but here goes anyway: what kind of condition
> is reported here?  Programming error that should never happen?  Guest
> doing something weird?
>
> Same for all the other fprintf(stderr, ...) in this file.
>
>>           return;
>>       }
>>   
>> @@ -1621,12 +1619,12 @@ void sd_write_data(SDState *sd, uint8_t value)
>>           break;
>>   
>>       default:
>> -        fprintf(stderr, "sd_write_data: unknown command\n");
>> +        fprintf(stderr, "sd_write_card_data: unknown command\n");
>>           break;
>>       }
>>   }
>>   
>> -uint8_t sd_read_data(SDState *sd)
>> +static uint8_t sd_read_card_data(SDState *sd)
>>   {
>>       /* TODO: Append CRCs */
>>       uint8_t ret;
>> @@ -1636,7 +1634,7 @@ uint8_t sd_read_data(SDState *sd)
>>           return 0x00;
>>   
>>       if (sd->state != sd_sendingdata_state) {
>> -        fprintf(stderr, "sd_read_data: not in Sending-Data state\n");
>> +        fprintf(stderr, "sd_read_card_data: not in Sending-Data state\n");
>>           return 0x00;
>>       }
>>   
>> @@ -1738,19 +1736,47 @@ uint8_t sd_read_data(SDState *sd)
>>           break;
>>   
>>       default:
>> -        fprintf(stderr, "sd_read_data: unknown command\n");
>> +        fprintf(stderr, "sd_read_card_data: unknown command\n");
>>           return 0x00;
>>       }
>>   
>>       return ret;
>>   }
>>   
>> -bool sd_data_ready(SDState *sd)
>> +static bool sd_is_data_ready(SDState *sd)
>>   {
>>       return sd->state == sd_sendingdata_state;
>>   }
>>   
>> -void sd_enable(SDState *sd, bool enable)
>> +static void sd_enable_card(SDState *sd, bool enable)
>>   {
>>       sd->enable = enable;
>>   }
>> +
>> +static void sd_class_init(ObjectClass *klass, void *data)
>> +{
>> +    SDClass *k = SD_CLASS(klass);
>> +
>> +    k->init = sd_init_card;
>> +    k->set_cb = sd_set_callbacks;
>> +    k->do_command = sd_send_command;
>> +    k->data_ready = sd_is_data_ready;
>> +    k->read_data = sd_read_card_data;
>> +    k->write_data = sd_write_card_data;
>> +    k->enable = sd_enable_card;
>> +}
>> +
>> +static const TypeInfo sd_type_info = {
>> +    .name = TYPE_SD_CARD,
>> +    .parent = TYPE_OBJECT,
> Possibly ignorant question: why TYPE_OBJECT, not TYPE_DEVICE?

QEMU requires all objects derived from TYPE_DEVICE to be connected to 
some bus, if no bus was specified in new object class description, QEMU 
practically assumes this object to be a sysbus device and connects it to 
main system bus.
A while ago it wasn't even possible to create a class directly derived 
from DEVICE_CLASS without tying this class to some bus, QEMU would have 
abort() during initialization. Now, after "bus_info" member was removed 
from DeviceClass structure, it became possible, but still, it most 
definitely will cause errors because QEMU will assume such an object to 
be a SysBusDevice. For example, sysbus_dev_print() (called by "info 
qtree" monitor command) directly casts DeviceState object to 
SysBusDevice without checking if it is actually possible.

My point is, to make SD of TYPE_DEVICE we need to implement SD bus. I 
have no idea what we need it for and what is it supposed to do, I think 
we can leave it for further improvement.

>> +    .instance_size = sizeof(SDState),
>> +    .class_init = sd_class_init,
>> +    .class_size = sizeof(SDClass)
>> +};
>> +
>> +static void sd_register_types(void)
>> +{
>> +    type_register_static(&sd_type_info);
>> +}
>> +
>> +type_init(sd_register_types)
>> diff --git a/hw/sd.h b/hw/sd.h
>> index 4eb9679..f84e863 100644
>> --- a/hw/sd.h
>> +++ b/hw/sd.h
>> @@ -29,6 +29,9 @@
>>   #ifndef __hw_sd_h
>>   #define __hw_sd_h		1
>>   
>> +#include "qemu-common.h"
>> +#include "qemu/object.h"
>> +
>>   #define OUT_OF_RANGE		(1 << 31)
>>   #define ADDRESS_ERROR		(1 << 30)
>>   #define BLOCK_LEN_ERROR		(1 << 29)
>> @@ -67,13 +70,61 @@ typedef struct {
>>   
>>   typedef struct SDState SDState;
>>   
>> -SDState *sd_init(BlockDriverState *bs, bool is_spi);
>> -int sd_do_command(SDState *sd, SDRequest *req,
>> -                  uint8_t *response);
>> -void sd_write_data(SDState *sd, uint8_t value);
>> -uint8_t sd_read_data(SDState *sd);
>> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
>> -bool sd_data_ready(SDState *sd);
>> -void sd_enable(SDState *sd, bool enable);
>> +typedef struct SDClass {
>> +    ObjectClass parent_class;
>> +
>> +    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
>> +    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
>> +    void (*write_data)(SDState *sd, uint8_t value);
>> +    uint8_t (*read_data)(SDState *sd);
>> +    void (*set_cb)(SDState *sd, qemu_irq readonly, qemu_irq insert);
>> +    bool (*data_ready)(SDState *sd);
>> +    void (*enable)(SDState *sd, bool enable);
>> +} SDClass;
>> +
>> +#define TYPE_SD_CARD            "sd-card"
>> +#define SD_CARD(obj)            \
>> +     OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
>> +#define SD_CLASS(klass)         \
>> +     OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD_CARD)
>> +#define SD_GET_CLASS(obj)       \
>> +     OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD_CARD)
>> +
>> +static inline SDState *sd_init(BlockDriverState *bs, bool is_spi)
>> +{
>> +    SDState *sd = SD_CARD(object_new(TYPE_SD_CARD));
>> +    SD_GET_CLASS(sd)->init(sd, bs, is_spi);
>> +    return sd;
> Shouldn't bs and spi be properties?  Oh, that's coming in PATCH
> 10-11/12.
>
>> +}
>> +
>> +static inline int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response)
>> +{
>> +    return SD_GET_CLASS(sd)->do_command(sd, req, response);
>> +}
>> +
>> +static inline uint8_t sd_read_data(SDState *sd)
>> +{
>> +    return SD_GET_CLASS(sd)->read_data(sd);
>> +}
>> +
>> +static inline void sd_write_data(SDState *sd, uint8_t value)
>> +{
>> +    SD_GET_CLASS(sd)->write_data(sd, value);
>> +}
>> +
>> +static inline bool sd_data_ready(SDState *sd)
>> +{
>> +    return SD_GET_CLASS(sd)->data_ready(sd);
>> +}
>> +
>> +static inline void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>> +{
>> +    SD_GET_CLASS(sd)->set_cb(sd, readonly, insert);
>> +}
>> +
>> +static inline void sd_enable(SDState *sd, bool enable)
>> +{
>> +    SD_GET_CLASS(sd)->enable(sd, enable);
>> +}
>>   
>>   #endif	/* __hw_sd_h */
Markus Armbruster - July 31, 2012, 3:29 p.m.
Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> On 07/31/2012 01:45 PM, Markus Armbruster wrote:
>> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>>
>>> A straightforward conversion of SD card implementation to a proper QEMU object.
>>> Wrapper functions were introduced for SDClass methods in order to avoid SD card
>>> users modification. Because of this, name change for several functions in hw/sd.c
>>> was required.
>>>
>>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>>> ---
>>>   hw/sd.c |   56 ++++++++++++++++++++++++++++++++++++++--------------
>>>   hw/sd.h |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>   2 files changed, 100 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/sd.c b/hw/sd.c
>>> index f8ab045..fe2c138 100644
>>> --- a/hw/sd.c
>>> +++ b/hw/sd.c
>>> @@ -75,6 +75,8 @@ enum {
>>>   };
>>>     struct SDState {
>>> +    Object parent_obj;
>>> +
>>>       uint32_t mode;
>>>       int32_t state;
>>>       uint32_t ocr;
>>> @@ -489,11 +491,8 @@ static const VMStateDescription sd_vmstate = {
>>>      whether card should be in SSI or MMC/SD mode.  It is also up to the
>>>      board to ensure that ssi transfers only occur when the chip select
>>>      is asserted.  */
>>> -SDState *sd_init(BlockDriverState *bs, bool is_spi)
>>> +static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
>>>   {
>>> -    SDState *sd;
>>> -
>>> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>>>       sd->buf = qemu_blockalign(bs, 512);
>>>       sd->spi = is_spi;
>>>       sd->enable = true;
>>> @@ -503,10 +502,9 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
>>>           bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
>>>       }
>>>       vmstate_register(NULL, -1, &sd_vmstate, sd);
>>> -    return sd;
>>>   }
>>>   -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>> +static void sd_set_callbacks(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>>   {
>>>       sd->readonly_cb = readonly;
>>>       sd->inserted_cb = insert;
>>> @@ -1334,7 +1332,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>>>       return sd_cmd_class[req->cmd] == 0 || sd_cmd_class[req->cmd] == 7;
>>>   }
>>>   -int sd_do_command(SDState *sd, SDRequest *req,
>>> +static int sd_send_command(SDState *sd, SDRequest *req,
>>>                     uint8_t *response) {
>>>       int last_state;
>>>       sd_rsp_type_t rtype;
>>> @@ -1502,7 +1500,7 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>>>   #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
>>>   #define APP_WRITE_BLOCK(a, len)
>>>   -void sd_write_data(SDState *sd, uint8_t value)
>>> +static void sd_write_card_data(SDState *sd, uint8_t value)
>>>   {
>>>       int i;
>>>   @@ -1510,7 +1508,7 @@ void sd_write_data(SDState *sd, uint8_t
>>> value)
>>>           return;
>>>         if (sd->state != sd_receivingdata_state) {
>>> -        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
>>> +        fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n");
>> Outside this patch's scope, but here goes anyway: what kind of condition
>> is reported here?  Programming error that should never happen?  Guest
>> doing something weird?
>>
>> Same for all the other fprintf(stderr, ...) in this file.
>>
>>>           return;
>>>       }
>>>   @@ -1621,12 +1619,12 @@ void sd_write_data(SDState *sd, uint8_t
>>> value)
>>>           break;
>>>         default:
>>> -        fprintf(stderr, "sd_write_data: unknown command\n");
>>> +        fprintf(stderr, "sd_write_card_data: unknown command\n");
>>>           break;
>>>       }
>>>   }
>>>   -uint8_t sd_read_data(SDState *sd)
>>> +static uint8_t sd_read_card_data(SDState *sd)
>>>   {
>>>       /* TODO: Append CRCs */
>>>       uint8_t ret;
>>> @@ -1636,7 +1634,7 @@ uint8_t sd_read_data(SDState *sd)
>>>           return 0x00;
>>>         if (sd->state != sd_sendingdata_state) {
>>> -        fprintf(stderr, "sd_read_data: not in Sending-Data state\n");
>>> +        fprintf(stderr, "sd_read_card_data: not in Sending-Data state\n");
>>>           return 0x00;
>>>       }
>>>   @@ -1738,19 +1736,47 @@ uint8_t sd_read_data(SDState *sd)
>>>           break;
>>>         default:
>>> -        fprintf(stderr, "sd_read_data: unknown command\n");
>>> +        fprintf(stderr, "sd_read_card_data: unknown command\n");
>>>           return 0x00;
>>>       }
>>>         return ret;
>>>   }
>>>   -bool sd_data_ready(SDState *sd)
>>> +static bool sd_is_data_ready(SDState *sd)
>>>   {
>>>       return sd->state == sd_sendingdata_state;
>>>   }
>>>   -void sd_enable(SDState *sd, bool enable)
>>> +static void sd_enable_card(SDState *sd, bool enable)
>>>   {
>>>       sd->enable = enable;
>>>   }
>>> +
>>> +static void sd_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    SDClass *k = SD_CLASS(klass);
>>> +
>>> +    k->init = sd_init_card;
>>> +    k->set_cb = sd_set_callbacks;
>>> +    k->do_command = sd_send_command;
>>> +    k->data_ready = sd_is_data_ready;
>>> +    k->read_data = sd_read_card_data;
>>> +    k->write_data = sd_write_card_data;
>>> +    k->enable = sd_enable_card;
>>> +}
>>> +
>>> +static const TypeInfo sd_type_info = {
>>> +    .name = TYPE_SD_CARD,
>>> +    .parent = TYPE_OBJECT,
>> Possibly ignorant question: why TYPE_OBJECT, not TYPE_DEVICE?
>
> QEMU requires all objects derived from TYPE_DEVICE to be connected to
> some bus, if no bus was specified in new object class description,
> QEMU practically assumes this object to be a sysbus device and
> connects it to main system bus.
> A while ago it wasn't even possible to create a class directly derived
> from DEVICE_CLASS without tying this class to some bus, QEMU would
> have abort() during initialization. Now, after "bus_info" member was
> removed from DeviceClass structure, it became possible, but still, it
> most definitely will cause errors because QEMU will assume such an
> object to be a SysBusDevice. For example, sysbus_dev_print() (called
> by "info qtree" monitor command) directly casts DeviceState object to
> SysBusDevice without checking if it is actually possible.

I'm afraid the first few device models that don't connect to a qbus are
bound to flush out a few bugs.  Nevertheless, device models should be
subtypes of TYPE_DEVICE, shouldn't they?  Anthony?

> My point is, to make SD of TYPE_DEVICE we need to implement SD bus. I
> have no idea what we need it for and what is it supposed to do, I
> think we can leave it for further improvement.

No, to make SD a sub of TYPE_DEVICE, we need to fix the qdev remaining
bugs around qbus-less device-device connections.

[...]
Peter Maydell - July 31, 2012, 4:17 p.m.
On 31 July 2012 16:29, Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>> QEMU requires all objects derived from TYPE_DEVICE to be connected to
>> some bus, if no bus was specified in new object class description,
>> QEMU practically assumes this object to be a sysbus device and
>> connects it to main system bus.
>> A while ago it wasn't even possible to create a class directly derived
>> from DEVICE_CLASS without tying this class to some bus, QEMU would
>> have abort() during initialization. Now, after "bus_info" member was
>> removed from DeviceClass structure, it became possible, but still, it
>> most definitely will cause errors because QEMU will assume such an
>> object to be a SysBusDevice. For example, sysbus_dev_print() (called
>> by "info qtree" monitor command) directly casts DeviceState object to
>> SysBusDevice without checking if it is actually possible.
>
> I'm afraid the first few device models that don't connect to a qbus are
> bound to flush out a few bugs.  Nevertheless, device models should be
> subtypes of TYPE_DEVICE, shouldn't they?  Anthony?

Sounds right to me. Added bonus, we can use nice APIs for declaring
and setting properties (qdev_prop_set_*) rather than nasty ones
(object_property-set_*) :-)

-- PMM
Mitsyanko Igor - July 31, 2012, 5:09 p.m.
On 07/31/2012 08:17 PM, Peter Maydell wrote:
> On 31 July 2012 16:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>>> QEMU requires all objects derived from TYPE_DEVICE to be connected to
>>> some bus, if no bus was specified in new object class description,
>>> QEMU practically assumes this object to be a sysbus device and
>>> connects it to main system bus.
>>> A while ago it wasn't even possible to create a class directly derived
>>> from DEVICE_CLASS without tying this class to some bus, QEMU would
>>> have abort() during initialization. Now, after "bus_info" member was
>>> removed from DeviceClass structure, it became possible, but still, it
>>> most definitely will cause errors because QEMU will assume such an
>>> object to be a SysBusDevice. For example, sysbus_dev_print() (called
>>> by "info qtree" monitor command) directly casts DeviceState object to
>>> SysBusDevice without checking if it is actually possible.
>> I'm afraid the first few device models that don't connect to a qbus are
>> bound to flush out a few bugs.  Nevertheless, device models should be
>> subtypes of TYPE_DEVICE, shouldn't they?  Anthony?
> Sounds right to me. Added bonus, we can use nice APIs for declaring
> and setting properties (qdev_prop_set_*) rather than nasty ones
> (object_property-set_*) :-)
>
> -- PMM
>

Just to clarify things for myself, system bus is supposed to go away in 
the future, but nobody knows when exactly since many things in qemu 
seriously intertwined with it. Bussless devices are presumably 
supported, but that support uses a "hack" of tying bussless devices to 
main system bus so they could be visible to the rest of the system. 
You're proposing to derive TYPE_SD directly from TYPE_DEVICE without 
specifying a bus for it, incidentally fixing any existing bugs in 
current bussless devices support.

Patch

diff --git a/hw/sd.c b/hw/sd.c
index f8ab045..fe2c138 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -75,6 +75,8 @@  enum {
 };
 
 struct SDState {
+    Object parent_obj;
+
     uint32_t mode;
     int32_t state;
     uint32_t ocr;
@@ -489,11 +491,8 @@  static const VMStateDescription sd_vmstate = {
    whether card should be in SSI or MMC/SD mode.  It is also up to the
    board to ensure that ssi transfers only occur when the chip select
    is asserted.  */
-SDState *sd_init(BlockDriverState *bs, bool is_spi)
+static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
 {
-    SDState *sd;
-
-    sd = (SDState *) g_malloc0(sizeof(SDState));
     sd->buf = qemu_blockalign(bs, 512);
     sd->spi = is_spi;
     sd->enable = true;
@@ -503,10 +502,9 @@  SDState *sd_init(BlockDriverState *bs, bool is_spi)
         bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
     }
     vmstate_register(NULL, -1, &sd_vmstate, sd);
-    return sd;
 }
 
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
+static void sd_set_callbacks(SDState *sd, qemu_irq readonly, qemu_irq insert)
 {
     sd->readonly_cb = readonly;
     sd->inserted_cb = insert;
@@ -1334,7 +1332,7 @@  static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
     return sd_cmd_class[req->cmd] == 0 || sd_cmd_class[req->cmd] == 7;
 }
 
-int sd_do_command(SDState *sd, SDRequest *req,
+static int sd_send_command(SDState *sd, SDRequest *req,
                   uint8_t *response) {
     int last_state;
     sd_rsp_type_t rtype;
@@ -1502,7 +1500,7 @@  static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
 #define APP_WRITE_BLOCK(a, len)
 
-void sd_write_data(SDState *sd, uint8_t value)
+static void sd_write_card_data(SDState *sd, uint8_t value)
 {
     int i;
 
@@ -1510,7 +1508,7 @@  void sd_write_data(SDState *sd, uint8_t value)
         return;
 
     if (sd->state != sd_receivingdata_state) {
-        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
+        fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n");
         return;
     }
 
@@ -1621,12 +1619,12 @@  void sd_write_data(SDState *sd, uint8_t value)
         break;
 
     default:
-        fprintf(stderr, "sd_write_data: unknown command\n");
+        fprintf(stderr, "sd_write_card_data: unknown command\n");
         break;
     }
 }
 
-uint8_t sd_read_data(SDState *sd)
+static uint8_t sd_read_card_data(SDState *sd)
 {
     /* TODO: Append CRCs */
     uint8_t ret;
@@ -1636,7 +1634,7 @@  uint8_t sd_read_data(SDState *sd)
         return 0x00;
 
     if (sd->state != sd_sendingdata_state) {
-        fprintf(stderr, "sd_read_data: not in Sending-Data state\n");
+        fprintf(stderr, "sd_read_card_data: not in Sending-Data state\n");
         return 0x00;
     }
 
@@ -1738,19 +1736,47 @@  uint8_t sd_read_data(SDState *sd)
         break;
 
     default:
-        fprintf(stderr, "sd_read_data: unknown command\n");
+        fprintf(stderr, "sd_read_card_data: unknown command\n");
         return 0x00;
     }
 
     return ret;
 }
 
-bool sd_data_ready(SDState *sd)
+static bool sd_is_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
 }
 
-void sd_enable(SDState *sd, bool enable)
+static void sd_enable_card(SDState *sd, bool enable)
 {
     sd->enable = enable;
 }
+
+static void sd_class_init(ObjectClass *klass, void *data)
+{
+    SDClass *k = SD_CLASS(klass);
+
+    k->init = sd_init_card;
+    k->set_cb = sd_set_callbacks;
+    k->do_command = sd_send_command;
+    k->data_ready = sd_is_data_ready;
+    k->read_data = sd_read_card_data;
+    k->write_data = sd_write_card_data;
+    k->enable = sd_enable_card;
+}
+
+static const TypeInfo sd_type_info = {
+    .name = TYPE_SD_CARD,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SDState),
+    .class_init = sd_class_init,
+    .class_size = sizeof(SDClass)
+};
+
+static void sd_register_types(void)
+{
+    type_register_static(&sd_type_info);
+}
+
+type_init(sd_register_types)
diff --git a/hw/sd.h b/hw/sd.h
index 4eb9679..f84e863 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -29,6 +29,9 @@ 
 #ifndef __hw_sd_h
 #define __hw_sd_h		1
 
+#include "qemu-common.h"
+#include "qemu/object.h"
+
 #define OUT_OF_RANGE		(1 << 31)
 #define ADDRESS_ERROR		(1 << 30)
 #define BLOCK_LEN_ERROR		(1 << 29)
@@ -67,13 +70,61 @@  typedef struct {
 
 typedef struct SDState SDState;
 
-SDState *sd_init(BlockDriverState *bs, bool is_spi);
-int sd_do_command(SDState *sd, SDRequest *req,
-                  uint8_t *response);
-void sd_write_data(SDState *sd, uint8_t value);
-uint8_t sd_read_data(SDState *sd);
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
-bool sd_data_ready(SDState *sd);
-void sd_enable(SDState *sd, bool enable);
+typedef struct SDClass {
+    ObjectClass parent_class;
+
+    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
+    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
+    void (*write_data)(SDState *sd, uint8_t value);
+    uint8_t (*read_data)(SDState *sd);
+    void (*set_cb)(SDState *sd, qemu_irq readonly, qemu_irq insert);
+    bool (*data_ready)(SDState *sd);
+    void (*enable)(SDState *sd, bool enable);
+} SDClass;
+
+#define TYPE_SD_CARD            "sd-card"
+#define SD_CARD(obj)            \
+     OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
+#define SD_CLASS(klass)         \
+     OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD_CARD)
+#define SD_GET_CLASS(obj)       \
+     OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD_CARD)
+
+static inline SDState *sd_init(BlockDriverState *bs, bool is_spi)
+{
+    SDState *sd = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(sd)->init(sd, bs, is_spi);
+    return sd;
+}
+
+static inline int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response)
+{
+    return SD_GET_CLASS(sd)->do_command(sd, req, response);
+}
+
+static inline uint8_t sd_read_data(SDState *sd)
+{
+    return SD_GET_CLASS(sd)->read_data(sd);
+}
+
+static inline void sd_write_data(SDState *sd, uint8_t value)
+{
+    SD_GET_CLASS(sd)->write_data(sd, value);
+}
+
+static inline bool sd_data_ready(SDState *sd)
+{
+    return SD_GET_CLASS(sd)->data_ready(sd);
+}
+
+static inline void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
+{
+    SD_GET_CLASS(sd)->set_cb(sd, readonly, insert);
+}
+
+static inline void sd_enable(SDState *sd, bool enable)
+{
+    SD_GET_CLASS(sd)->enable(sd, enable);
+}
 
 #endif	/* __hw_sd_h */