[PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Anthony PERARD]

Hi all,

This series ports few patches from qemu-xen-traditionnal to qemu-xen
(upstream). There is as little modification as possible on those patches so
they *can not* be upstream as is. So this is only for 4.3. A proper series will
be done for 4.4.

There make possible to have CPU hotplug on qemu-xen.

A patch for libxl will follow, which make use of the old command line
-vcpu_avail with qemu-xen. This command line will probably not be upstream.

BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
initialisation, SeaBIOS will count the number of CPU running, and the number is
always equal to maxvcpu when SeaBIOS expect less. This happen when we have
something like:
vcpus = 2
maxvcpus = 8
in the VM config file. Linux is fine with this and will use only $vcpus.  So
the probleme is: an infinit loop in SeaBIOS.

Regards,

Ian Jackson (4):
  HVM vcpu add/remove: qemu logic for vcpu add/revmoe
  Fix vcpu hotplug bug: get correct vcpu_avail bitmap
  Update vcpu hotplug logic
  Implement 'xm vcpu-set' command for HVM guest

 hmp-commands.hx |  9 ++++++
 hw/acpi_piix4.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 monitor.c       | 18 ++++++++++++
 qemu-options.hx |  3 ++
 sysemu.h        |  4 +++
 vl.c            | 55 ++++++++++++++++++++++++++++++++++++
 xen-all.c       | 65 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 241 insertions(+), 1 deletion(-)

-- 
Anthony PERARD

[PATCH 1/4] HVM vcpu add/remove: qemu logic for vcpu add/revmoe

[Anthony PERARD]

From: Ian Jackson <ian.jackson@eu.citrix.com>

-- at qemu side, get vcpu_avail which used for original cpu avail map;
-- setup gpe ioread/iowrite at qmeu;
-- setup vcpu add/remove user interface through monitor;
-- setup SCI logic;

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

[ PATCH 4/4 ] HVM vcpu add/remove: qemu logic for vcpu add/revmoe

Port from qemu-xen-traditionnal to qemu-xen.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hmp-commands.hx |  9 +++++++
 hw/acpi_piix4.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 monitor.c       | 18 ++++++++++++++
 qemu-options.hx |  3 +++
 sysemu.h        |  3 +++
 vl.c            |  6 +++++
 6 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..e92f173 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1581,3 +1581,12 @@ ETEXI
 STEXI
 @end table
 ETEXI
+
+HXCOMM TODO doc
+    {
+        .name       = "cpu_set",
+        .args_type  = "cpu_index:i,status:s",
+        .params     = "cpu [online|offline]",
+        .help       = "change cpu state",
+        .mhandler.cmd = do_cpu_set_nr,
+    },
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 519269a..c73dc7c 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -45,8 +45,11 @@
 #define PCI_DOWN_BASE 0xae04
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
+/* ioport to monitor cpu add/remove status */
+#define PROC_BASE 0xaf00
 
 #define PIIX4_PCI_HOTPLUG_STATUS 2
+#define PIIX4_CPU_HOTPLUG_STATUS 4
 
 struct pci_status {
     uint32_t up; /* deprecated, maintained for migration compatibility */
@@ -77,6 +80,9 @@ typedef struct PIIX4PMState {
     uint8_t disable_s3;
     uint8_t disable_s4;
     uint8_t s4_val;
+
+    /* CPU bitmap */
+    uint8_t cpus_sts[32];
 } PIIX4PMState;
 
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
@@ -95,7 +101,7 @@ static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
         (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
-          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+          & (PIIX4_PCI_HOTPLUG_STATUS|PIIX4_CPU_HOTPLUG_STATUS)) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */
@@ -602,11 +608,48 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr)
     return s->pci0_hotplug_enable;
 }
 
+static uint32_t gpe_cpus_readb(void *opaque, uint32_t addr)
+{
+    uint32_t val = 0;
+    PIIX4PMState *g = opaque;
+
+    switch (addr) {
+        case PROC_BASE ... PROC_BASE+31:
+            val = g->cpus_sts[addr - PROC_BASE];
+        default:
+            break;
+    }
+
+    return val;
+}
+
+static void gpe_cpus_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    switch (addr) {
+        case PROC_BASE ... PROC_BASE + 31:
+            /* don't allow to change cpus_sts from inside a guest */
+            break;
+        default:
+            break;
+    }
+}
+
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
+extern uint64_t vcpu_avail;
+static PIIX4PMState *acpi_state;
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
+    int i = 0, cpus = max_cpus;
+    char *vcpumap = (char *)&vcpu_avail;
+
+    while (cpus > 0) {
+        s->cpus_sts[i] = vcpumap[i];
+        i++;
+        cpus -= 8;
+    }
+    acpi_state = s;
 
     register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
     register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
@@ -620,6 +663,9 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 
     register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
 
+    register_ioport_read(PROC_BASE, 32, 1,  gpe_cpus_readb, s);
+    register_ioport_write(PROC_BASE, 32, 1, gpe_cpus_writeb, s);
+
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
 }
 
@@ -660,3 +706,30 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
 
     return 0;
 }
+
+static void enable_processor(PIIX4PMState *g, int cpu)
+{
+    g->ar.gpe.sts[0] |= 4;
+    g->cpus_sts[cpu/8] |= (1 << (cpu%8));
+}
+
+static void disable_processor(PIIX4PMState *g, int cpu)
+{
+    g->ar.gpe.sts[0] |= 4;
+    g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
+}
+
+void qemu_cpu_add_remove(int cpu, int state)
+{
+    if ((cpu <=0) || (cpu >= max_cpus)) {
+        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
+        return;
+    }
+
+    if (state)
+        enable_processor(acpi_state, cpu);
+    else
+        disable_processor(acpi_state, cpu);
+
+    pm_update_sci(acpi_state);
+}
diff --git a/monitor.c b/monitor.c
index c0e32d6..564373c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2421,6 +2421,24 @@ int monitor_handle_fd_param(Monitor *mon, const char *fdname)
     return fd;
 }
 
+static void do_cpu_set_nr(Monitor *mon, const QDict *qdict)
+{
+    int cpu_index = qdict_get_int(qdict, "cpu_index");
+    const char *status = qdict_get_str(qdict, "status");
+    int state;
+
+    if (!strcmp(status, "online")) {
+        state = 1;
+    } else if (!strcmp(status, "offline")) {
+        state = 0;
+    } else {
+        monitor_printf(mon, "invalid status: %s\n", status);
+        return;
+    }
+
+    qemu_cpu_add_remove(cpu_index, state);
+}
+
 /* mon_cmds and info_cmds would be sorted at runtime */
 static mon_cmd_t mon_cmds[] = {
 #include "hmp-commands.h"
diff --git a/qemu-options.hx b/qemu-options.hx
index de43b1b..0ba668f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -94,6 +94,9 @@ given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
 specifies the maximum number of hotpluggable CPUs.
 ETEXI
 
+DEF("vcpu_avail", HAS_ARG, QEMU_OPTION_vcpu_avail,
+    "-vcpu_avail bitmap\n", QEMU_ARCH_ALL)
+
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
     "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
 STEXI
diff --git a/sysemu.h b/sysemu.h
index f5ac664..ed7b264 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -149,6 +149,9 @@ int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
                       DriveInfo *dinfo, int type);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
 
+/* cpu hotplug */
+void qemu_cpu_add_remove(int cpu, int state);
+
 /* generic hotplug */
 void drive_hot_add(Monitor *mon, const QDict *qdict);
 
