Patchwork [4/5] Add generic drive hotplugging

login
register
mail settings
Submitter Alexander Graf
Date Aug. 23, 2010, 10:02 p.m.
Message ID <1282600951-30803-5-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/62517/
State New
Headers show

Comments

Alexander Graf - Aug. 23, 2010, 10:02 p.m.
The monitor command for hotplugging is in i386 specific code. This is just
plain wrong, as S390 just learned how to do hotplugging too and needs to
get drives for that.

So let's add a generic copy to generic code that handles drive_add in a
way that doesn't have pci dependencies.

I'm not fully happy with the patch as is. IMHO there should only be a
single target agnostic drive_hot_add function available. How we could
potentially fit IF_SCSI in there I don't know though.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/device-hotplug.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)
Anthony Liguori - Aug. 23, 2010, 10:21 p.m.
On 08/23/2010 05:02 PM, Alexander Graf wrote:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
>
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
>
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.
>
> Signed-off-by: Alexander Graf<agraf@suse.de>
>    

I think you really want device_add plus a blockdev_add.

Regards,

Anthony Liguori

> ---
>   hw/device-hotplug.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index c1a9a56..f311c7f 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -25,6 +25,9 @@
>   #include "hw.h"
>   #include "boards.h"
>   #include "net.h"
> +#include "qemu-config.h"
> +#include "sysemu.h"
> +#include "monitor.h"
>
>   DriveInfo *add_init_drive(const char *optstr)
>   {
> @@ -44,3 +47,43 @@ DriveInfo *add_init_drive(const char *optstr)
>
>       return dinfo;
>   }
> +
> +#if !defined(TARGET_I386)
> +
> +/*
> + * This version of drive_hot_add does not know anything about PCI specifics.
> + * It is used as fallback on architectures that don't implement pci-hotplug.
> + */
> +void drive_hot_add(Monitor *mon, const QDict *qdict)
> +{
> +    int type;
> +    DriveInfo *dinfo = NULL;
> +    const char *opts = qdict_get_str(qdict, "opts");
> +
> +    dinfo = add_init_drive(opts);
> +    if (!dinfo)
> +        goto err;
> +    if (dinfo->devaddr) {
> +        monitor_printf(mon, "Parameter addr not supported\n");
> +        goto err;
> +    }
> +    type = dinfo->type;
> +
> +    switch (type) {
> +    case IF_NONE:
> +        monitor_printf(mon, "OK\n");
> +        break;
> +    default:
> +        monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
> +        goto err;
> +    }
> +    return;
> +
> +err:
> +    if (dinfo)
> +        drive_uninit(dinfo);
> +    return;
> +}
> +
> +#endif /* !defined(TARGET_I386) */
> +
>
Alexander Graf - Aug. 23, 2010, 10:23 p.m.
On 24.08.2010, at 00:21, Anthony Liguori wrote:

> On 08/23/2010 05:02 PM, Alexander Graf wrote:
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>> 
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>> 
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>> 
>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>   
> 
> I think you really want device_add plus a blockdev_add.

Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.

What is blockdev_add supposed to be? drive_add without IF_SCSI?


Alex
Alexander Graf - Aug. 23, 2010, 10:45 p.m.
On 24.08.2010, at 00:23, Alexander Graf wrote:

> 
> On 24.08.2010, at 00:21, Anthony Liguori wrote:
> 
>> On 08/23/2010 05:02 PM, Alexander Graf wrote:
>>> The monitor command for hotplugging is in i386 specific code. This is just
>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>>> get drives for that.
>>> 
>>> So let's add a generic copy to generic code that handles drive_add in a
>>> way that doesn't have pci dependencies.
>>> 
>>> I'm not fully happy with the patch as is. IMHO there should only be a
>>> single target agnostic drive_hot_add function available. How we could
>>> potentially fit IF_SCSI in there I don't know though.
>>> 
>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>> 
>> 
>> I think you really want device_add plus a blockdev_add.
> 
> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
> 
> What is blockdev_add supposed to be? drive_add without IF_SCSI?

