Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Alistair Francis <alistair.francis@xilinx.com>]

On Thu, Dec 17, 2015 at 2:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 December 2015 at 21:45, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Turn the SD card into a QOM device.
>>> This conversion only changes the device itself; the various
>>> functions which are effectively methods on the device are not
>>> touched at this point.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>>> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
>>>      }
>>>  };
>>>
>>> -/* We do not model the chip select pin, so allow the board to select
>>> -   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.  */
>>> +/* Legacy initialization function for use by non-qdevified callers */
>>>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>>>  {
>>> -    SDState *sd;
>>> +    DeviceState *dev;
>>> +    Error *err = NULL;
>>>
>>> -    if (blk && blk_is_read_only(blk)) {
>>> -        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
>>> +    dev = qdev_create(NULL, TYPE_SD);
>>> +    qdev_prop_set_drive(dev, "drive", blk, &err);
>>> +    if (err) {
>>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>>>          return NULL;
>>>      }
>>> -
>>> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>>> -    sd->buf = blk_blockalign(blk, 512);
>>> -    sd->spi = is_spi;
>>> -    sd->enable = true;
>>> -    sd->blk = blk;
>>> -    sd_reset(sd);
>>> -    if (sd->blk) {
>>> -        /* Attach dev if not already attached.  (This call ignores an
>>> -         * error return code if sd->blk is already attached.) */
>>> -        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
>>> -        blk_attach_dev(sd->blk, sd);
>>
>> This functionality is removed with this patch. If the block is not
>> already attached won't this cause errors?
>
> The block backend is attached when we set the "drive" property
> (which we do in this function in the new code just above).
> [the actual call to blk_attach_dev() is in parse_drive() in
> hw/core/qdev-properties-system.c.]
>
> The blk_set_dev_ops() moves in this patch to sd_realize().
>
> There is incidentally no longer any case where the block backend
> could be already attached when we call sd_init(), because the
> only case there was when it had been attached to the sdhci
> controller device because of the x-drive property on that device,
> and we removed the property in the previous patch. It's exactly
> because setting a drive property does an implicit blk_attach_dev
> that this code previously had to have special casing for
> "attach of an already attached blk".

Ok, fair enough, just thought I would check. The rest of the logic
looks good then.

Thanks,

Alistair

>
>>> -        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>>> +    qdev_prop_set_bit(dev, "spi", is_spi);
>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>> +    if (err) {
>>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>>> +        return NULL;
>>>      }
>>> -    vmstate_register(NULL, -1, &sd_vmstate, sd);
>>> -    return sd;
>>> +
>>> +    return SD(dev);
>>>  }
>>>
>>>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
>>>  {
>>>      sd->enable = enable;
>>>  }
>>> +
>>> +static void sd_instance_init(Object *obj)
>>> +{
>>> +    SDState *sd = SD(obj);
>>> +
>>> +    sd->enable = true;
>>> +}
>>> +
>>> +static void sd_realize(DeviceState *dev, Error ** errp)
>>
>> You have one too many spaces here.
>
> Yep, will fix.
>
> thanks
> -- PMM
>