Re: [libvirt] [PATCH V2 3/4] qemu: Add domain support for VCPU halted state

[Viktor Mihajlovski]

On 29.09.2016 16:34, John Ferlan wrote:
[...]
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5956,6 +5956,75 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>  return ret;
>>  }
>>  
>> +/**
>> + * qemuDomainGetVcpuHalted:
>> + * @vm: domain object
>> + * @vcpu: cpu id
>> + *
>> + * Returns the vCPU halted state.
>> +  */
>> +bool
>> +qemuDomainGetVcpuHalted(virDomainObjPtr vm,
>> +unsigned int vcpuid)
>> +{
>> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
>> +return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted;
>> +}
>> +
>> +/**
>> + * qemuDomainRefreshVcpuHalted:
>> + * @driver: qemu driver data
>> + * @vm: domain object
>> + * @asyncJob: current asynchronous job type
>> + *
>> + * Updates vCPU halted state in the private data of @vm.
>> + *
>> + * Returns number of detected vCPUs on success, -1 on error and reports
>> + * an appropriate error, -2 if the domain doesn't exist any more.
> 
> Neither of the callers checks -1 or -2, just < 0, so is this really
> necessary?
> 
yes, this is nonsense, IIRC this was needed in the first version of the
series to differentiate the two cases
>> + */
>> +int
>> +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>> +virDomainObjPtr vm,
>> +int asyncJob)
>> +{
>> +virDomainVcpuDefPtr vcpu;
>> +qemuMonitorCPUInfoPtr info = NULL;
>> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
>> +size_t i;
>> +bool hotplug;
>> +int rc;
>> +int ret = -1;
>> +
>> +/* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */
>> +if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
>> +return 0;
> 
> Since the "default" is halted = true could we run into a situation where
> "halted" (or "running (inactive)") is always set for TCG...
> 
good point, "halted" as the new kid in town should actually be false by
default, as you have observed the issue is introduced in patch 2/4
(monitors).
[...]

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 3/4] qemu: Add domain support for VCPU halted state

[John Ferlan]

On 09/20/2016 04:11 AM, Viktor Mihajlovski wrote:
> Adding a field to the domain's private vcpu object to hold the halted
> state information.
> Adding two functions in support of the halted state:
> - qemuDomainGetVcpuHalted: retrieve the halted state from a
>   private vcpu object
> - qemuDomainRefreshVcpuHalted: obtain the per-vcpu halted states
>   via qemu monitor and store the results in the private vcpu objects
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Hao QingFeng 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_domain.c | 69 
> ++
>  src/qemu/qemu_domain.h |  5 
>  2 files changed, 74 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3f16dbe..3fb9b4f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5956,6 +5956,75 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>  return ret;
>  }
>  
> +/**
> + * qemuDomainGetVcpuHalted:
> + * @vm: domain object
> + * @vcpu: cpu id
> + *
> + * Returns the vCPU halted state.
> +  */
> +bool
> +qemuDomainGetVcpuHalted(virDomainObjPtr vm,
> +unsigned int vcpuid)
> +{
> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
> +return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted;
> +}
> +
> +/**
> + * qemuDomainRefreshVcpuHalted:
> + * @driver: qemu driver data
> + * @vm: domain object
> + * @asyncJob: current asynchronous job type
> + *
> + * Updates vCPU halted state in the private data of @vm.
> + *
> + * Returns number of detected vCPUs on success, -1 on error and reports
> + * an appropriate error, -2 if the domain doesn't exist any more.

Neither of the callers checks -1 or -2, just < 0, so is this really
necessary?

> + */
> +int
> +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +int asyncJob)
> +{
> +virDomainVcpuDefPtr vcpu;
> +qemuMonitorCPUInfoPtr info = NULL;
> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> +size_t i;
> +bool hotplug;
> +int rc;
> +int ret = -1;
> +
> +/* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */
> +if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
> +return 0;

Since the "default" is halted = true could we run into a situation where
"halted" (or "running (inactive)") is always set for TCG...

> +
> +hotplug = qemuDomainSupportsNewVcpuHotplug(vm);
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +return -1;
> +
> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus, 
> hotplug);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +ret = -2;

I see no need to ret = -2;

> +goto cleanup;
> +}
> +
> +if (rc < 0)
> +goto cleanup;
> +
> +for (i = 0; i < maxvcpus; i++) {
> +vcpu = virDomainDefGetVcpu(vm->def, i);
> +QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = info[i].halted;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +qemuMonitorCPUInfoFree(info, maxvcpus);
> +return ret;
> +}
>  
>  bool
>  qemuDomainSupportsNicdev(virDomainDefPtr def,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index a1404d0..03e58c5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -317,6 +317,7 @@ struct _qemuDomainVcpuPrivate {
>  pid_t tid; /* vcpu thread id */
>  int enable_id; /* order in which the vcpus were enabled in qemu */
>  char *alias;
> +bool halted; /* vcpu halted state */

Another less than necessary comment ;-)

John

>  
>  /* information for hotpluggable cpus */
>  char *type;
> @@ -662,6 +663,10 @@ int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>int asyncJob,
>bool state);
> +bool qemuDomainGetVcpuHalted(virDomainObjPtr vm, unsigned int vcpu);
> +int qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +int asyncJob);
>  
>  bool qemuDomainSupportsNicdev(virDomainDefPtr def,
>virDomainNetDefPtr net);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list