To be a bit more precise on how things work with this set:

(qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
OK
(qemu) device_add virtio-blk-s390,drive=my_disk,id=new_disk

gives me a working new virtio disk in the VM that's mapped to /dev/null :).


Alex
Anthony Liguori - Aug. 23, 2010, 10:50 p.m.
On 08/23/2010 05:45 PM, Alexander Graf wrote:
>> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
>>
>> What is blockdev_add supposed to be? drive_add without IF_SCSI?

drive_add without if= and without the PCI-isms.

>> To be a bit more precise on how things work with this set:
>>
>> (qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
>> OK
>>      

In theory, something like:

(qemu) blockdev_add id=my_disk,file=/dev/null

Regards,

Anthony Liguori

>> (qemu) device_add virtio-blk-s390,drive=my_disk,id=new_disk
>>
>> gives me a working new virtio disk in the VM that's mapped to /dev/null :).
>>
>>
>> Alex
>>
Alexander Graf - Aug. 23, 2010, 10:54 p.m.
On 24.08.2010, at 00:50, Anthony Liguori wrote:

> On 08/23/2010 05:45 PM, Alexander Graf wrote:
>>> Device_add already works with this set and only required minor changes the s390 specific code. So that part was pretty slick :). The part that didn't work was the drive_add one.
>>> 
>>> What is blockdev_add supposed to be? drive_add without IF_SCSI?
> 
> drive_add without if= and without the PCI-isms.
> 
>>> To be a bit more precise on how things work with this set:
>>> 
>>> (qemu) drive_add 0 id=my_disk,if=none,file=/dev/null
>>> OK
>>>     
> 
> In theory, something like:
> 
> (qemu) blockdev_add id=my_disk,file=/dev/null

So why add yet another command when we could use the existing drive_add? Wouldn't it make more sense to deprecate if=!none, move forward from there, make if=none the default and around 0.16 or so drop if= from drive_add?


Alex
Daniel P. Berrange - Aug. 24, 2010, 9:31 a.m.
On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
> 
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
> 
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.

I'm not sure that this patch is actually neccessary. Via a undocumented,
sick, dirty hack, you can already use the current drive_add command
without a PCI address, for both virtio + scsi. In fact not using the
PCI address with drive_add is the preferred approach in the new qdev
world even on x86

The key is that you should use  if=none for all cases. Here are two
examples of how libvirt does it currently:

VirtIO:

  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
  device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'

SCSI:

  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
  device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1

The 'dummy' value there can be absolutely anything you want.
It is totaly ignored when QEMU sees if=none in 2nd arg.

Regards,
Daniel
Alexander Graf - Aug. 24, 2010, 10:45 a.m.
Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
>   
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>>
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>>
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>>     
>
> I'm not sure that this patch is actually neccessary. Via a undocumented,
> sick, dirty hack, you can already use the current drive_add command
> without a PCI address, for both virtio + scsi. In fact not using the
> PCI address with drive_add is the preferred approach in the new qdev
> world even on x86
>   

It is certainly necessary since the current code is in a big fat #if
defined(TARGET_I386) block :).

> The key is that you should use  if=none for all cases. Here are two
> examples of how libvirt does it currently:
>
> VirtIO:
>
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>
> SCSI:
>
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>
> The 'dummy' value there can be absolutely anything you want.
> It is totaly ignored when QEMU sees if=none in 2nd arg.
>   

I'd be all for removing the pci-hotplug.c version of drive_add then. But
I think the IF_SCSI option there is to append a drive to an existing
SCSI bus, no?


