[libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string

[John Ferlan]

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).

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

> +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.

Also, you could move the CapsGet into here so you can keep the void
designation for the called function.

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.

Tks -

John

>          if (def->virtType == VIR_DOMAIN_VIRT_QEMU)
>              virBufferAddLit(&buf, ",accel=tcg");
>          else if (def->virtType == VIR_DOMAIN_VIRT_KVM)
>