diff --git a/vl.c b/vl.c
index a3ab384..27ae6c1 100644
--- a/vl.c
+++ b/vl.c
@@ -207,6 +207,7 @@ int win2k_install_hack = 0;
 int singlestep = 0;
 int smp_cpus = 1;
 int max_cpus = 0;
+uint64_t vcpu_avail = 1;
 int smp_cores = 1;
 int smp_threads = 1;
 #ifdef CONFIG_VNC
@@ -3302,6 +3303,11 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_vcpu_avail:
+                vcpu_avail = atol(optarg);
+                fprintf(stderr, "qemu: the avail cpu bitmap is %lx\n",
+                        vcpu_avail);
+                break;
 	    case QEMU_OPTION_vnc:
 #ifdef CONFIG_VNC
                 display_remote++;
-- 
Anthony PERARD

[PATCH 2/4] Fix vcpu hotplug bug: get correct vcpu_avail bitmap

[Anthony PERARD]

From: Ian Jackson <ian.jackson@eu.citrix.com>

Currently qemu has a bug: When maxvcpus > 64, qemu will get wrong
vcpu bitmap (s->cpus_sts[i]) since it only get bitmap from a long variable.

This patch, cooperate with another xend python patch, is to fix this bug.
This patch get hex string from xend, transfer it to correct vcpu_avail bitmap
which saved at an uint32_t array.

Signed-off-By: Liu, Jinsong <jinsong.liu@intel.com>
(This is [PATCH 2/2], the other half is in xen-unstable.hg)

Port from qemu-xen-traditionnal to qemu-xen.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/acpi_piix4.c |  3 +--
 sysemu.h        |  1 +
 vl.c            | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index c73dc7c..bc7b454 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -637,12 +637,11 @@ static void gpe_cpus_writeb(void *opaque, uint32_t addr, uint32_t val)
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
-extern uint64_t vcpu_avail;
 static PIIX4PMState *acpi_state;
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
     int i = 0, cpus = max_cpus;
-    char *vcpumap = (char *)&vcpu_avail;
+    char *vcpumap = (char *)vcpu_avail;
 
     while (cpus > 0) {
         s->cpus_sts[i] = vcpumap[i];
diff --git a/sysemu.h b/sysemu.h
index ed7b264..5789158 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -113,6 +113,7 @@ extern int alt_grab;
 extern int ctrl_grab;
 extern int smp_cpus;
 extern int max_cpus;
+extern uint32_t vcpu_avail[];
 extern int cursor_hide;
 extern int graphic_rotate;
 extern int no_quit;
diff --git a/vl.c b/vl.c
index 27ae6c1..2ff1093 100644
--- a/vl.c
+++ b/vl.c
@@ -170,6 +170,8 @@ int main(int argc, char **argv)
 #include "ui/qemu-spice.h"
 #include "qapi/string-input-visitor.h"
 
+#include <xen/hvm/hvm_info_table.h>
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -207,7 +209,9 @@ int win2k_install_hack = 0;
 int singlestep = 0;
 int smp_cpus = 1;
 int max_cpus = 0;
-uint64_t vcpu_avail = 1;
+/* use 32b array to record whatever vcpu number bitmap */
+/* do not use 64b array to avoid underflow/overflow when strtol */
+uint32_t vcpu_avail[(HVM_MAX_VCPUS + 31)/32] = {0};
 int smp_cores = 1;
 int smp_threads = 1;
 #ifdef CONFIG_VNC
@@ -2525,6 +2529,53 @@ static int object_create(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+#define STEP   8   /* 8 characters fill uint32_t bitmap */
+#define SPACE  8   /* space for non-hex characters in vcpu str */
+#define MAX_VCPU_STR_LEN    ((HVM_MAX_VCPUS + 3)/4 + SPACE)
+static int hex_legal(char a)
+{
+    return  ((a >= '0' && a <= '9') ||
+             (a >= 'a' && a <= 'f') ||
+             (a >= 'A' && a <= 'F'));
+}
+
+static void vcpu_hex_str_to_bitmap(const char *optarg)
+{
+    char str[MAX_VCPU_STR_LEN + 1] = {'\0'};
+    char *pstr;
+    int length;
+    int step = STEP;
+    int i,j = 0;
+
+    length = strlen(optarg);
+    if(length > MAX_VCPU_STR_LEN)
+        exit(EXIT_FAILURE);
+    strncpy(str, optarg, MAX_VCPU_STR_LEN);
+
+    pstr = ((str[1] == 'x') || (str[1] == 'X')) ?
+             str + 2 : str;
+    length = strlen(pstr);
+
+    for(i = 0; i < length; i++)
+        if(hex_legal(pstr[i]))
+            str[j++] = pstr[i];
+    str[j] = '\0';
+    length = strlen(str);
+
+    i = 0;
+    while(length > 0) {
+        char vcpustr[STEP + SPACE] = {'\0'};
+        int start = ((length - step) > 0) ? length - step : 0;
+        int size  = ((length - step) > 0) ? step : length;
+        memcpy(vcpustr, str + start, size);
+        errno = 0;
+        vcpu_avail[i++] = strtol(vcpustr, NULL, 16);
+        if(errno)
+            exit(EXIT_FAILURE);
+        length -= step;
+    }
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -3304,9 +3355,7 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_vcpu_avail:
-                vcpu_avail = atol(optarg);
-                fprintf(stderr, "qemu: the avail cpu bitmap is %lx\n",
-                        vcpu_avail);
+                vcpu_hex_str_to_bitmap(optarg);
                 break;
 	    case QEMU_OPTION_vnc:
 #ifdef CONFIG_VNC
-- 
Anthony PERARD

[PATCH 3/4] Update vcpu hotplug logic

[Anthony PERARD]

From: Ian Jackson <ian.jackson@eu.citrix.com>

Add vcpu online/offline check to avoid redundant SCI interrupt.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

Port from qemu-xen-traditionnal to qemu-xen.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/acpi_piix4.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index bc7b454..49c38d3 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -706,16 +706,24 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
     return 0;
 }
 
-static void enable_processor(PIIX4PMState *g, int cpu)
+static int enable_processor(PIIX4PMState *g, int cpu)
 {
+    if (g->cpus_sts[cpu/8] & (1 << (cpu%8)))
+        return 0;
+
     g->ar.gpe.sts[0] |= 4;
     g->cpus_sts[cpu/8] |= (1 << (cpu%8));
+    return 1;
 }
 
-static void disable_processor(PIIX4PMState *g, int cpu)
+static int disable_processor(PIIX4PMState *g, int cpu)
 {
+    if (!(g->cpus_sts[cpu/8] & (1 << (cpu%8))))
+        return 0;
+
     g->ar.gpe.sts[0] |= 4;
     g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
+    return 1;
 }
 
 void qemu_cpu_add_remove(int cpu, int state)
@@ -725,10 +733,15 @@ void qemu_cpu_add_remove(int cpu, int state)
         return;
     }
 
-    if (state)
-        enable_processor(acpi_state, cpu);
-    else
-        disable_processor(acpi_state, cpu);
+    if (state) {
+        if (!enable_processor(acpi_state, cpu)) {
+            return;
+        }
+    } else {
+        if (!disable_processor(acpi_state, cpu)) {
+            return;
+        }
+    }
 
     pm_update_sci(acpi_state);
 }
-- 
Anthony PERARD

[PATCH 4/4] Implement 'xm vcpu-set' command for HVM guest

[Anthony PERARD]

From: Ian Jackson <ian.jackson@eu.citrix.com>