Alex
Daniel P. Berrange - Aug. 24, 2010, 10:51 a.m.
On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> >   
> >> The monitor command for hotplugging is in i386 specific code. This is just
> >> plain wrong, as S390 just learned how to do hotplugging too and needs to
> >> get drives for that.
> >>
> >> So let's add a generic copy to generic code that handles drive_add in a
> >> way that doesn't have pci dependencies.
> >>
> >> I'm not fully happy with the patch as is. IMHO there should only be a
> >> single target agnostic drive_hot_add function available. How we could
> >> potentially fit IF_SCSI in there I don't know though.
> >>     
> >
> > I'm not sure that this patch is actually neccessary. Via a undocumented,
> > sick, dirty hack, you can already use the current drive_add command
> > without a PCI address, for both virtio + scsi. In fact not using the
> > PCI address with drive_add is the preferred approach in the new qdev
> > world even on x86
> >   
> 
> It is certainly necessary since the current code is in a big fat #if
> defined(TARGET_I386) block :).

True, true, killing the #ifdef is needed :-)

> > The key is that you should use  if=none for all cases. Here are two
> > examples of how libvirt does it currently:
> >
> > VirtIO:
> >
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >
> > SCSI:
> >
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >
> > The 'dummy' value there can be absolutely anything you want.
> > It is totaly ignored when QEMU sees if=none in 2nd arg.
> >   
> 
> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> I think the IF_SCSI option there is to append a drive to an existing
> SCSI bus, no?

Actually this SCSI example I give above is appending a drive to an existing
bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
approach can cope with all, modulo bugs that appear periodically with code
that mistakenly checks for a particular IF_XXX constant.

If you wanted to also create a new SCSI bus, before creating the drive on
it, you'd need to run three commands in total:

  device_add lsi,id=scsi0,bus=pci.0,addr=0x7
  drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
  device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1

Regards,
Daniel
Alexander Graf - Aug. 24, 2010, 1:40 p.m.
Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
>>>   
>>>       
>>>> The monitor command for hotplugging is in i386 specific code. This is just
>>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>>>> get drives for that.
>>>>
>>>> So let's add a generic copy to generic code that handles drive_add in a
>>>> way that doesn't have pci dependencies.
>>>>
>>>> I'm not fully happy with the patch as is. IMHO there should only be a
>>>> single target agnostic drive_hot_add function available. How we could
>>>> potentially fit IF_SCSI in there I don't know though.
>>>>     
>>>>         
>>> I'm not sure that this patch is actually neccessary. Via a undocumented,
>>> sick, dirty hack, you can already use the current drive_add command
>>> without a PCI address, for both virtio + scsi. In fact not using the
>>> PCI address with drive_add is the preferred approach in the new qdev
>>> world even on x86
>>>   
>>>       
>> It is certainly necessary since the current code is in a big fat #if
>> defined(TARGET_I386) block :).
>>     
>
> True, true, killing the #ifdef is needed :-)
>   

And moving it out of the pci specific file. Yeah :). Basically my
proposal was to take the patches as is and phase out the pci-hotplug.c
version.

>   
>>> The key is that you should use  if=none for all cases. Here are two
>>> examples of how libvirt does it currently:
>>>
>>> VirtIO:
>>>
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>>>
>>> SCSI:
>>>
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>
>>> The 'dummy' value there can be absolutely anything you want.
>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>>>   
>>>       
>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>> I think the IF_SCSI option there is to append a drive to an existing
>> SCSI bus, no?
>>     
>
> Actually this SCSI example I give above is appending a drive to an existing
> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> approach can cope with all, modulo bugs that appear periodically with code
> that mistakenly checks for a particular IF_XXX constant.
>
> If you wanted to also create a new SCSI bus, before creating the drive on
> it, you'd need to run three commands in total:
>
>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>   

Nice - so we can just deprecate if=!none?


