Re: [PATCH v1 2/3] qemu_driver: Implement qemuDomainSetVcpuTuneParameters

On Sat, Feb 15, 2025 at 16:35:52 +0800, yong.huang(a)smartx.com wrote: > From: Hyman Huang <yong.huang(a)smartx.com&gt; > > Support hotplug/hotunplug of virtual CPU by wrapping the > existing interface qemuDomainSetVcpu. > > Signed-off-by: Hyman Huang <yong.huang(a)smartx.com&gt; > --- > include/libvirt/libvirt-domain.h | 12 ++++++++++++ > src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index ae1e07b7b6..16e3140e09 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -6612,6 +6612,18 @@ virDomainGraphicsReload(virDomainPtr domain, > unsigned int type, > unsigned int flags); > > +/* Virtual CPU set parameters */ > + > +/** > + * VIR_DOMAIN_VCPU_STATE: > + * > + * The tunable enable/disable individual vcpus described by @vcpumap > + * in the hypervisor* > + * > + * Since: 11.1.0 > + */ > +# define VIR_DOMAIN_VCPU_STATE "state" > + > /** > * virDomainSetVcpuTuneParameters: > * > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 772cb405d6..59d94c6dd6 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -20081,6 +20081,34 @@ qemuDomainGraphicsReload(virDomainPtr domain, > return ret; > } > > +static int > +qemuDomainSetVcpuTuneParameters(virDomainPtr dom, > + const char *vcpumap, > + virTypedParameterPtr params, > + int nparams, > + unsigned int flags) > +{ > + int state; > + int rc; > + > + if (virTypedParamsValidate(params, nparams, > + VIR_DOMAIN_VCPU_STATE, > + VIR_TYPED_PARAM_INT, > + NULL) < 0) > + return -1; > + > + if ((rc = virTypedParamsGetInt(params, nparams, > + VIR_DOMAIN_VCPU_STATE, > + &state)) < 0) > + return -1; > + > + if (rc == 1) { > + return qemuDomainSetVcpu(dom, vcpumap, state, flags); I don't think we want to expose cpu hotplug via another API. Especially this has weird semantics once you try to combine it with other "parameter" setting. E.g. what if you set CPU to disabled, but set some other parameters? It has even ordering implications because enabling CPU needs to happen first. I don't think this should be done. CPU state doesn't even feel like a 'tunable' variable.

NACK > + } > + > + return 0; > +}