Currently Xen has 'xm vcpu-set' command for PV domain, but not
available for HVM domain.  This patch is use to enable 'xm vcpu-set'
command for HVM domain. It setup vcpu watch at xenstore, and at qemu
side, handle vcpu online/offline accordingly.  With this patch, 'xm
vcpu-set' command works for both PV and HVM guest with same format.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

Port from qemu-xen-traditionnal to qemu-xen:
  qemu-xen does not support anymore command through xenstore, so this
  path include an initialisation of a xenstore loop.
  An upstream of this patch would need to go QMP.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/acpi_piix4.c |  5 +++--
 xen-all.c       | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 49c38d3..4c01aa2 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -728,8 +728,8 @@ static int disable_processor(PIIX4PMState *g, int cpu)
 
 void qemu_cpu_add_remove(int cpu, int state)
 {
-    if ((cpu <=0) || (cpu >= max_cpus)) {
-        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
+    if ((cpu < 0) || (cpu >= max_cpus)) {
+        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", max_cpus - 1);
         return;
     }
 
@@ -742,6 +742,7 @@ void qemu_cpu_add_remove(int cpu, int state)
             return;
         }
     }
+    fprintf(stderr, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);
 
     pm_update_sci(acpi_state);
 }
diff --git a/xen-all.c b/xen-all.c
index daf43b9..04b88a6 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -99,6 +99,8 @@ typedef struct XenIOState {
     Notifier suspend;
 } XenIOState;
 
+static void xen_xenstore_watch_vcpu_set(XenIOState *state);
+
 /* Xen specific function for piix pci */
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
@@ -1170,6 +1172,7 @@ int xen_hvm_init(void)
     xen_be_register("vkbd", &xen_kbdmouse_ops);
     xen_be_register("qdisk", &xen_blkdev_ops);
     xen_read_physmap(state);
+    xen_xenstore_watch_vcpu_set(state);
 
     return 0;
 }
@@ -1234,3 +1237,65 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
         }
     }
 }
+
+/* Xenstore watch init for vcpu-set */
+
+static void xenstore_process_vcpu_set_event(char **vec, struct xs_handle *xsh)
+{
+    char *act = NULL;
+    char *vcpustr, *node = vec[XS_WATCH_PATH];
+    unsigned int vcpu, len;
+
+    vcpustr = strstr(node, "cpu/");
+    if (!vcpustr) {
+        fprintf(stderr, "vcpu-set: watch node error.\n");
+        return;
+    }
+    sscanf(vcpustr, "cpu/%u", &vcpu);
+
+    act = xs_read(xsh, XBT_NULL, node, &len);
+    if (!act) {
+        fprintf(stderr, "vcpu-set: no command yet.\n");
+        return;
+    }
+
+    if (!strncmp(act, "online", len))
+        qemu_cpu_add_remove(vcpu, 1);
+    else if (!strncmp(act, "offline", len))
+        qemu_cpu_add_remove(vcpu, 0);
+    else
+        fprintf(stderr, "vcpu-set: command error.\n");
+
+    free(act);
+    return;
+}
+
+static void xenstore_process_event(void *opaque)
+{
+    char **vec;
+    unsigned int num;
+    struct xs_handle *xsh = opaque;
+
+    vec = xs_read_watch(xsh, &num);
+    if (!vec)
+        return;
+
+    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
+        xenstore_process_vcpu_set_event(vec, xsh);
+        goto out;
+    }
+
+ out:
+    free(vec);
+}
+
+static void xen_xenstore_watch_vcpu_set(XenIOState *state)
+{
+    char path[40];
+    /* Set a watch for vcpu-set */
+    snprintf(path, sizeof(path), "/local/domain/%d/cpu", xen_domid);
+    xs_watch(state->xenstore, path, "vcpu-set");
+
+    qemu_set_fd_handler(xs_fileno(state->xenstore), xenstore_process_event,
+                        NULL, state->xenstore);
+}
-- 
Anthony PERARD

[PATCH] libxl: Use -vcpu_avail with qemu-xen.

[Anthony PERARD]

This require the series CPU hotplug for qemu-xen.

Note: this patch is valid only for 4.3 as the -vcpu_avail will
probably not be upstream to Qemu.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 1e3a9f4..2db2372 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -530,11 +530,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-smp");
             if (b_info->avail_vcpus.size) {
                 int nr_set_cpus = 0;
+                char *s;
                 nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus);
 
                 flexarray_append(dm_args, libxl__sprintf(gc, "%d,maxcpus=%d",
                                                          nr_set_cpus,
                                                          b_info->max_vcpus));
+                
+                s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus);
+                flexarray_vappend(dm_args, "-vcpu_avail",
+                                  libxl__sprintf(gc, "%s", s), NULL);
+                free(s);
             } else
                 flexarray_append(dm_args, libxl__sprintf(gc, "%d",
                                                          b_info->max_vcpus));
-- 
Anthony PERARD

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Anthony PERARD]

On 31/05/13 17:33, Anthony PERARD wrote:
> Hi all,
> 
> This series ports few patches from qemu-xen-traditionnal to qemu-xen
> (upstream). There is as little modification as possible on those patches so
> they *can not* be upstream as is. So this is only for 4.3. A proper series will
> be done for 4.4.
> 
> There make possible to have CPU hotplug on qemu-xen.
> 
> A patch for libxl will follow, which make use of the old command line
> -vcpu_avail with qemu-xen. This command line will probably not be upstream.
> 
> BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
> initialisation, SeaBIOS will count the number of CPU running, and the number is
> always equal to maxvcpu when SeaBIOS expect less. This happen when we have
> something like:
> vcpus = 2
> maxvcpus = 8
> in the VM config file. Linux is fine with this and will use only $vcpus.  So
> the probleme is: an infinit loop in SeaBIOS.

To let you know, I just realize that QEMU 1.5 (we are using 1.3, I
think) already have everything needed for CPU hotplug. It will just be a
matter of plumbing libxl to use the cpu-add QMP command. But cpu-remove
QMP command seams to be missing from QEMU.

Regards,

-- 
Anthony PERARD

Re: [PATCH 4/4] Implement 'xm vcpu-set' command for HVM guest

[Konrad Rzeszutek Wilk]

