| Message ID | 1282600951-30803-5-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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) */ > + >
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
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
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 >>
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
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
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
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
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
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
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
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
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 >
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
"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.
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.
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
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) */ +
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(-)