Alex
Daniel P. Berrange - Aug. 24, 2010, 1:44 p.m.
On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> >   
> >> Daniel P. Berrange wrote:
> >>     
> >>> On Tue, Aug 24, 2010 at 12:02:30AM +0200, Alexander Graf wrote:
> >>>   
> >>>       
> >>>> The monitor command for hotplugging is in i386 specific code. This is just
> >>>> plain wrong, as S390 just learned how to do hotplugging too and needs to
> >>>> get drives for that.
> >>>>
> >>>> So let's add a generic copy to generic code that handles drive_add in a
> >>>> way that doesn't have pci dependencies.
> >>>>
> >>>> I'm not fully happy with the patch as is. IMHO there should only be a
> >>>> single target agnostic drive_hot_add function available. How we could
> >>>> potentially fit IF_SCSI in there I don't know though.
> >>>>     
> >>>>         
> >>> I'm not sure that this patch is actually neccessary. Via a undocumented,
> >>> sick, dirty hack, you can already use the current drive_add command
> >>> without a PCI address, for both virtio + scsi. In fact not using the
> >>> PCI address with drive_add is the preferred approach in the new qdev
> >>> world even on x86
> >>>   
> >>>       
> >> It is certainly necessary since the current code is in a big fat #if
> >> defined(TARGET_I386) block :).
> >>     
> >
> > True, true, killing the #ifdef is needed :-)
> >   
> 
> And moving it out of the pci specific file. Yeah :). Basically my
> proposal was to take the patches as is and phase out the pci-hotplug.c
> version.
> 
> >   
> >>> The key is that you should use  if=none for all cases. Here are two
> >>> examples of how libvirt does it currently:
> >>>
> >>> VirtIO:
> >>>
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >>>
> >>> SCSI:
> >>>
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>
> >>> The 'dummy' value there can be absolutely anything you want.
> >>> It is totaly ignored when QEMU sees if=none in 2nd arg.
> >>>   
> >>>       
> >> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> >> I think the IF_SCSI option there is to append a drive to an existing
> >> SCSI bus, no?
> >>     
> >
> > Actually this SCSI example I give above is appending a drive to an existing
> > bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> > remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> > approach can cope with all, modulo bugs that appear periodically with code
> > that mistakenly checks for a particular IF_XXX constant.
> >
> > If you wanted to also create a new SCSI bus, before creating the drive on
> > it, you'd need to run three commands in total:
> >
> >   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
> >   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
> >   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >   
> 
> Nice - so we can just deprecate if=!none?

In theory yes, but its not nice to tell users to switch everything over to
use if=none, if we're going to deprecate that too in the next release when
blockdev appears. Might as well just deprecate entire of drive_add/-drive
at once.

Regards,
Daniel
Alexander Graf - Aug. 24, 2010, 1:46 p.m.
Daniel P. Berrange wrote:
> On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>>>       
>>>>> The key is that you should use  if=none for all cases. Here are two
>>>>> examples of how libvirt does it currently:
>>>>>
>>>>> VirtIO:
>>>>>
>>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>>>>>
>>>>> SCSI:
>>>>>
>>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>>>
>>>>> The 'dummy' value there can be absolutely anything you want.
>>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>>>>>   
>>>>>       
>>>>>           
>>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>>>> I think the IF_SCSI option there is to append a drive to an existing
>>>> SCSI bus, no?
>>>>     
>>>>         
>>> Actually this SCSI example I give above is appending a drive to an existing
>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>> approach can cope with all, modulo bugs that appear periodically with code
>>> that mistakenly checks for a particular IF_XXX constant.
>>>
>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>> it, you'd need to run three commands in total:
>>>
>>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>   
>>>       
>> Nice - so we can just deprecate if=!none?
>>     
>
> In theory yes, but its not nice to tell users to switch everything over to
> use if=none, if we're going to deprecate that too in the next release when
> blockdev appears. Might as well just deprecate entire of drive_add/-drive
> at once.
>   

I guess I still fail to see the reason for blockdev when we force
drive_add to if=none...