On Fri, May 31, 2013 at 05:33:13PM +0100, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Currently Xen has 'xm vcpu-set' command for PV domain, but not
> available for HVM domain.  This patch is use to enable 'xm vcpu-set'
> command for HVM domain. It setup vcpu watch at xenstore, and at qemu
> side, handle vcpu online/offline accordingly.  With this patch, 'xm
> vcpu-set' command works for both PV and HVM guest with same format.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> Port from qemu-xen-traditionnal to qemu-xen:
>   qemu-xen does not support anymore command through xenstore, so this
>   path include an initialisation of a xenstore loop.
>   An upstream of this patch would need to go QMP.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  hw/acpi_piix4.c |  5 +++--
>  xen-all.c       | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 49c38d3..4c01aa2 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -728,8 +728,8 @@ static int disable_processor(PIIX4PMState *g, int cpu)
>  
>  void qemu_cpu_add_remove(int cpu, int state)
>  {
> -    if ((cpu <=0) || (cpu >= max_cpus)) {
> -        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
> +    if ((cpu < 0) || (cpu >= max_cpus)) {
> +        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", max_cpus - 1);
>          return;
>      }
>  
> @@ -742,6 +742,7 @@ void qemu_cpu_add_remove(int cpu, int state)
>              return;
>          }
>      }
> +    fprintf(stderr, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);
>  
>      pm_update_sci(acpi_state);
>  }
> diff --git a/xen-all.c b/xen-all.c
> index daf43b9..04b88a6 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -99,6 +99,8 @@ typedef struct XenIOState {
>      Notifier suspend;
>  } XenIOState;
>  
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state);
> +
>  /* Xen specific function for piix pci */
>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> @@ -1170,6 +1172,7 @@ int xen_hvm_init(void)
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
>      xen_read_physmap(state);
> +    xen_xenstore_watch_vcpu_set(state);
>  
>      return 0;
>  }
> @@ -1234,3 +1237,65 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>          }
>      }
>  }
> +
> +/* Xenstore watch init for vcpu-set */
> +
> +static void xenstore_process_vcpu_set_event(char **vec, struct xs_handle *xsh)
> +{
> +    char *act = NULL;
> +    char *vcpustr, *node = vec[XS_WATCH_PATH];
> +    unsigned int vcpu, len;
> +
> +    vcpustr = strstr(node, "cpu/");
> +    if (!vcpustr) {
> +        fprintf(stderr, "vcpu-set: watch node error.\n");
> +        return;
> +    }
> +    sscanf(vcpustr, "cpu/%u", &vcpu);
> +
> +    act = xs_read(xsh, XBT_NULL, node, &len);
> +    if (!act) {
> +        fprintf(stderr, "vcpu-set: no command yet.\n");
> +        return;
> +    }
> +
> +    if (!strncmp(act, "online", len))
> +        qemu_cpu_add_remove(vcpu, 1);
> +    else if (!strncmp(act, "offline", len))
> +        qemu_cpu_add_remove(vcpu, 0);
> +    else
> +        fprintf(stderr, "vcpu-set: command error.\n");
> +
> +    free(act);
> +    return;
> +}
> +
> +static void xenstore_process_event(void *opaque)
> +{
> +    char **vec;
> +    unsigned int num;
> +    struct xs_handle *xsh = opaque;
> +
> +    vec = xs_read_watch(xsh, &num);
> +    if (!vec)
> +        return;
> +
> +    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
> +        xenstore_process_vcpu_set_event(vec, xsh);
> +        goto out;
> +    }
> +
> + out:
> +    free(vec);
> +}
> +
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state)
> +{
> +    char path[40];
> +    /* Set a watch for vcpu-set */
> +    snprintf(path, sizeof(path), "/local/domain/%d/cpu", xen_domid);
> +    xs_watch(state->xenstore, path, "vcpu-set");
> +
> +    qemu_set_fd_handler(xs_fileno(state->xenstore), xenstore_process_event,
> +                        NULL, state->xenstore);
> +}
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 3/4] Update vcpu hotplug logic

[Konrad Rzeszutek Wilk]

On Fri, May 31, 2013 at 05:33:12PM +0100, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Add vcpu online/offline check to avoid redundant SCI interrupt.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> Port from qemu-xen-traditionnal to qemu-xen.
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
.. some notes below.
> ---
>  hw/acpi_piix4.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index bc7b454..49c38d3 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -706,16 +706,24 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>      return 0;
>  }
>  
> -static void enable_processor(PIIX4PMState *g, int cpu)
> +static int enable_processor(PIIX4PMState *g, int cpu)
>  {
> +    if (g->cpus_sts[cpu/8] & (1 << (cpu%8)))
> +        return 0;

Don't know if the styleguide in QEME is at this point, but I thought
that it was suppose to have space between the "/" and "%", so you have:

    if (g->cpus_sts[cpu / 8] & (1 << (cpu % 8)))

?
> +
>      g->ar.gpe.sts[0] |= 4;
>      g->cpus_sts[cpu/8] |= (1 << (cpu%8));
> +    return 1;
>  }
>  
> -static void disable_processor(PIIX4PMState *g, int cpu)
> +static int disable_processor(PIIX4PMState *g, int cpu)
>  {
> +    if (!(g->cpus_sts[cpu/8] & (1 << (cpu%8))))
> +        return 0;
> +
>      g->ar.gpe.sts[0] |= 4;
>      g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
> +    return 1;
>  }
>  
>  void qemu_cpu_add_remove(int cpu, int state)
> @@ -725,10 +733,15 @@ void qemu_cpu_add_remove(int cpu, int state)
>          return;
>      }
>  
> -    if (state)
> -        enable_processor(acpi_state, cpu);
> -    else
> -        disable_processor(acpi_state, cpu);
> +    if (state) {
> +        if (!enable_processor(acpi_state, cpu)) {
> +            return;
> +        }
> +    } else {
> +        if (!disable_processor(acpi_state, cpu)) {
> +            return;
> +        }
> +    }
>  
>      pm_update_sci(acpi_state);
>  }
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Konrad Rzeszutek Wilk]

On Fri, May 31, 2013 at 06:14:57PM +0100, Anthony PERARD wrote:

Hm, you might want to also back-port (or up-port) the three fixes
I had posted that fix the race.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 1/4] HVM vcpu add/remove: qemu logic for vcpu add/revmoe

[Konrad Rzeszutek Wilk]

On Fri, May 31, 2013 at 05:33:10PM +0100, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> -- at qemu side, get vcpu_avail which used for original cpu avail map;
> -- setup gpe ioread/iowrite at qmeu;
> -- setup vcpu add/remove user interface through monitor;
> -- setup SCI logic;
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> [ PATCH 4/4 ] HVM vcpu add/remove: qemu logic for vcpu add/revmoe
> 
> Port from qemu-xen-traditionnal to qemu-xen.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hmp-commands.hx |  9 +++++++
>  hw/acpi_piix4.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  monitor.c       | 18 ++++++++++++++
>  qemu-options.hx |  3 +++
>  sysemu.h        |  3 +++
>  vl.c            |  6 +++++
>  6 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..e92f173 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1581,3 +1581,12 @@ ETEXI
>  STEXI
>  @end table
>  ETEXI
> +
> +HXCOMM TODO doc

What is the TODO For?

> +    {
> +        .name       = "cpu_set",
> +        .args_type  = "cpu_index:i,status:s",
> +        .params     = "cpu [online|offline]",
> +        .help       = "change cpu state",
> +        .mhandler.cmd = do_cpu_set_nr,
> +    },
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 519269a..c73dc7c 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -45,8 +45,11 @@
>  #define PCI_DOWN_BASE 0xae04
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
> +/* ioport to monitor cpu add/remove status */
> +#define PROC_BASE 0xaf00
>  
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
> +#define PIIX4_CPU_HOTPLUG_STATUS 4
>  
>  struct pci_status {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
> @@ -77,6 +80,9 @@ typedef struct PIIX4PMState {
>      uint8_t disable_s3;
>      uint8_t disable_s4;
>      uint8_t s4_val;
> +
> +    /* CPU bitmap */
> +    uint8_t cpus_sts[32];

Could we use a #define for it?
>  } PIIX4PMState;
>  
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
> @@ -95,7 +101,7 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
>          (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
> -          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +          & (PIIX4_PCI_HOTPLUG_STATUS|PIIX4_CPU_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> @@ -602,11 +608,48 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr)
>      return s->pci0_hotplug_enable;
>  }
>  
> +static uint32_t gpe_cpus_readb(void *opaque, uint32_t addr)
> +{
> +    uint32_t val = 0;
> +    PIIX4PMState *g = opaque;
> +
> +    switch (addr) {
> +        case PROC_BASE ... PROC_BASE+31:
> +            val = g->cpus_sts[addr - PROC_BASE];
> +        default:
> +            break;
> +    }
> +
> +    return val;
> +}
> +
> +static void gpe_cpus_writeb(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    switch (addr) {
> +        case PROC_BASE ... PROC_BASE + 31:
> +            /* don't allow to change cpus_sts from inside a guest */
> +            break;
> +        default:
> +            break;
> +    }
> +}
> +
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>                                  PCIHotplugState state);
>  
> +extern uint64_t vcpu_avail;
> +static PIIX4PMState *acpi_state;
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  {
> +    int i = 0, cpus = max_cpus;
> +    char *vcpumap = (char *)&vcpu_avail;
> +
> +    while (cpus > 0) {
> +        s->cpus_sts[i] = vcpumap[i];
> +        i++;
> +        cpus -= 8;
> +    }
> +    acpi_state = s;
>  
>      register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
>      register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
> @@ -620,6 +663,9 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  
>      register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
>  
> +    register_ioport_read(PROC_BASE, 32, 1,  gpe_cpus_readb, s);
> +    register_ioport_write(PROC_BASE, 32, 1, gpe_cpus_writeb, s);
> +
>      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
>  }
>  
> @@ -660,3 +706,30 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>  
>      return 0;
>  }
> +
> +static void enable_processor(PIIX4PMState *g, int cpu)
> +{
> +    g->ar.gpe.sts[0] |= 4;
> +    g->cpus_sts[cpu/8] |= (1 << (cpu%8));

Is the StyleGuide OK with that? Or should it have a space in front and in
the back of "/" and "%" ?
> +}
> +
> +static void disable_processor(PIIX4PMState *g, int cpu)
> +{
> +    g->ar.gpe.sts[0] |= 4;
> +    g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));

