On Wed, 3 Apr 2013 08:42:25 +0200
Jens Freimann <jf...@linux.vnet.ibm.com> wrote:
> From: Thang Pham <thang.p...@us.ibm.com>
>
[...]
>
> @@ -31,25 +34,152 @@ static inline S390SCLPDevice *get_event_facility(void)
> static void read_SCP_info(SCCB *sccb)
> {
> ReadInfo *read_info = (ReadInfo *) sccb;
> + int cpu_count = standby_cpus + smp_cpus;
> + int i = 0;
> + int max_vcpus = standby_cpus + smp_cpus;
> int shift = 0;
> -
> +#ifdef CONFIG_KVM
> + max_vcpus = kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS);
> +#endif
> while ((ram_size >> (20 + shift)) > 65535) {
> shift++;
> }
> read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> read_info->rnsize = 1 << shift;
> +
> + /* CPU information */
> + read_info->entries_cpu = cpu_to_be16(standby_cpus + smp_cpus);
see below comment, possibly entries_cpu value might be +off.
> + read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
> + read_info->highest_cpu = cpu_to_be16(max_vcpus);
> +
> + /*
> + * Insert a valid 16-byte entry for each CPU in each the
> + * list (configured & standby)
> + */
> + for (i = 0; i < cpu_count; i++) {
> + read_info->entries[i].address = i;
> + read_info->entries[i].type = 0; /* General purpose CPU */
> + }
> +
> + read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO);
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +}
> +
> +/* Provide information about the CPU */
> +static void sclp_read_cpu_info(SCCB *sccb)
> +{
> + ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> + int cpu_count = standby_cpus + smp_cpus;
in qemu_system_cpu_hot_add() standby_cpus increased but smp_cpus isn't
touched. That might create access out of array bound in below for loop,
and.
> + int i = 0;
> +
> + cpu_info->nr_configured = cpu_to_be16(smp_cpus);
> + cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo,
> entries));
> + cpu_info->nr_standby = cpu_to_be16(standby_cpus);
> +
> + /* The standby offset is 16-byte for each CPU */
> + cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
> + + cpu_info->nr_configured*sizeof(CpuEntry));
> +
> + /*
> + * Insert a valid 16-byte entry for each CPU in each the
> + * list (configured & standby)
> + */
> + for (i = 0; i < cpu_count; i++) {
> + cpu_info->entries[i].address = i;
> + cpu_info->entries[i].type = 0; /* General purpose CPU */
> + }
> +
> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> }
>
> +static void sclp_configure_cpu(SCCB *sccb, uint16_t cpu_addr)
> +{
> + /* Create underlying CPU thread */
> + S390CPU *target_cpu = s390_cpu_addr2state(cpu_addr);
> +
> + if (!target_cpu) {
> + /* Create new vCPU thread and associate it with the CPU address */
> + target_cpu = configure_cpu(cpu_addr);
> + }
> +
> + /*
> + * Bring CPU from standby to configure state. Increment configure CPU
> count
> + * and decrement standby CPU count.
> + */
> + smp_cpus++;
> + if (standby_cpus) {
> + standby_cpus--;
> + }
what if guest calls it several times for the same CPU?
It could increase its VCPU limit given on cmd line.
> +
> + /* CPU hotplug requires SCCB response code */
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void sclp_deconfigure_cpu(SCCB *sccb, uint16_t cpu_addr)
> +{
> + /*
> + * Bring configured CPU to standby state.
> + * Underlying CPU thread should be halted.
> + */
> + S390CPU *target_cpu = s390_cpu_addr2state(cpu_addr);
> + CPUState *target_cpu_state = CPU(target_cpu);
> + CPUS390XState *target_env;
> +
> + if (target_cpu) {
> + target_env = &target_cpu->env;
> +
> + /* Halt CPU */
> + if (target_cpu_state->halted != 1) {
> + fprintf(stderr, "CPU %d must be off-lined first before it can
> be "
> + "de-configured\n", cpu_addr);
> + sccb->h.response_code =
> + cpu_to_be16(SCLP_RC_IMPROPER_RESOURCE_STATE);
> + return;
> + }
> +
> + /* Do not de-configure the last configured CPU */
> + if (smp_cpus == 1) {
> + fprintf(stderr, "At least one CPU must be running. CPU %d
> cannot "
> + "be de-configured\n", cpu_addr);
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_REQUIRED_RESOURCE);
> + return;
> + }
> +
> + standby_cpus++;
> + smp_cpus--;
> +
> + qemu_cpu_kick(ENV_GET_CPU(target_env));
> +
> + /* CPU hotplug done on guest requires SCCB response code*/
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> + } else {
> + /* CPU was not created if target_cpu is NULL */
> + fprintf(stderr, "CPU %d does not exist\n", cpu_addr);
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_RESOURCE_ID);
> + }
> +}
> +
> static void sclp_execute(SCCB *sccb, uint64_t code)
> {
> S390SCLPDevice *sdev = get_event_facility();
>
> - switch (code) {
> + switch (code & SCLP_NO_CMD_PARM) {
> case SCLP_CMDW_READ_SCP_INFO:
> case SCLP_CMDW_READ_SCP_INFO_FORCED:
> read_SCP_info(sccb);
> break;
> + case SCLP_CMDW_READ_CPU_INFO:
> + sclp_read_cpu_info(sccb);
> + break;
> + case SCLP_CMDW_CONFIGURE_CPU:
> + sclp_configure_cpu(sccb,
> + (uint16_t) (code & SCLP_CMDW_CPU_CMD_PARM) >> 8);
> + break;
> + case SCLP_CMDW_DECONFIGURE_CPU:
> + /* Obtain CPU address from code: (uint16_t) (code & 0xff00) >> 8 */
> + sclp_deconfigure_cpu(sccb,
> + (uint16_t) (code & SCLP_CMDW_CPU_CMD_PARM) >> 8);
> + break;
> default:
> sdev->sclp_command_handler(sdev->ef, sccb, code);
> break;
[...]
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 23fe51f..71afe6e 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -28,12 +28,71 @@
> #include "qemu/timer.h"
> #include "hw/hw.h"
> #ifndef CONFIG_USER_ONLY
> +#include "hw/s390x/sclp.h"
> #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> #endif
>
> #define CR0_RESET 0xE0UL
> #define CR14_RESET 0xC2000000UL;
>
> +#ifndef CONFIG_USER_ONLY
> +
> +void qemu_system_cpu_hot_add(int cpu, int state)
> +{
> + int max_vcpus = smp_cpus + standby_cpus;
> + int next_cpu = smp_cpus + standby_cpus;
/me confused
from vl.c
+ if (max_cpus) {
+ standby_cpus = max_cpus - smp_cpus;
+ }
> +#ifdef CONFIG_KVM
> + max_vcpus = kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS);
> +#endif
MIN(max_vcpus, max_cpus) ? maybe machine should be aborted at startup if
kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS) < max_cpus
> +
[...]
> + /*
> + * Find out next CPU address that could be attached.
> + * Only standby CPUs can be added by the monitor and it must be in
> + * sequential order
> + */
> + if (next_cpu >= max_vcpus) {
> + fprintf(stderr, "The maximum number of configurable CPUs has been "
> + "reached\n");
> + return;
> + }
from above vl.c snippet
next_cpu might be == max_cpu ???
> +
> + if (cpu != next_cpu) {
> + fprintf(stderr, "The next standby CPU that can be hotplugged is "
> + "CPU %d\n", next_cpu);
> + return;
> + }
> +
> + /* Configure standby CPU */
> + configure_cpu((uint16_t) cpu);
> +
> + /* Configure is invoked from monitor. Increment standby CPU count. */
> + standby_cpus++;
Is smp_cpus-- missing here intentionally?
> +
> + /* Trigger SCLP interrupt */
> + raise_irq_cpu_hotplug();
> +}
> +#endif
> +
> /* generate CPU information for cpu -? */
> void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index e351005..12bea46 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -316,6 +316,12 @@ S390CPU *cpu_s390x_init(const char *cpu_model);
> void s390x_translate_init(void);
> int cpu_s390x_exec(CPUS390XState *s);
>
> +#ifndef CONFIG_USER_ONLY
> +/* CPU hotplug support on s390 */
> +void qemu_system_cpu_hot_add(int cpu, int state);
> +S390CPU *s390x_cpu_hotplug(int cpu_addr);
> +#endif
> +
> /* you can call this signal handler from your SIGBUS and SIGSEGV
> signal handlers to inform the virtual CPU of exceptions. non zero
> is returned if the signal was handled by the virtual CPU. */
> @@ -373,6 +379,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU
> *cpu, int type, }
> #endif
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> +void s390_cpu_add_state(uint16_t cpu_addr, S390CPU *state);
> void s390_add_running_cpu(S390CPU *cpu);
> unsigned s390_del_running_cpu(S390CPU *cpu);
>
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index b425054..c159c78 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -51,6 +51,8 @@
> #endif
>
> #ifndef CONFIG_USER_ONLY
> +static int last_hotplug_cpu; /* Track which CPU was last hotplugged */
> +
> void s390x_tod_timer(void *opaque)
> {
> S390CPU *cpu = opaque;
> @@ -68,6 +70,47 @@ void s390x_cpu_timer(void *opaque)
> env->pending_int |= INTERRUPT_CPUTIMER;
> cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> }
> +
> +S390CPU *s390x_cpu_hotplug(int cpu_addr)
> +{
> + const char *cpu_model = "host";
> + S390CPU *new_cpu;
> + CPUS390XState *new_env;
> + CPUState *new_cs;
> + S390CPU *conf_cpu;
> + CPUS390XState *conf_env;
> +
> + /* Initialize new CPU */
> + new_cpu = S390_CPU(object_new(TYPE_S390_CPU));
> + new_cs = CPU(new_cpu);
> + new_env = &new_cpu->env;
> + new_env->cpu_model_str = cpu_model;
> + new_env->cpu_num = cpu_addr;
> + new_cs->cpu_index = cpu_addr;
usage of cpu_num and cpu_index looks like a complete mess.
just do:
git grep cpu_num target-s390x hw/s390x
It make sense to return cpu_num from kvm_arch_vcpu_id() and drop usage of
cpu_index altogether.
moreover cpu_index is assigned in cpu_exec_init() and used there by
vmstate_*(), yes CPU marked as unmigrateable but why intentionally make
the current state worse.
> +
> + /*
> + * Find the last CPU hotplugged and chain it to this one
> + */
> + if (!last_hotplug_cpu) {
> + /*
> + * If no CPU was hotplugged before, set it to the last configured
> CPU
> + * and/or standby CPU brought online
> + */
> + last_hotplug_cpu = smp_cpus + standby_cpus - 1;
> + }
> +
> + /* Use the last hotplugged CPU */
> + conf_cpu = s390_cpu_addr2state((uint16_t) last_hotplug_cpu);
since goal is sequential hot-add then all this last_hotplug_cpu and
cpu_num/cpu_index fiddling looks unnecessary.
cpu_num provides this kind of counter and increments with every new CPU
in initfn().
> + conf_env = &conf_cpu->env;
> + conf_env->next_cpu = new_env;
> + new_env->next_cpu = NULL;
cpu_exec_init() does this for you, doesn't it?
why this meddling with next_cpu needed?
> + qemu_init_vcpu(new_env);
Something wrong here, ^^^ is part of realizefn() of CPU class and probably
shouldn't be accessed directly outside of it, ever.
> +
> + /* Update last hotplugged CPU */
> + last_hotplug_cpu = cpu_addr;
> +
> + return new_cpu;
> +}
> #endif
>
> S390CPU *cpu_s390x_init(const char *cpu_model)
> diff --git a/vl.c b/vl.c
> index 52eacca..fa2d45f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -211,6 +211,7 @@ int win2k_install_hack = 0;
> int singlestep = 0;
> int smp_cpus = 1;
> int max_cpus = 0;
> +int standby_cpus = 0;
target specific, pls do it inside target code if possible
> int smp_cores = 1;
> int smp_threads = 1;
> #ifdef CONFIG_VNC
> @@ -1423,6 +1424,11 @@ static void smp_parse(const char *optarg)
> smp_threads = threads > 0 ? threads : 1;
> if (max_cpus == 0)
> max_cpus = smp_cpus;
> +
> + /* Compute standby CPUs */
> + if (max_cpus) {
> + standby_cpus = max_cpus - smp_cpus;
> + }
ditto
> }
>
> /***********************************************************/