Alex
Daniel P. Berrange - Aug. 24, 2010, 1:51 p.m.
On Tue, Aug 24, 2010 at 03:46:14PM +0200, Alexander Graf wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
> >   
> >> Daniel P. Berrange wrote:
> >>     
> >>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
> >>>       
> >>>>> The key is that you should use  if=none for all cases. Here are two
> >>>>> examples of how libvirt does it currently:
> >>>>>
> >>>>> VirtIO:
> >>>>>
> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
> >>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
> >>>>>
> >>>>> SCSI:
> >>>>>
> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
> >>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>>>
> >>>>> The 'dummy' value there can be absolutely anything you want.
> >>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
> >>>>>   
> >>>>>       
> >>>>>           
> >>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
> >>>> I think the IF_SCSI option there is to append a drive to an existing
> >>>> SCSI bus, no?
> >>>>     
> >>>>         
> >>> Actually this SCSI example I give above is appending a drive to an existing
> >>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
> >>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
> >>> approach can cope with all, modulo bugs that appear periodically with code
> >>> that mistakenly checks for a particular IF_XXX constant.
> >>>
> >>> If you wanted to also create a new SCSI bus, before creating the drive on
> >>> it, you'd need to run three commands in total:
> >>>
> >>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
> >>>   
> >>>       
> >> Nice - so we can just deprecate if=!none?
> >>     
> >
> > In theory yes, but its not nice to tell users to switch everything over to
> > use if=none, if we're going to deprecate that too in the next release when
> > blockdev appears. Might as well just deprecate entire of drive_add/-drive
> > at once.
> >   
> 
> I guess I still fail to see the reason for blockdev when we force
> drive_add to if=none...

Markus can no doubt explain better than me, but off the top of my head
 
 - 'drive' has guest properties that should be against the device
   not the drive which is for host properties (eg serial=, if=)
 - 'file' is mangled to include protocol/format which means that 
   it can't be unambigously parsed. (eg filenames containing :)

Fixing those, particularly the latter, would breaks back-compat so we
really need a new arg with sensible definition. This is what blockdev
is intended todo (as well as a internal code cleanup)

Regards,
Daniel
Anthony Liguori - Aug. 24, 2010, 6:35 p.m.
On 08/24/2010 08:44 AM, Daniel P. Berrange wrote:
>
>>> Actually this SCSI example I give above is appending a drive to an existing
>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>> approach can cope with all, modulo bugs that appear periodically with code
>>> that mistakenly checks for a particular IF_XXX constant.
>>>
>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>> it, you'd need to run three commands in total:
>>>
>>>    device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>    drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>    device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>
>>>        
>> Nice - so we can just deprecate if=!none?
>>      
> In theory yes, but its not nice to tell users to switch everything over to
> use if=none, if we're going to deprecate that too in the next release when
> blockdev appears. Might as well just deprecate entire of drive_add/-drive
> at once.
>    

I think what Alex is really asking is can we have 'blockdev_add 
var0=val0,var1=val1[,...]' implemented as 'drive_add dummy 
if=none,var0=val0,var1=val1[,...]'.  I don't know the answer to why that 
isn't possible or desirable.

Regards,

Anthony Liguori

> Regards,
> Daniel
>
Alexander Graf - Aug. 24, 2010, 9:53 p.m.
On 24.08.2010, at 20:35, Anthony Liguori wrote:

> On 08/24/2010 08:44 AM, Daniel P. Berrange wrote:
>> 
>>>> Actually this SCSI example I give above is appending a drive to an existing
>>>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>>>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>>>> approach can cope with all, modulo bugs that appear periodically with code
>>>> that mistakenly checks for a particular IF_XXX constant.
>>>> 
>>>> If you wanted to also create a new SCSI bus, before creating the drive on
>>>> it, you'd need to run three commands in total:
>>>> 
>>>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>>>> 
>>>>       
>>> Nice - so we can just deprecate if=!none?
>>>     
>> In theory yes, but its not nice to tell users to switch everything over to
>> use if=none, if we're going to deprecate that too in the next release when
>> blockdev appears. Might as well just deprecate entire of drive_add/-drive
>> at once.
>>   
> 
> I think what Alex is really asking is can we have 'blockdev_add var0=val0,var1=val1[,...]' implemented as 'drive_add dummy if=none,var0=val0,var1=val1[,...]'.  I don't know the answer to why that isn't possible or desirable.