Ditto.
> +}
> +
> +void qemu_cpu_add_remove(int cpu, int state)
> +{
> +    if ((cpu <=0) || (cpu >= max_cpus)) {
> +        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
> +        return;
> +    }
> +
> +    if (state)
> +        enable_processor(acpi_state, cpu);
> +    else
> +        disable_processor(acpi_state, cpu);
> +
> +    pm_update_sci(acpi_state);
> +}
> diff --git a/monitor.c b/monitor.c
> index c0e32d6..564373c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2421,6 +2421,24 @@ int monitor_handle_fd_param(Monitor *mon, const char *fdname)
>      return fd;
>  }
>  
> +static void do_cpu_set_nr(Monitor *mon, const QDict *qdict)
> +{
> +    int cpu_index = qdict_get_int(qdict, "cpu_index");
> +    const char *status = qdict_get_str(qdict, "status");
> +    int state;
> +
> +    if (!strcmp(status, "online")) {
> +        state = 1;
> +    } else if (!strcmp(status, "offline")) {
> +        state = 0;
> +    } else {
> +        monitor_printf(mon, "invalid status: %s\n", status);
> +        return;
> +    }
> +
> +    qemu_cpu_add_remove(cpu_index, state);
> +}
> +
>  /* mon_cmds and info_cmds would be sorted at runtime */
>  static mon_cmd_t mon_cmds[] = {
>  #include "hmp-commands.h"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index de43b1b..0ba668f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -94,6 +94,9 @@ given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
>  specifies the maximum number of hotpluggable CPUs.
>  ETEXI
>  
> +DEF("vcpu_avail", HAS_ARG, QEMU_OPTION_vcpu_avail,
> +    "-vcpu_avail bitmap\n", QEMU_ARCH_ALL)
> +
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>      "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
>  STEXI
> diff --git a/sysemu.h b/sysemu.h
> index f5ac664..ed7b264 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -149,6 +149,9 @@ int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
>                        DriveInfo *dinfo, int type);
>  void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>  
> +/* cpu hotplug */
> +void qemu_cpu_add_remove(int cpu, int state);
> +
>  /* generic hotplug */
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  
> diff --git a/vl.c b/vl.c
> index a3ab384..27ae6c1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -207,6 +207,7 @@ int win2k_install_hack = 0;
>  int singlestep = 0;
>  int smp_cpus = 1;
>  int max_cpus = 0;
> +uint64_t vcpu_avail = 1;
>  int smp_cores = 1;
>  int smp_threads = 1;
>  #ifdef CONFIG_VNC
> @@ -3302,6 +3303,11 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_vcpu_avail:
> +                vcpu_avail = atol(optarg);
> +                fprintf(stderr, "qemu: the avail cpu bitmap is %lx\n",
> +                        vcpu_avail);
> +                break;
>  	    case QEMU_OPTION_vnc:
>  #ifdef CONFIG_VNC
>                  display_remote++;
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH] libxl: Use -vcpu_avail with qemu-xen.

[Ian Campbell]

On Fri, 2013-05-31 at 17:39 +0100, Anthony PERARD wrote:
> This require the series CPU hotplug for qemu-xen.
> 
> Note: this patch is valid only for 4.3 as the -vcpu_avail will
> probably not be upstream to Qemu.

Ugh. So how are we going to handle this in the future? libxl ideally
needs to work with upstream qemu or our qemu seamlessly, which would
mean needed to know whether to use -vcpu_avail vs. whatever upstream
has. I'd much prefer to make our qemu export the same interface and use
that...

> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_dm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 1e3a9f4..2db2372 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -530,11 +530,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-smp");
>              if (b_info->avail_vcpus.size) {
>                  int nr_set_cpus = 0;
> +                char *s;
>                  nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus);
>  
>                  flexarray_append(dm_args, libxl__sprintf(gc, "%d,maxcpus=%d",
>                                                           nr_set_cpus,
>                                                           b_info->max_vcpus));
> +                
> +                s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus);
> +                flexarray_vappend(dm_args, "-vcpu_avail",
> +                                  libxl__sprintf(gc, "%s", s), NULL);
> +                free(s);
>              } else
>                  flexarray_append(dm_args, libxl__sprintf(gc, "%d",
>                                                           b_info->max_vcpus));

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Ian Campbell]

On Fri, 2013-05-31 at 18:20 +0100, Anthony PERARD wrote:
> On 31/05/13 17:33, Anthony PERARD wrote:
> > Hi all,
> > 
> > This series ports few patches from qemu-xen-traditionnal to qemu-xen
> > (upstream). There is as little modification as possible on those patches so
> > they *can not* be upstream as is. So this is only for 4.3. A proper series will
> > be done for 4.4.
> > 
> > There make possible to have CPU hotplug on qemu-xen.
> > 
> > A patch for libxl will follow, which make use of the old command line
> > -vcpu_avail with qemu-xen. This command line will probably not be upstream.
> > 
> > BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
> > initialisation, SeaBIOS will count the number of CPU running, and the number is
> > always equal to maxvcpu when SeaBIOS expect less. This happen when we have
> > something like:
> > vcpus = 2
> > maxvcpus = 8
> > in the VM config file. Linux is fine with this and will use only $vcpus.  So
> > the probleme is: an infinit loop in SeaBIOS.
> 
> To let you know, I just realize that QEMU 1.5 (we are using 1.3, I
> think) already have everything needed for CPU hotplug. It will just be a
> matter of plumbing libxl to use the cpu-add QMP command.

Cool! Is the implementation be very different to what we have here? Is
it backportable?

At a minimum I'd prefer that whatever we do now interface wise will
allow libxl to work with the qemu upstream mechanisms in the future (see
my comment on your libxl patch).

> But cpu-remove QMP command seams to be missing from QEMU.

For a long time we didn't support CPU hot remove from HVM guests, in
fact I'm not even sure that we support it today with qemu-trad.

Perhaps we do for PVHVM guests, but I'd half expect that to be using the
PV path.

Ian.

