Message ID | 1613982039-13861-2-git-send-email-sai.pavan.boddu@xilinx.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | eMMC support | expand |
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?
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > 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? External interface extensions need rationale: why do we want / need it? The commit message neglects to provide one. Even more so when the interface in question is in a state like -drive is.
* Markus Armbruster (armbru@redhat.com) wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > > 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? > > External interface extensions need rationale: why do we want / need it? > The commit message neglects to provide one. > > Even more so when the interface in question is in a state like -drive > is. I wouldn't be too nasty about -drive; for me I still find it the easiest way to start a VM. Dave
On 2/22/21 2:16 PM, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >> >>> 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? >> >> External interface extensions need rationale: why do we want / need it? >> The commit message neglects to provide one. >> >> Even more so when the interface in question is in a state like -drive >> is. > > I wouldn't be too nasty about -drive; for me I still find it the > easiest way to start a VM. But eMMC isn't a bus where you can plug drives, it is soldered on-board and is mmio mapped to a fixed address. I don't see the point of having a drive interface for it...
Hi Philippe, > -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@redhat.com> > Sent: Monday, February 22, 2021 6:54 PM > To: Dr. David Alan Gilbert <dgilbert@redhat.com>; Markus Armbruster > <armbru@redhat.com> > Cc: Sai Pavan Boddu <saipava@xilinx.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>; 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>; 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 2:16 PM, Dr. David Alan Gilbert wrote: > > * Markus Armbruster (armbru@redhat.com) wrote: > >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> > >>> 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? > >> > >> External interface extensions need rationale: why do we want / need it? > >> The commit message neglects to provide one. > >> > >> Even more so when the interface in question is in a state like -drive > >> is. > > > > I wouldn't be too nasty about -drive; for me I still find it the > > easiest way to start a VM. > > But eMMC isn't a bus where you can plug drives, it is soldered on-board and is > mmio mapped to a fixed address. I don't see the point of having a drive > interface for it... [Sai Pavan Boddu] Yeah, this makes sense but having a drive would be a simple implementation without disturbing much in the sd card emulation code. And its just easy to use, just as how sd cards are inserted. I need to see, how easy it would be with -device. Thanks, Sai Pavan
* Sai Pavan Boddu (saipava@xilinx.com) wrote: > Hi Philippe, > > > -----Original Message----- > > From: Philippe Mathieu-Daudé <philmd@redhat.com> > > Sent: Monday, February 22, 2021 6:54 PM > > To: Dr. David Alan Gilbert <dgilbert@redhat.com>; Markus Armbruster > > <armbru@redhat.com> > > Cc: Sai Pavan Boddu <saipava@xilinx.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>; 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>; 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 2:16 PM, Dr. David Alan Gilbert wrote: > > > * Markus Armbruster (armbru@redhat.com) wrote: > > >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > >> > > >>> 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? > > >> > > >> External interface extensions need rationale: why do we want / need it? > > >> The commit message neglects to provide one. > > >> > > >> Even more so when the interface in question is in a state like -drive > > >> is. > > > > > > I wouldn't be too nasty about -drive; for me I still find it the > > > easiest way to start a VM. > > > > But eMMC isn't a bus where you can plug drives, it is soldered on-board and is > > mmio mapped to a fixed address. I don't see the point of having a drive > > interface for it... > [Sai Pavan Boddu] Yeah, this makes sense but having a drive would be a simple implementation without disturbing much in the sd card emulation code. And its just easy to use, just as how sd cards are inserted. > > I need to see, how easy it would be with -device. Lets see what your command line looks like for starting it with emmc. Dave > Thanks, > Sai Pavan >
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 ? Regards, Sai Pavan
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.) Stefan
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. C.
Hi Stefan > -----Original Message----- > From: Stefan Hajnoczi <stefanha@redhat.com> > Sent: Wednesday, February 24, 2021 5:10 PM > To: 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>; Cédric Le Goater <clg@kaod.org>; 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 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 [Sai Pavan Boddu] Great, this works for me. > > Or the more modern: > > -blockdev node-name=emmc0,driver=file,filename=test.img > -device emmc,...,drive=emmc0 [Sai Pavan Boddu] Thanks, I would try to follow the modern approach then! Regards, Sai Pavan > > ? > > (The syntax might need small tweaks but is shows the general idea.) > > Stefan
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.
On 2/24/21 8:13 PM, Sai Pavan Boddu wrote: > 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! It could be a SDCardClass attribute instead since it's a constant for the emmc device. C. > > I would get back with v3, your review on those patches would be great. > > Thanks & Regards, > Sai Pavan >> >> C.
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", }; static int if_max_devs[IF_COUNT] = {