RE: [PATCH v2 01/22] block: add eMMC block device type

[Sai Pavan Boddu]

Hi Cedric,


> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Wednesday, February 24, 2021 7:25 PM
> To: Stefan Hajnoczi <stefanha@redhat.com>; Sai Pavan Boddu
> <saipava@xilinx.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>; Markus Armbruster
> <armbru@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com>; Eric Blake <eblake@redhat.com>; Joel Stanley
> <joel@jms.id.au>; Vincent Palatin <vpalatin@chromium.org>; Dr. David Alan
> Gilbert <dgilbert@redhat.com>; Thomas Huth <thuth@redhat.com>; Peter
> Maydell <peter.maydell@linaro.org>; Alistair Francis
> <alistair.francis@wdc.com>; Edgar Iglesias <edgari@xilinx.com>; Luc Michel
> <luc.michel@greensocs.com>; Paolo Bonzini <pbonzini@redhat.com>; qemu-
> devel@nongnu.org; qemu-block@nongnu.org
> Subject: Re: [PATCH v2 01/22] block: add eMMC block device type
> 
> On 2/24/21 12:40 PM, Stefan Hajnoczi wrote:
> > On Tue, Feb 23, 2021 at 05:35:20PM +0000, Sai Pavan Boddu wrote:
> >> Hi Philippe,
> >>
> >>> -----Original Message-----
> >>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>> Sent: Monday, February 22, 2021 5:34 PM
> >>> To: Sai Pavan Boddu <saipava@xilinx.com>; Markus Armbruster
> >>> <armbru@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> >>> <mreitz@redhat.com>; Vladimir Sementsov-Ogievskiy
> >>> <vsementsov@virtuozzo.com>; Eric Blake <eblake@redhat.com>; Joel
> >>> Stanley <joel@jms.id.au>; Cédric Le Goater <clg@kaod.org>; Vincent
> >>> Palatin <vpalatin@chromium.org>; Dr. David Alan Gilbert
> >>> <dgilbert@redhat.com>; Thomas Huth <thuth@redhat.com>; Stefan
> >>> Hajnoczi <stefanha@redhat.com>; Peter Maydell
> >>> <peter.maydell@linaro.org>; Alistair Francis
> >>> <alistair.francis@wdc.com>; Edgar Iglesias <edgari@xilinx.com>; Luc
> >>> Michel <luc.michel@greensocs.com>; Paolo Bonzini
> >>> <pbonzini@redhat.com>
> >>> Cc: Sai Pavan Boddu <saipava@xilinx.com>; qemu-devel@nongnu.org;
> >>> qemu- block@nongnu.org
> >>> Subject: Re: [PATCH v2 01/22] block: add eMMC block device type
> >>>
> >>> On 2/22/21 9:20 AM, Sai Pavan Boddu wrote:
> >>>> From: Vincent Palatin <vpalatin@chromium.org>
> >>>>
> >>>> Add new block device type.
> >>>>
> >>>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> >>>> [SPB: Rebased over 5.1 version]
> >>>> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> >>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >>>> ---
> >>>>  include/sysemu/blockdev.h | 1 +
> >>>>  blockdev.c                | 1 +
> >>>>  2 files changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> >>>> index 3b5fcda..eefae9f 100644
> >>>> --- a/include/sysemu/blockdev.h
> >>>> +++ b/include/sysemu/blockdev.h
> >>>> @@ -24,6 +24,7 @@ typedef enum {
> >>>>       */
> >>>>      IF_NONE = 0,
> >>>>      IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD,
> >>>> IF_VIRTIO, IF_XEN,
> >>>> +    IF_EMMC,
> >>>>      IF_COUNT
> >>>>  } BlockInterfaceType;
> >>>>
> >>>> diff --git a/blockdev.c b/blockdev.c index cd438e6..390d43c 100644
> >>>> --- a/blockdev.c
> >>>> +++ b/blockdev.c
> >>>> @@ -83,6 +83,7 @@ static const char *const if_name[IF_COUNT] = {
> >>>>      [IF_SD] = "sd",
> >>>>      [IF_VIRTIO] = "virtio",
> >>>>      [IF_XEN] = "xen",
> >>>> +    [IF_EMMC] = "emmc",
> >>>>  };
> >>>
> >>> We don't need to introduce support for the legacy -drive magic.
> >>>
> >>> -device should be enough for this device, right?
> >> [Sai Pavan Boddu] I was seeing to use -device for emmc. But I see we
> anyway need blockdev support for this, which would require us the use -drive.
> >>
> >> Can you give some pointers, how to approach this ?
> >
> > It is probably not necessary to add a new IF_ constant. Would this work:
> >
> >   -drive if=none,id=emmc0,file=test.img,format=raw
> >   -device emmc,...,drive=emmc0
> >
> > Or the more modern:
> >
> >   -blockdev node-name=emmc0,driver=file,filename=test.img
> >   -device emmc,...,drive=emmc0
> >
> > ?
> >
> > (The syntax might need small tweaks but is shows the general idea.)
> 
> Yes. This is better.
> 
> We could have an "emmc" device inheriting from "sd-card". The "emmc"
> property would not be necessary anymore and may be, we could cleanup up
> some parts doing :
> 
>     if (sd->emmc) { /* eMMC */
>         ...
>     } else {
> 
>     }
> 
> with SDCardClass handlers. the SWITCH_FUNCTION command is a good
> candidate, CMD8 also.
[Sai Pavan Boddu] Nice, this approach looks clean.
But we still may be depending on emmc property. Not sure!

I would get back with v3, your review on those patches would be great.

Thanks & Regards,
Sai Pavan
> 
> C.