Re: [PATCH 4/4] Implement 'xm vcpu-set' command for HVM guest

[Ian Campbell]

On Fri, 2013-05-31 at 17:33 +0100, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Currently Xen has 'xm vcpu-set' command for PV domain, but not
> available for HVM domain.  This patch is use to enable 'xm vcpu-set'
> command for HVM domain. It setup vcpu watch at xenstore, and at qemu
> side, handle vcpu online/offline accordingly.  With this patch, 'xm
> vcpu-set' command works for both PV and HVM guest with same format.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> Port from qemu-xen-traditionnal to qemu-xen:
>   qemu-xen does not support anymore command through xenstore, so this
>   path include an initialisation of a xenstore loop.
>   An upstream of this patch would need to go QMP.

Because of this forward port the title is inaccurate, since this doesn't
do anything for xm and only xl, right?

How much of the original patch is actually left?

BTW, for forward (or backward) ports including the original changeset ID
in the new comment is very useful.


> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/acpi_piix4.c |  5 +++--
>  xen-all.c       | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 49c38d3..4c01aa2 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -728,8 +728,8 @@ static int disable_processor(PIIX4PMState *g, int cpu)
>  
>  void qemu_cpu_add_remove(int cpu, int state)
>  {
> -    if ((cpu <=0) || (cpu >= max_cpus)) {
> -        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
> +    if ((cpu < 0) || (cpu >= max_cpus)) {
> +        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", max_cpus - 1);
>          return;
>      }
>  
> @@ -742,6 +742,7 @@ void qemu_cpu_add_remove(int cpu, int state)
>              return;
>          }
>      }
> +    fprintf(stderr, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);

Is this left over debugging?

>      pm_update_sci(acpi_state);
>  }
> diff --git a/xen-all.c b/xen-all.c
> index daf43b9..04b88a6 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -99,6 +99,8 @@ typedef struct XenIOState {
>      Notifier suspend;
>  } XenIOState;
>  
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state);
> +
>  /* Xen specific function for piix pci */
>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> @@ -1170,6 +1172,7 @@ int xen_hvm_init(void)
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
>      xen_read_physmap(state);
> +    xen_xenstore_watch_vcpu_set(state);
>  
>      return 0;
>  }
> @@ -1234,3 +1237,65 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>          }
>      }
>  }
> +
> +/* Xenstore watch init for vcpu-set */
> +
> +static void xenstore_process_vcpu_set_event(char **vec, struct xs_handle *xsh)
> +{
> +    char *act = NULL;
> +    char *vcpustr, *node = vec[XS_WATCH_PATH];
> +    unsigned int vcpu, len;
> +
> +    vcpustr = strstr(node, "cpu/");
> +    if (!vcpustr) {
> +        fprintf(stderr, "vcpu-set: watch node error.\n");
> +        return;
> +    }
> +    sscanf(vcpustr, "cpu/%u", &vcpu);
> +
> +    act = xs_read(xsh, XBT_NULL, node, &len);
> +    if (!act) {
> +        fprintf(stderr, "vcpu-set: no command yet.\n");
> +        return;
> +    }
> +
> +    if (!strncmp(act, "online", len))
> +        qemu_cpu_add_remove(vcpu, 1);
> +    else if (!strncmp(act, "offline", len))
> +        qemu_cpu_add_remove(vcpu, 0);
> +    else
> +        fprintf(stderr, "vcpu-set: command error.\n");
> +
> +    free(act);
> +    return;
> +}
> +
> +static void xenstore_process_event(void *opaque)
> +{
> +    char **vec;
> +    unsigned int num;
> +    struct xs_handle *xsh = opaque;
> +
> +    vec = xs_read_watch(xsh, &num);
> +    if (!vec)
> +        return;
> +
> +    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
> +        xenstore_process_vcpu_set_event(vec, xsh);
> +        goto out;
> +    }
> +
> + out:
> +    free(vec);
> +}
> +
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state)
> +{
> +    char path[40];
> +    /* Set a watch for vcpu-set */
> +    snprintf(path, sizeof(path), "/local/domain/%d/cpu", xen_domid);
> +    xs_watch(state->xenstore, path, "vcpu-set");
> +
> +    qemu_set_fd_handler(xs_fileno(state->xenstore), xenstore_process_event,
> +                        NULL, state->xenstore);
> +}

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Ian Campbell]

On Fri, 2013-05-31 at 17:33 +0100, Anthony PERARD wrote:
> Hi all,
> 
> This series ports few patches from qemu-xen-traditionnal to qemu-xen
> (upstream). There is as little modification as possible on those patches so
> they *can not* be upstream as is. So this is only for 4.3. A proper series will
> be done for 4.4.
> 
> There make possible to have CPU hotplug on qemu-xen.
> 
> A patch for libxl will follow, which make use of the old command line
> -vcpu_avail with qemu-xen. This command line will probably not be upstream.
> 
> BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
> initialisation, SeaBIOS will count the number of CPU running, and the number is
> always equal to maxvcpu when SeaBIOS expect less. This happen when we have
> something like:
> vcpus = 2
> maxvcpus = 8
> in the VM config file. Linux is fine with this and will use only $vcpus.  So
> the probleme is: an infinit loop in SeaBIOS.

I'm a bit confused -- if SeaBIOS has an infinite loop how do you even
get to know Linux is OK?

Do you have a fix for SeaBIOS, I don't see it in this series.

Ian.

Re: [PATCH] libxl: Use -vcpu_avail with qemu-xen.

[Stefano Stabellini]

On Mon, 3 Jun 2013, Ian Campbell wrote:
> On Fri, 2013-05-31 at 17:39 +0100, Anthony PERARD wrote:
> > This require the series CPU hotplug for qemu-xen.
> > 
> > Note: this patch is valid only for 4.3 as the -vcpu_avail will
> > probably not be upstream to Qemu.
> 
> Ugh. So how are we going to handle this in the future? libxl ideally
> needs to work with upstream qemu or our qemu seamlessly, which would
> mean needed to know whether to use -vcpu_avail vs. whatever upstream
> has. I'd much prefer to make our qemu export the same interface and use
> that...

Yes, I agree.


> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  tools/libxl/libxl_dm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 1e3a9f4..2db2372 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -530,11 +530,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >              flexarray_append(dm_args, "-smp");
> >              if (b_info->avail_vcpus.size) {
> >                  int nr_set_cpus = 0;
> > +                char *s;
> >                  nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus);
> >  
> >                  flexarray_append(dm_args, libxl__sprintf(gc, "%d,maxcpus=%d",
> >                                                           nr_set_cpus,
> >                                                           b_info->max_vcpus));
> > +                
> > +                s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus);
> > +                flexarray_vappend(dm_args, "-vcpu_avail",
> > +                                  libxl__sprintf(gc, "%s", s), NULL);
> > +                free(s);
> >              } else
> >                  flexarray_append(dm_args, libxl__sprintf(gc, "%d",
> >                                                           b_info->max_vcpus));
> 
> 

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Stefano Stabellini]

On Fri, 31 May 2013, Anthony PERARD wrote:
> Hi all,
> 
> This series ports few patches from qemu-xen-traditionnal to qemu-xen
> (upstream). There is as little modification as possible on those patches so
> they *can not* be upstream as is. So this is only for 4.3. A proper series will
> be done for 4.4.
> 
> There make possible to have CPU hotplug on qemu-xen.
> 
> A patch for libxl will follow, which make use of the old command line
> -vcpu_avail with qemu-xen. This command line will probably not be upstream.
> 
> BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
> initialisation, SeaBIOS will count the number of CPU running, and the number is
> always equal to maxvcpu when SeaBIOS expect less. This happen when we have
> something like:
> vcpus = 2
> maxvcpus = 8
> in the VM config file. Linux is fine with this and will use only $vcpus.  So
> the probleme is: an infinit loop in SeaBIOS.

