[libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
Farhan Ali
alifm at linux.vnet.ibm.com
Fri May 26 15:00:50 UTC 2017
On 05/25/2017 08:34 PM, John Ferlan wrote:
>
>
> On 05/23/2017 09:27 AM, Farhan Ali wrote:
>> Introduce a new QEMU capability for loadparm and if the capability is
>> supported by QEMU then append the loadparm value to "-machine" string
>> of qemu command line.
>>
>> Signed-off-by: Farhan Ali <alifm at linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> Reviewed-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>> ---
>> src/qemu/qemu_capabilities.c | 2 ++
>> src/qemu/qemu_capabilities.h | 1 +
>> src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 40 insertions(+)
>>
>
> I was wondering why there weren't any changes to the tests/
> qemucapabilitiesdata/*s390*.xml files... Until I realized this is a
> really new feature for qemu... As in post 2.9 and there is no 2.10
> *.replies and *.xml file yet created...
>
> The *.xml files add the <flag name=...> entries for whatever qemu
> versions the machine loadparm=xxx flag can be found in order to define
> the flag to create the .args. But you have .args in v3, which is
> confusing (it's late too).
>
> See commit '6b5c6314' for a recently added capability for kernel-irqchip
> which modified a number of tests/qemucapabilitiesdata/* output .xml files.
>
> There's also "tests/qemucapsprobe /path/to/binary >caps.replies" which
> is what's used to generate those .replies files (those could be added as
> part of this or the xml2xml commit).
Will add the relevant files for s390x qemu 2.10
>
> Sometimes the patch order choice is have patch 1 have all the necessary
> flag stuff done, patch 2 the xml/rng/docs, patch 3 the qemu, and patch 4
> the news.xml.
>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 546dfd7..e3bd445 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -371,6 +371,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>> "kernel-irqchip.split",
>> "intel-iommu.intremap",
>> "intel-iommu.caching-mode",
>> + "loadparm",
>> );
>>
>>
>> @@ -3144,6 +3145,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
>> { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP },
>> { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
>> { "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
>> + { "machine", "loadparm", QEMU_CAPS_LOADPARM },
>> };
>>
>> static int
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index aa99fda..a18c61c 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -409,6 +409,7 @@ typedef enum {
>> QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split */
>> QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
>> QEMU_CAPS_INTEL_IOMMU_CACHING_MODE, /* intel-iommu.caching-mode */
>> + QEMU_CAPS_LOADPARM, /* -machine loadparm */
>>
>> QEMU_CAPS_LAST /* this must always be the last item */
>> } virQEMUCapsFlags;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index aa66e3d..1291b62 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7226,6 +7226,41 @@ qemuAppendKeyWrapMachineParms(virBuffer *buf, virQEMUCapsPtr qemuCaps,
>> return true;
>> }
>>
>
> The newer style is two lines between functions
>
Sure, wasn't aware of this. Thanks
>> +static void
>> +qemuAppendLoadparmMachineParm(virBuffer *buf,
>> + const virDomainDef *def,
>> + virQEMUCapsPtr qemuCaps)
>> +{
>> + size_t i = 0;
>> +
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) ||
>> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>> + return;
>
>
> Typically when the flag doesn't exist, we elicit an error message and
> cause failure. I'm still scratching my head over how this works for test
> when the flags aren't in the XML file, but I'm way too tired to think to
> hard about it.
>
>> +
>> + for (i = 0; i < def->ndisks; i++) {
>> + virDomainDiskDefPtr disk = def->disks[i];
>> +
>> + if (disk->info.bootIndex == 1) {
>> + if (disk->info.loadparm)
>
> I think you could combine the if conditions... Unless there's thoughts
> that other bootIndex's would work... Could end up having arch specific
> stuff (yuck).
>
>> + virBufferAsprintf(buf, ",loadparm=%s", disk->info.loadparm);> +
>> + return;
>> + }
>> + }
>> +
>> + /* Network boot device*/
> ^
> Need a space before end of comment
>
>> + for (i = 0; i < def->nnets; i++) {
>> + virDomainNetDefPtr net = def->nets[i];
>> +
>> + if (net->info.bootIndex == 1) {
>> + if (net->info.loadparm)
>> + virBufferAsprintf(buf, ",loadparm=%s", net->info.loadparm);
>> +
>> + return;
>> + }
>> + }
>> +}
>> +
>
> Two lines between functions
>
>> static int
>> qemuBuildNameCommandLine(virCommandPtr cmd,
>> virQEMUDriverConfigPtr cfg,
>> @@ -7315,6 +7350,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>> virCommandAddArg(cmd, "-machine");
>> virBufferAdd(&buf, def->os.machine, -1);
>>
>> + qemuAppendLoadparmMachineParm(&buf, def, qemuCaps);
>> +
>
> Is this required to be the first parameter to -machine? If not then
> perhaps it'd be better to place it at the end after the irqchip stuff.
>
Sure.
> Also, you could move the CapsGet into here so you can keep the void
> designation for the called function.
>
okay
> Was there ever thought to adding loadparm to the machine XML? What's the
> reasoning to not have it there. If it's only valid for bootindex=1,
> then it's far easier to check if the machine XML has it defined rather
> than perusing the disk/network lists (which could be lengthy) only to
> fine none. If the concern is some other arch allowing a different
> bootindex to have loadparm, then the simple decision there is to have a
> "loadparm_bootindex=#" value that would match the disk bootindex=# value.
>
I am not sure what you mean here? By machine xml do you mean <os> xml?
> Tks -
>
> John
>
>> if (def->virtType == VIR_DOMAIN_VIRT_QEMU)
>> virBufferAddLit(&buf, ",accel=tcg");
>> else if (def->virtType == VIR_DOMAIN_VIRT_KVM)
>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list