| Message ID | 20220224060653.74229-1-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | vl: transform QemuOpts device to JSON syntax device | expand |
On Thu, Feb 24, 2022 at 02:06:53PM +0800, Zhenzhong Duan wrote: > While there are mixed use of traditional -device option and JSON > syntax option, QEMU reports conflict, e.x: > > /usr/libexec/qemu-kvm -nodefaults \ > -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ > -device virtio-scsi-pci,id=scsi1,bus=pci.0 Why are you attempting to mix JSON and non-JSON syntax at the same time ? The expectation is that any mgmt app adopting JSON syntax will do so universally and not mix old and new syntax. So in practice the scenario above is not one that QEMU ever intended to have used by apps. Regards, Daniel
On Thu, Feb 24, 2022 at 09:02:02 +0000, Daniel P. Berrangé wrote: > On Thu, Feb 24, 2022 at 02:06:53PM +0800, Zhenzhong Duan wrote: > > While there are mixed use of traditional -device option and JSON > > syntax option, QEMU reports conflict, e.x: > > > > /usr/libexec/qemu-kvm -nodefaults \ > > -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ > > -device virtio-scsi-pci,id=scsi1,bus=pci.0 > > Why are you attempting to mix JSON and non-JSON syntax at the same > time ? The expectation is that any mgmt app adopting JSON syntax > will do so universally and not mix old and new syntax. So in practice > the scenario above is not one that QEMU ever intended to have used > by apps. Based on the previous post they are using some 'qemu:commandline' overrides with the legacy syntax with new libvirt which uses JSON syntax: <qemu:commandline> <qemu:arg value='-netdev'/> <qemu:arg value='user,id=mynet0,hostfwd=tcp::44483-:22'/> <qemu:arg value='-device'/> <qemu:arg value='virtio-net-pci,netdev=mynet0,mac=00:16:3E:68:00:10,romfile='/> </qemu:commandline> I suggested that they should add the required functionality to libvirt instead of trying to hack qemu: https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05068.html
On Thu, Feb 24, 2022 at 10:09:35AM +0100, Peter Krempa wrote: > On Thu, Feb 24, 2022 at 09:02:02 +0000, Daniel P. Berrangé wrote: > > On Thu, Feb 24, 2022 at 02:06:53PM +0800, Zhenzhong Duan wrote: > > > While there are mixed use of traditional -device option and JSON > > > syntax option, QEMU reports conflict, e.x: > > > > > > /usr/libexec/qemu-kvm -nodefaults \ > > > -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ > > > -device virtio-scsi-pci,id=scsi1,bus=pci.0 > > > > Why are you attempting to mix JSON and non-JSON syntax at the same > > time ? The expectation is that any mgmt app adopting JSON syntax > > will do so universally and not mix old and new syntax. So in practice > > the scenario above is not one that QEMU ever intended to have used > > by apps. > > Based on the previous post they are using some 'qemu:commandline' > overrides with the legacy syntax with new libvirt which uses JSON > syntax: > > <qemu:commandline> > <qemu:arg value='-netdev'/> > <qemu:arg value='user,id=mynet0,hostfwd=tcp::44483-:22'/> > <qemu:arg value='-device'/> > <qemu:arg value='virtio-net-pci,netdev=mynet0,mac=00:16:3E:68:00:10,romfile='/> > </qemu:commandline> > > I suggested that they should add the required functionality to libvirt > instead of trying to hack qemu: > > https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05068.html Or the other answer is to switch to use JSON syntax in the above command line passthrough config. Or to specify the PCI address so it doesn't clash with other devices already present. Regards, Daniel
>-----Original Message----- >From: Daniel P. Berrangé <berrange@redhat.com> >Sent: Thursday, February 24, 2022 5:14 PM >To: Peter Krempa <pkrempa@redhat.com> >Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org; kwolf@redhat.com; mst@redhat.com; >lersek@redhat.com; pbonzini@redhat.com; eblake@redhat.com >Subject: Re: [PATCH] vl: transform QemuOpts device to JSON syntax device > >On Thu, Feb 24, 2022 at 10:09:35AM +0100, Peter Krempa wrote: >> On Thu, Feb 24, 2022 at 09:02:02 +0000, Daniel P. Berrangé wrote: >> > On Thu, Feb 24, 2022 at 02:06:53PM +0800, Zhenzhong Duan wrote: >> > > While there are mixed use of traditional -device option and JSON >> > > syntax option, QEMU reports conflict, e.x: >> > > >> > > /usr/libexec/qemu-kvm -nodefaults \ >> > > -device '{"driver":"virtio-scsi- >pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ >> > > -device virtio-scsi-pci,id=scsi1,bus=pci.0 >> > >> > Why are you attempting to mix JSON and non-JSON syntax at the same >> > time ? The expectation is that any mgmt app adopting JSON syntax >> > will do so universally and not mix old and new syntax. So in >> > practice the scenario above is not one that QEMU ever intended to >> > have used by apps. >> >> Based on the previous post they are using some 'qemu:commandline' >> overrides with the legacy syntax with new libvirt which uses JSON >> syntax: >> >> <qemu:commandline> >> <qemu:arg value='-netdev'/> >> <qemu:arg value='user,id=mynet0,hostfwd=tcp::44483-:22'/> >> <qemu:arg value='-device'/> >> <qemu:arg >> value='virtio-net-pci,netdev=mynet0,mac=00:16:3E:68:00:10,romfile='/> >> </qemu:commandline> >> >> I suggested that they should add the required functionality to libvirt >> instead of trying to hack qemu: >> >> https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05068.html > >Or the other answer is to switch to use JSON syntax in the above command >line passthrough config. > >Or to specify the PCI address so it doesn't clash with other devices already >present. Thanks Daniel and Peter for comments. Clear on my side. I'll synchronize your suggestions to my colleague. Regards Zhenzhong
Am 24.02.2022 um 07:06 hat Zhenzhong Duan geschrieben: > While there are mixed use of traditional -device option and JSON > syntax option, QEMU reports conflict, e.x: > > /usr/libexec/qemu-kvm -nodefaults \ > -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ > -device virtio-scsi-pci,id=scsi1,bus=pci.0 > > It breaks with: > > qemu-kvm: -device {"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}: PCI: slot 2 function 0 not available for virtio-scsi-pci, in use by virtio-scsi-pci > > But if we reformat first -device same as the second, so only same kind > of option for all the devices, it succeeds, vice versa. e.x: > > /usr/libexec/qemu-kvm -nodefaults \ > -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=02.0 \ > -device virtio-scsi-pci,id=scsi1,bus=pci.0 > > Succeed! > > Because both kind of options are inserted into their own list and > break the order in QEMU command line during BDF auto assign. Fix it > by transform QemuOpts into JSON syntax and insert in JSON device > list, so the order in QEMU command line kept. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> This patch is incorrect and breaks several cases, which are the reason why the QemuOpts path hasn't been changed yet. For example, after this patch, help doesn't work any more: $ build/qemu-system-x86_64 -device help qemu-system-x86_64: -device help: 'help' is not a valid device model name Any non-string property doesn't work any more in non-JSON syntax: $ $ build/qemu-system-x86_64 -blockdev null-co,node-name=disk -device virtio-blk,drive=disk,physical_block_size=4096 qemu-system-x86_64: -device virtio-blk,drive=disk,physical_block_size=4096: Parameter 'physical_block_size' expects uint64 There may be more cases that are broken with this patch. Kevin
>-----Original Message----- >From: Kevin Wolf <kwolf@redhat.com> >Sent: Thursday, February 24, 2022 7:31 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; eblake@redhat.com; >mst@redhat.com; pkrempa@redhat.com; lersek@redhat.com >Subject: Re: [PATCH] vl: transform QemuOpts device to JSON syntax device > >Am 24.02.2022 um 07:06 hat Zhenzhong Duan geschrieben: >> While there are mixed use of traditional -device option and JSON >> syntax option, QEMU reports conflict, e.x: >> >> /usr/libexec/qemu-kvm -nodefaults \ >> -device '{"driver":"virtio-scsi- >pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ >> -device virtio-scsi-pci,id=scsi1,bus=pci.0 >> >> It breaks with: >> >> qemu-kvm: -device >> {"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0" >> }: PCI: slot 2 function 0 not available for virtio-scsi-pci, in use by >> virtio-scsi-pci >> >> But if we reformat first -device same as the second, so only same kind >> of option for all the devices, it succeeds, vice versa. e.x: >> >> /usr/libexec/qemu-kvm -nodefaults \ >> -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=02.0 \ >> -device virtio-scsi-pci,id=scsi1,bus=pci.0 >> >> Succeed! >> >> Because both kind of options are inserted into their own list and >> break the order in QEMU command line during BDF auto assign. Fix it by >> transform QemuOpts into JSON syntax and insert in JSON device list, so >> the order in QEMU command line kept. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >This patch is incorrect and breaks several cases, which are the reason why the >QemuOpts path hasn't been changed yet. > >For example, after this patch, help doesn't work any more: > >$ build/qemu-system-x86_64 -device help >qemu-system-x86_64: -device help: 'help' is not a valid device model name > >Any non-string property doesn't work any more in non-JSON syntax: > >$ $ build/qemu-system-x86_64 -blockdev null-co,node-name=disk -device >virtio-blk,drive=disk,physical_block_size=4096 >qemu-system-x86_64: -device virtio-blk,drive=disk,physical_block_size=4096: >Parameter 'physical_block_size' expects uint64 > >There may be more cases that are broken with this patch. Ah, yes. Thanks for point out. I should have learned more before writing this patch. Sorry for the noise. And we will try the libvirt maintainer suggested way on this issue. Regards Zhenzhong
diff --git a/softmmu/vl.c b/softmmu/vl.c index 1fe028800fdf..3def40b5405e 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -3394,21 +3394,26 @@ void qemu_init(int argc, char **argv, char **envp) qdict_put_str(machine_opts_dict, "usb", "on"); add_device_config(DEV_USB, optarg); break; - case QEMU_OPTION_device: + case QEMU_OPTION_device: { + QObject *obj; if (optarg[0] == '{') { - QObject *obj = qobject_from_json(optarg, &error_fatal); - DeviceOption *opt = g_new0(DeviceOption, 1); - opt->opts = qobject_to(QDict, obj); - loc_save(&opt->loc); - assert(opt->opts != NULL); - QTAILQ_INSERT_TAIL(&device_opts, opt, next); + obj = qobject_from_json(optarg, &error_fatal); } else { - if (!qemu_opts_parse_noisily(qemu_find_opts("device"), - optarg, true)) { + opts = qemu_opts_parse_noisily(qemu_find_opts("device"), + optarg, true); + if (!opts) { exit(1); } + obj = QOBJECT(qemu_opts_to_qdict(opts, NULL)); + qemu_opts_del(opts); } + DeviceOption *opt = g_new0(DeviceOption, 1); + opt->opts = qobject_to(QDict, obj); + loc_save(&opt->loc); + assert(opt->opts != NULL); + QTAILQ_INSERT_TAIL(&device_opts, opt, next); break; + } case QEMU_OPTION_smp: machine_parse_property_opt(qemu_find_opts("smp-opts"), "smp", optarg);
While there are mixed use of traditional -device option and JSON syntax option, QEMU reports conflict, e.x: /usr/libexec/qemu-kvm -nodefaults \ -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}' \ -device virtio-scsi-pci,id=scsi1,bus=pci.0 It breaks with: qemu-kvm: -device {"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x02.0"}: PCI: slot 2 function 0 not available for virtio-scsi-pci, in use by virtio-scsi-pci But if we reformat first -device same as the second, so only same kind of option for all the devices, it succeeds, vice versa. e.x: /usr/libexec/qemu-kvm -nodefaults \ -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=02.0 \ -device virtio-scsi-pci,id=scsi1,bus=pci.0 Succeed! Because both kind of options are inserted into their own list and break the order in QEMU command line during BDF auto assign. Fix it by transform QemuOpts into JSON syntax and insert in JSON device list, so the order in QEMU command line kept. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- softmmu/vl.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)