Is a patch coming for that?



> 
> Ian Jackson (4):
>   HVM vcpu add/remove: qemu logic for vcpu add/revmoe
>   Fix vcpu hotplug bug: get correct vcpu_avail bitmap
>   Update vcpu hotplug logic
>   Implement 'xm vcpu-set' command for HVM guest
> 
>  hmp-commands.hx |  9 ++++++
>  hw/acpi_piix4.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  monitor.c       | 18 ++++++++++++
>  qemu-options.hx |  3 ++
>  sysemu.h        |  4 +++
>  vl.c            | 55 ++++++++++++++++++++++++++++++++++++
>  xen-all.c       | 65 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 241 insertions(+), 1 deletion(-)
> 
> -- 
> Anthony PERARD
> 

Re: [PATCH 1/4] HVM vcpu add/remove: qemu logic for vcpu add/revmoe

[Stefano Stabellini]

On Fri, 31 May 2013, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> -- at qemu side, get vcpu_avail which used for original cpu avail map;
> -- setup gpe ioread/iowrite at qmeu;
> -- setup vcpu add/remove user interface through monitor;
> -- setup SCI logic;
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> [ PATCH 4/4 ] HVM vcpu add/remove: qemu logic for vcpu add/revmoe
> 
> Port from qemu-xen-traditionnal to qemu-xen.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hmp-commands.hx |  9 +++++++
>  hw/acpi_piix4.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  monitor.c       | 18 ++++++++++++++
>  qemu-options.hx |  3 +++
>  sysemu.h        |  3 +++
>  vl.c            |  6 +++++
>  6 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..e92f173 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1581,3 +1581,12 @@ ETEXI
>  STEXI
>  @end table
>  ETEXI
> +
> +HXCOMM TODO doc
> +    {
> +        .name       = "cpu_set",
> +        .args_type  = "cpu_index:i,status:s",
> +        .params     = "cpu [online|offline]",
> +        .help       = "change cpu state",
> +        .mhandler.cmd = do_cpu_set_nr,
> +    },
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 519269a..c73dc7c 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -45,8 +45,11 @@
>  #define PCI_DOWN_BASE 0xae04
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
> +/* ioport to monitor cpu add/remove status */
> +#define PROC_BASE 0xaf00
>  
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
> +#define PIIX4_CPU_HOTPLUG_STATUS 4
>  
>  struct pci_status {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
> @@ -77,6 +80,9 @@ typedef struct PIIX4PMState {
>      uint8_t disable_s3;
>      uint8_t disable_s4;
>      uint8_t s4_val;
> +
> +    /* CPU bitmap */
> +    uint8_t cpus_sts[32];
>  } PIIX4PMState;
>  
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
> @@ -95,7 +101,7 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
>          (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
> -          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +          & (PIIX4_PCI_HOTPLUG_STATUS|PIIX4_CPU_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> @@ -602,11 +608,48 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr)
>      return s->pci0_hotplug_enable;
>  }
>  
> +static uint32_t gpe_cpus_readb(void *opaque, uint32_t addr)
> +{
> +    uint32_t val = 0;
> +    PIIX4PMState *g = opaque;
> +
> +    switch (addr) {
> +        case PROC_BASE ... PROC_BASE+31:
> +            val = g->cpus_sts[addr - PROC_BASE];
> +        default:
> +            break;
> +    }
> +
> +    return val;
> +}
> +
> +static void gpe_cpus_writeb(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    switch (addr) {
> +        case PROC_BASE ... PROC_BASE + 31:
> +            /* don't allow to change cpus_sts from inside a guest */
> +            break;
> +        default:
> +            break;
> +    }
> +}
> +
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>                                  PCIHotplugState state);
>  
> +extern uint64_t vcpu_avail;
> +static PIIX4PMState *acpi_state;
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  {
> +    int i = 0, cpus = max_cpus;
> +    char *vcpumap = (char *)&vcpu_avail;
> +
> +    while (cpus > 0) {
> +        s->cpus_sts[i] = vcpumap[i];
> +        i++;
> +        cpus -= 8;
> +    }
> +    acpi_state = s;
>  
>      register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
>      register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
> @@ -620,6 +663,9 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  
>      register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
>  
> +    register_ioport_read(PROC_BASE, 32, 1,  gpe_cpus_readb, s);
> +    register_ioport_write(PROC_BASE, 32, 1, gpe_cpus_writeb, s);
> +
>      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
>  }
>  
> @@ -660,3 +706,30 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>  
>      return 0;
>  }
> +
> +static void enable_processor(PIIX4PMState *g, int cpu)
> +{
> +    g->ar.gpe.sts[0] |= 4;
> +    g->cpus_sts[cpu/8] |= (1 << (cpu%8));
> +}
> +
> +static void disable_processor(PIIX4PMState *g, int cpu)
> +{
> +    g->ar.gpe.sts[0] |= 4;
> +    g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
> +}
> +
> +void qemu_cpu_add_remove(int cpu, int state)
> +{
> +    if ((cpu <=0) || (cpu >= max_cpus)) {

the original code has cpu < 0

> +        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
> +        return;
> +    }
> +
> +    if (state)
> +        enable_processor(acpi_state, cpu);
> +    else
> +        disable_processor(acpi_state, cpu);
> +
> +    pm_update_sci(acpi_state);
> +}

This is also different, the code we have on qemu-xen-unstable is:

if (gpe_state.gpe0_en[0] & 4) {
        qemu_set_irq(sci_irq, 1);
        qemu_set_irq(sci_irq, 0);
}

what's the reason for the change?


If possible, it would be better to port a QMP command for vcpu hotplug
to avoid compatibility problems, so I would get rid of the code below


> diff --git a/monitor.c b/monitor.c
> index c0e32d6..564373c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2421,6 +2421,24 @@ int monitor_handle_fd_param(Monitor *mon, const char *fdname)
>      return fd;
>  }
>  
> +static void do_cpu_set_nr(Monitor *mon, const QDict *qdict)
> +{
> +    int cpu_index = qdict_get_int(qdict, "cpu_index");
> +    const char *status = qdict_get_str(qdict, "status");
> +    int state;
> +
> +    if (!strcmp(status, "online")) {
> +        state = 1;
> +    } else if (!strcmp(status, "offline")) {
> +        state = 0;
> +    } else {
> +        monitor_printf(mon, "invalid status: %s\n", status);
> +        return;
> +    }
> +
> +    qemu_cpu_add_remove(cpu_index, state);
> +}
> +
>  /* mon_cmds and info_cmds would be sorted at runtime */
>  static mon_cmd_t mon_cmds[] = {
>  #include "hmp-commands.h"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index de43b1b..0ba668f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -94,6 +94,9 @@ given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
>  specifies the maximum number of hotpluggable CPUs.
>  ETEXI
>  
> +DEF("vcpu_avail", HAS_ARG, QEMU_OPTION_vcpu_avail,
> +    "-vcpu_avail bitmap\n", QEMU_ARCH_ALL)
> +
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>      "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
>  STEXI
> diff --git a/sysemu.h b/sysemu.h
> index f5ac664..ed7b264 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -149,6 +149,9 @@ int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
>                        DriveInfo *dinfo, int type);
>  void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>  
> +/* cpu hotplug */
> +void qemu_cpu_add_remove(int cpu, int state);
> +
>  /* generic hotplug */
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  
> diff --git a/vl.c b/vl.c
> index a3ab384..27ae6c1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -207,6 +207,7 @@ int win2k_install_hack = 0;
>  int singlestep = 0;
>  int smp_cpus = 1;
>  int max_cpus = 0;
> +uint64_t vcpu_avail = 1;
>  int smp_cores = 1;
>  int smp_threads = 1;
>  #ifdef CONFIG_VNC
> @@ -3302,6 +3303,11 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_vcpu_avail:
> +                vcpu_avail = atol(optarg);
> +                fprintf(stderr, "qemu: the avail cpu bitmap is %lx\n",
> +                        vcpu_avail);
> +                break;
>  	    case QEMU_OPTION_vnc:
>  #ifdef CONFIG_VNC
>                  display_remote++;

