Re: [PATCH RFC 1/2] qemu: use "-accel" option to specify accelerator instead of "-machine"

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Monday in 2021, Paolo Bonzini wrote:
> On 09/01/21 19:58, huangy81@xxxxxxxxxxxxxxx wrote:
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 6f970a3..9a64473 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -6711,40 +6711,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> >      virCommandAddArg(cmd, "-machine");
> >      virBufferAdd(&buf, def->os.machine, -1);
> > -    switch ((virDomainVirtType)def->virtType) {
> > -    case VIR_DOMAIN_VIRT_QEMU:
> > -        virBufferAddLit(&buf, ",accel=tcg");
> > -        break;
> > -
> > -    case VIR_DOMAIN_VIRT_KVM:
> > -        virBufferAddLit(&buf, ",accel=kvm");
> > -        break;
> > -
> > -    case VIR_DOMAIN_VIRT_KQEMU:
> > -    case VIR_DOMAIN_VIRT_XEN:
> > -    case VIR_DOMAIN_VIRT_LXC:
> > -    case VIR_DOMAIN_VIRT_UML:
> > -    case VIR_DOMAIN_VIRT_OPENVZ:
> > -    case VIR_DOMAIN_VIRT_TEST:
> > -    case VIR_DOMAIN_VIRT_VMWARE:
> > -    case VIR_DOMAIN_VIRT_HYPERV:
> > -    case VIR_DOMAIN_VIRT_VBOX:
> > -    case VIR_DOMAIN_VIRT_PHYP:
> > -    case VIR_DOMAIN_VIRT_PARALLELS:
> > -    case VIR_DOMAIN_VIRT_BHYVE:
> > -    case VIR_DOMAIN_VIRT_VZ:
> > -    case VIR_DOMAIN_VIRT_NONE:
> > -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -                       _("the QEMU binary does not support %s"),
> > -                       virDomainVirtTypeToString(def->virtType));
> > -        return -1;
> > -
> > -    case VIR_DOMAIN_VIRT_LAST:
> > -    default:
> > -        virReportEnumRangeError(virDomainVirtType, def->virtType);
> > -        return -1;
> > -    }
> > -
> >      /* To avoid the collision of creating USB controllers when calling
> >       * machine->init in QEMU, it needs to set usb=off
> >       */
> > @@ -6945,6 +6911,52 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> >      return 0;
> >  }
> > +static int
> > +qemuBuildAccelCommandLine(virCommandPtr cmd,
> > +                          const virDomainDef *def)
> > +{

Is this a patch sent to libvir-list that I'm not seeing yet because of
moderation?

Either way, please split the qemuBuildAccelCommandLine function
separation from the actual functional changes to make it easier to see
what's going on.

> > +    /* the '-machine' options for accelerator are legacy,
> > +     * using the '-accel' options by default */
> > +    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> > +    virCommandAddArg(cmd, "-accel");
> > +
> > +    switch ((virDomainVirtType)def->virtType) {
> > +    case VIR_DOMAIN_VIRT_QEMU:
> > +        virBufferAddLit(&buf, "tcg");
> > +        break;
> > +
> > +    case VIR_DOMAIN_VIRT_KVM:
> > +        virBufferAddLit(&buf, "kvm");
> > +        break;
> > +
> > +    case VIR_DOMAIN_VIRT_KQEMU:
> > +    case VIR_DOMAIN_VIRT_XEN:
> > +    case VIR_DOMAIN_VIRT_LXC:
> > +    case VIR_DOMAIN_VIRT_UML:
> > +    case VIR_DOMAIN_VIRT_OPENVZ:
> > +    case VIR_DOMAIN_VIRT_TEST:
> > +    case VIR_DOMAIN_VIRT_VMWARE:
> > +    case VIR_DOMAIN_VIRT_HYPERV:
> > +    case VIR_DOMAIN_VIRT_VBOX:
> > +    case VIR_DOMAIN_VIRT_PHYP:
> > +    case VIR_DOMAIN_VIRT_PARALLELS:
> > +    case VIR_DOMAIN_VIRT_BHYVE:
> > +    case VIR_DOMAIN_VIRT_VZ:
> > +    case VIR_DOMAIN_VIRT_NONE:
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("the QEMU binary does not support %s"),
> > +                       virDomainVirtTypeToString(def->virtType));
> > +        return -1;
> > +
> > +    case VIR_DOMAIN_VIRT_LAST:
> > +    default:
> > +        virReportEnumRangeError(virDomainVirtType, def->virtType);
> > +        return -1;
> > +    }
> > +
> > +    virCommandAddArgBuffer(cmd, &buf);
> > +    return 0;
> > +}
> >  static void
> >  qemuBuildTSEGCommandLine(virCommandPtr cmd,
> > @@ -9840,6 +9852,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> >      if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps, priv) < 0)
> >          return NULL;
> > +    if (qemuBuildAccelCommandLine(cmd, def) < 0)
> > +        return NULL;
> > +
> >      qemuBuildTSEGCommandLine(cmd, def);
>
> This unfortunately cannot be done unconditionally.  You need to probe 
> for the availability of -accel, using something like

What are we probing for?

The minimal QEMU version supported by libvirt is 1.5.3 and I'm seeing
some test cases using -machine pc-i440fx-1.5,accel=tcg in our test
suite.

And I don't see any explicit use of -accel in this patch.

Jano

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 4d132defbd..23efaccbad 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -609,6 +609,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>               "ncr53c90",
>               "dc390",
>               "am53c974",
> +              "accel",
>     );
>
>
> @@ -3220,6 +3221,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>     { "fsdev", "multidevs", QEMU_CAPS_FSDEV_MULTIDEVS },
>     { "fw_cfg", "file", QEMU_CAPS_FW_CFG },
>     { "fsdev", "fmode", QEMU_CAPS_FSDEV_CREATEMODE }, /* Could have 
> also checked fsdev->dmode */
> +    { "accel", "accel", QEMU_CAPS_ACCEL },
> };
>
> static int
>
> Then you can choose between the two code paths using 
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL).
>
> Thanks,
>
> Paolo

Attachment: signature.asc
Description: PGP signature