Uh, I was really asking for why we need something not drive_add and instead have yet another iteration of drive parameter. But oh well.

I guess we deferred a bit from the original thread. What's the preferred way to go here? Remove the IF_SCSI case completely or keep two copies of the drive_add function around?


Alex
Markus Armbruster - Aug. 27, 2010, 9:27 a.m.
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Aug 24, 2010 at 03:46:14PM +0200, Alexander Graf wrote:
>> Daniel P. Berrange wrote:
>> > On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
>> >   
>> >> Daniel P. Berrange wrote:
>> >>     
>> >>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>> >>>       
>> >>>>> The key is that you should use  if=none for all cases. Here are two
>> >>>>> examples of how libvirt does it currently:
>> >>>>>
>> >>>>> VirtIO:
>> >>>>>
>> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>> >>>>>   device_add virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>> >>>>>
>> >>>>> SCSI:
>> >>>>>
>> >>>>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>> >>>>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>>>
>> >>>>> The 'dummy' value there can be absolutely anything you want.
>> >>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>> >>>>>   
>> >>>>>       
>> >>>>>           
>> >>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>> >>>> I think the IF_SCSI option there is to append a drive to an existing
>> >>>> SCSI bus, no?
>> >>>>     
>> >>>>         
>> >>> Actually this SCSI example I give above is appending a drive to an existing
>> >>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>> >>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>> >>> approach can cope with all, modulo bugs that appear periodically with code
>> >>> that mistakenly checks for a particular IF_XXX constant.
>> >>>
>> >>> If you wanted to also create a new SCSI bus, before creating the drive on
>> >>> it, you'd need to run three commands in total:
>> >>>
>> >>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>> >>>   drive_add dummy file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>> >>>   device_add scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>   
>> >>>       
>> >> Nice - so we can just deprecate if=!none?
>> >>     
>> >
>> > In theory yes, but its not nice to tell users to switch everything over to
>> > use if=none, if we're going to deprecate that too in the next release when
>> > blockdev appears. Might as well just deprecate entire of drive_add/-drive
>> > at once.
>> >   
>> 
>> I guess I still fail to see the reason for blockdev when we force
>> drive_add to if=none...
>
> Markus can no doubt explain better than me, but off the top of my head
>  
>  - 'drive' has guest properties that should be against the device
>    not the drive which is for host properties (eg serial=, if=)
>  - 'file' is mangled to include protocol/format which means that 
>    it can't be unambigously parsed. (eg filenames containing :)
>
> Fixing those, particularly the latter, would breaks back-compat so we
> really need a new arg with sensible definition. This is what blockdev
> is intended todo (as well as a internal code cleanup)

Yes, -drive and drive_add are a tangled mess.  I decided to start with a
clean slate, because getting it right is difficult enough without the
historical baggage.

Perhaps the end result can be hammered into the drive_add mold somehow,
by adding a few new options and deprecating a few old ones.  Let's
decide when we get there.

The goal is clean separation of host and guest properties.  device_add
is for guest properties, blockdev_add is for host properties.  Just like
netdev_add, only for block instead of network.

We also want sufficient flexibility (or at least extensibility) to be
able to specify any useful host block driver configuration.  And all
that without the syntactic embarrassments that plague drive_add, such as
':' in filenames.
Markus Armbruster - Aug. 27, 2010, 9:53 a.m.
Alexander Graf <agraf@suse.de> writes:

> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
>
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies.
>
> I'm not fully happy with the patch as is. IMHO there should only be a
> single target agnostic drive_hot_add function available. How we could
> potentially fit IF_SCSI in there I don't know though.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Yes, we need a target-agnostic command to add host block devices.  All
we have now is x86's drive_add, which isn't target-agnostic, because its
normal function is to add both host and guest part, and the latter is
entangled with PCI.

Your patch creates a copy of x86's drive_add with the guest bits
stripped from the code, so it's usable for non-PCI targets.  Guest bits
remain in the syntax (first argument, useless baggage with your
command).  Ugly.

I'm working on a clean solution for this problem.  If you must have a
solution right away, and ugly is fine, then your patch should do for
now.

In my opinion, pci_add, pci_del and drive_add all need to go.  They're
insufficiently general, and they fail to separate host and guest parts.
Alexander Graf - Aug. 27, 2010, 9:56 a.m.
On 27.08.2010, at 11:53, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> The monitor command for hotplugging is in i386 specific code. This is just
>> plain wrong, as S390 just learned how to do hotplugging too and needs to
>> get drives for that.
>> 
>> So let's add a generic copy to generic code that handles drive_add in a
>> way that doesn't have pci dependencies.
>> 
>> I'm not fully happy with the patch as is. IMHO there should only be a
>> single target agnostic drive_hot_add function available. How we could
>> potentially fit IF_SCSI in there I don't know though.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Yes, we need a target-agnostic command to add host block devices.  All
> we have now is x86's drive_add, which isn't target-agnostic, because its
> normal function is to add both host and guest part, and the latter is
> entangled with PCI.
> 
> Your patch creates a copy of x86's drive_add with the guest bits
> stripped from the code, so it's usable for non-PCI targets.  Guest bits
> remain in the syntax (first argument, useless baggage with your
> command).  Ugly.
> 
> I'm working on a clean solution for this problem.  If you must have a
> solution right away, and ugly is fine, then your patch should do for
> now.

Yes. IMHO it'd be useful to just broaden the scope of drive_add for now and remove it later in favor of your improved blockdev_add framework. So I guess the code duplication doesn't really hurt either, since this will all go away anyways.

> In my opinion, pci_add, pci_del and drive_add all need to go.  They're
> insufficiently general, and they fail to separate host and guest parts.

Sure :). Please make sure to always keep non-PCI capably systems in mind on your redesign!


Alex

Patch

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index c1a9a56..f311c7f 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -25,6 +25,9 @@ 
 #include "hw.h"
 #include "boards.h"
 #include "net.h"
+#include "qemu-config.h"
+#include "sysemu.h"
+#include "monitor.h"
 
 DriveInfo *add_init_drive(const char *optstr)
 {
@@ -44,3 +47,43 @@  DriveInfo *add_init_drive(const char *optstr)
 
     return dinfo;
 }
+
+#if !defined(TARGET_I386)
+
+/*
+ * This version of drive_hot_add does not know anything about PCI specifics.
+ * It is used as fallback on architectures that don't implement pci-hotplug.
+ */
+void drive_hot_add(Monitor *mon, const QDict *qdict)
+{
+    int type;
+    DriveInfo *dinfo = NULL;
+    const char *opts = qdict_get_str(qdict, "opts");
+
+    dinfo = add_init_drive(opts);
+    if (!dinfo)
+        goto err;
+    if (dinfo->devaddr) {
+        monitor_printf(mon, "Parameter addr not supported\n");
+        goto err;
+    }
+    type = dinfo->type;
+
+    switch (type) {
+    case IF_NONE:
+        monitor_printf(mon, "OK\n");
+        break;
+    default:
+        monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
+        goto err;
+    }
+    return;
+
+err:
+    if (dinfo)
+        drive_uninit(dinfo);
+    return;
+}
+
+#endif /* !defined(TARGET_I386) */
+