Re: [PATCH 4/4] Implement 'xm vcpu-set' command for HVM guest

[Stefano Stabellini]

On Fri, 31 May 2013, Anthony PERARD wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Currently Xen has 'xm vcpu-set' command for PV domain, but not
> available for HVM domain.  This patch is use to enable 'xm vcpu-set'
> command for HVM domain. It setup vcpu watch at xenstore, and at qemu
> side, handle vcpu online/offline accordingly.  With this patch, 'xm
> vcpu-set' command works for both PV and HVM guest with same format.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> Port from qemu-xen-traditionnal to qemu-xen:
>   qemu-xen does not support anymore command through xenstore, so this
>   path include an initialisation of a xenstore loop.
>   An upstream of this patch would need to go QMP.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/acpi_piix4.c |  5 +++--
>  xen-all.c       | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 49c38d3..4c01aa2 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -728,8 +728,8 @@ static int disable_processor(PIIX4PMState *g, int cpu)
>  
>  void qemu_cpu_add_remove(int cpu, int state)
>  {
> -    if ((cpu <=0) || (cpu >= max_cpus)) {
> -        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", max_cpus - 1);
> +    if ((cpu < 0) || (cpu >= max_cpus)) {
> +        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", max_cpus - 1);
>          return;
>      }

I see where this change is coming from now but..

> @@ -742,6 +742,7 @@ void qemu_cpu_add_remove(int cpu, int state)
>              return;
>          }
>      }
> +    fprintf(stderr, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);
>  
>      pm_update_sci(acpi_state);
>  }

..the pm_update_sci is still in place


> diff --git a/xen-all.c b/xen-all.c
> index daf43b9..04b88a6 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -99,6 +99,8 @@ typedef struct XenIOState {
>      Notifier suspend;
>  } XenIOState;
>  
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state);
> +
>  /* Xen specific function for piix pci */
>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> @@ -1170,6 +1172,7 @@ int xen_hvm_init(void)
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
>      xen_read_physmap(state);
> +    xen_xenstore_watch_vcpu_set(state);
>  
>      return 0;
>  }
> @@ -1234,3 +1237,65 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>          }
>      }
>  }
> +
> +/* Xenstore watch init for vcpu-set */
> +
> +static void xenstore_process_vcpu_set_event(char **vec, struct xs_handle *xsh)
> +{
> +    char *act = NULL;
> +    char *vcpustr, *node = vec[XS_WATCH_PATH];
> +    unsigned int vcpu, len;
> +
> +    vcpustr = strstr(node, "cpu/");
> +    if (!vcpustr) {
> +        fprintf(stderr, "vcpu-set: watch node error.\n");
> +        return;
> +    }
> +    sscanf(vcpustr, "cpu/%u", &vcpu);
> +
> +    act = xs_read(xsh, XBT_NULL, node, &len);
> +    if (!act) {
> +        fprintf(stderr, "vcpu-set: no command yet.\n");
> +        return;
> +    }
> +
> +    if (!strncmp(act, "online", len))
> +        qemu_cpu_add_remove(vcpu, 1);
> +    else if (!strncmp(act, "offline", len))
> +        qemu_cpu_add_remove(vcpu, 0);
> +    else
> +        fprintf(stderr, "vcpu-set: command error.\n");
> +
> +    free(act);
> +    return;
> +}
> +
> +static void xenstore_process_event(void *opaque)
> +{
> +    char **vec;
> +    unsigned int num;
> +    struct xs_handle *xsh = opaque;
> +
> +    vec = xs_read_watch(xsh, &num);
> +    if (!vec)
> +        return;
> +
> +    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
> +        xenstore_process_vcpu_set_event(vec, xsh);
> +        goto out;
> +    }
> +
> + out:
> +    free(vec);
> +}
> +
> +static void xen_xenstore_watch_vcpu_set(XenIOState *state)
> +{
> +    char path[40];
> +    /* Set a watch for vcpu-set */
> +    snprintf(path, sizeof(path), "/local/domain/%d/cpu", xen_domid);
> +    xs_watch(state->xenstore, path, "vcpu-set");
> +
> +    qemu_set_fd_handler(xs_fileno(state->xenstore), xenstore_process_event,
> +                        NULL, state->xenstore);
> +}
> -- 
> Anthony PERARD
> 

Re: [PATCH 0/4] CPU hotplug port from qemu-traditionnal to qemu-xen for 4.3.

[Anthony PERARD]

On 03/06/13 09:41, Ian Campbell wrote:
>> > BUG: There is an issue with SeaBIOS (used with qemu-xen). At SMP
>> > initialisation, SeaBIOS will count the number of CPU running, and the number is
>> > always equal to maxvcpu when SeaBIOS expect less. This happen when we have
>> > something like:
>> > vcpus = 2
>> > maxvcpus = 8
>> > in the VM config file. Linux is fine with this and will use only $vcpus.  So
>> > the probleme is: an infinit loop in SeaBIOS.
> I'm a bit confused -- if SeaBIOS has an infinite loop how do you even
> get to know Linux is OK?
> 
> Do you have a fix for SeaBIOS, I don't see it in this series.

The thing is, I don't know how to fix this bug.  This is why there is no
patch.

The probleme is this loop:
> u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
> while (cmos_smp_count + 1 != readl(&CountCPUs))
>   ;

where CountCPUs is incremented by each CPU by a small piece of code and
cmos_smp_count is equal to 1, which is the number of expected vcpus-1.
CountCPUs always go up to maxvcpus under Xen.

I could simply replace the '!=' by a '>' but it feel like a workaround...

-- 
Anthony PERARD

Re: [PATCH] libxl: Use -vcpu_avail with qemu-xen.

[Anthony PERARD]

On 03/06/13 09:37, Ian Campbell wrote:
> On Fri, 2013-05-31 at 17:39 +0100, Anthony PERARD wrote:
>> > This require the series CPU hotplug for qemu-xen.
>> > 
>> > Note: this patch is valid only for 4.3 as the -vcpu_avail will
>> > probably not be upstream to Qemu.
> Ugh. So how are we going to handle this in the future? libxl ideally
> needs to work with upstream qemu or our qemu seamlessly, which would
> mean needed to know whether to use -vcpu_avail vs. whatever upstream
> has. I'd much prefer to make our qemu export the same interface and use
> that...

It could maybe be removed all together. QEMU already know how many
initial CPUs there is. The only difference is the bitmap. -vcpu_avail is
giving to QEMU a bitmap of the running CPUs.

But I don't know if it useful to have a bitmap, or if a number is enough.

With QEMU upstream, we could actually pass the bitmap through several
QMP command "cpu-add CPUx", if a number is not enough.

-- 
Anthony PERARD