[Question] vcpu-set before or after xen_pause_requested

[Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

Keir,

I have a question.
Recently we want to support 'xm vcpu-set' command for HVM vcpu hotplug.
At xenstore.c --> xenstore_parse_domain_config(), we add xs_watch for xendstore cpu node;
At xenstore.c --> xenstore_process_event(), we add xenstore_process_vcpu_set_event() to handle vcpu-set event.

Now the question is,
At xenstore.c --> xenstore_process_event(), where is right place to add xenstore_process_vcpu_set_event():
1. add xenstore_process_vcpu_set_event() before xen_pause_requested check? or
2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
That means, may vcpu-set take effect between xm save/resume command? seems now pci hotplug and usb hotplug do so.

Thanks,
Jinsong

Re: [Question] vcpu-set before or after xen_pause_requested

[Keir Fraser]

This might be a question for Ian Jackson, since it is qemu related?

 -- Keir

On 15/04/2010 09:58, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> Keir,
> 
> I have a question.
> Recently we want to support 'xm vcpu-set' command for HVM vcpu hotplug.
> At xenstore.c --> xenstore_parse_domain_config(), we add xs_watch for
> xendstore cpu node;
> At xenstore.c --> xenstore_process_event(), we add
> xenstore_process_vcpu_set_event() to handle vcpu-set event.
> 
> Now the question is,
> At xenstore.c --> xenstore_process_event(), where is right place to add
> xenstore_process_vcpu_set_event():
> 1. add xenstore_process_vcpu_set_event() before xen_pause_requested check? or
> 2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
> That means, may vcpu-set take effect between xm save/resume command? seems now
> pci hotplug and usb hotplug do so.
> 
> Thanks,
> Jinsong

Re: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("[Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> At xenstore.c --> xenstore_process_event(), where is right place to add xenstore_process_vcpu_set_event():
> 1. add xenstore_process_vcpu_set_event() before xen_pause_requested check? or
> 2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
> That means, may vcpu-set take effect between xm save/resume command? seems now pci hotplug and usb hotplug do so.

Is your vcpu-set done by writing to the xenstore key
   /local/domain/0/device-model/<domain>/command
?  If so it should be added as a new branch to the ifs in to
xenstore_process_dm_command_event, and you should make sure that when
your command is processed you correctly write a "processed" value to
that same xenstore key, just as usb-add does.  Because only one
command can be processed at once there is no possibility that a domain
which is supposed to be pausing could simultaneously be told to do a
hotplug (or vcpu-set).

Please also add corresponding code to xl and libxl to do the vcpu-set
operation.  We're hoping make xl and libxl the primary mechanism in
4.1.

If your vcpu-set is not done by writing to /command, how does it work
and what is the protocol ?  Or haven't you designed the protocol yet?
If you haven't I'd suggest using the existing /command mechanism.

The logic surrounding pci-{rem,ins} seems to be wrong to me because it
does not write a "processed" value.  So don't use that as an example.

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

Ian Jackson wrote:
> 
> Is your vcpu-set done by writing to the xenstore key
>    /local/domain/0/device-model/<domain>/command
No. Currently it has 'xm vcpu-set' command for PV domain, but not available for HVM domain.
We keep same (HVM) xm command --> xend server --> xenstore path as PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and handle at qemu side.

> 
> Please also add corresponding code to xl and libxl to do the vcpu-set
> operation.  We're hoping make xl and libxl the primary mechanism in
> 4.1.
OK, we will add xl and libxl at next patch. Attached are our patches for 'xm vcpu-set' command for HVM domain.


Ian,

We implement patches to enable 'xm vcpu-set' command for HVM domain, as attached.
Our question is, at qemu-xm-vcpu-set-2.patch, is it safe to do 'xenstore_process_vcpu_set_event()' before 'xen_pause_requested' check?
Or, is it safe for 'xm vcpu-set' command runing between 'xm save' and 'xm resume'?

Thanks,
Jinsong

[-- Attachment #2: qemu-xm-vcpu-set-1.patch --]
[-- Type: application/octet-stream, Size: 1708 bytes --]

From bcf02bd010a38e7660f4dd7b36bd618a623c3366 Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Mon, 12 Apr 2010 17:03:14 +0800
Subject: [PATCH] Update vcpu hotplug logic

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

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
---
 hw/piix4acpi.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index fb905d1..3c52c4b 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -727,16 +727,24 @@ void i440fx_init_memory_mappings(PCIDevice *d) {
     /* our implementation doesn't need this */
 }
 
-static void enable_processor(GPEState *g, int cpu)
+static int enable_processor(GPEState *g, int cpu)
 {
+    if (g->cpus_sts[cpu/8] & (1 << (cpu%8)))
+        return 0;
+
     g->gpe0_sts[0] |= 4;
     g->cpus_sts[cpu/8] |= (1 << (cpu%8));
+    return 1;
 }
 
-static void disable_processor(GPEState *g, int cpu)
+static int disable_processor(GPEState *g, int cpu)
 {
+    if (!(g->cpus_sts[cpu/8] & (1 << (cpu%8))))
+        return 0;
+
     g->gpe0_sts[0] |= 4;
     g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
+    return 1;
 }
 
 void qemu_cpu_add_remove(int cpu, int state)
@@ -746,10 +754,13 @@ void qemu_cpu_add_remove(int cpu, int state)
         return;
     }
 
-    if (state)
-        enable_processor(&gpe_state, cpu);
-    else
-        disable_processor(&gpe_state, cpu);
+    if (state) {
+        if (!enable_processor(&gpe_state, cpu))
+            return;
+    } else {
+        if (!disable_processor(&gpe_state, cpu))
+            return;
+    }
 
     if (gpe_state.gpe0_en[0] & 4) {
         qemu_set_irq(sci_irq, 1);
-- 
1.5.6.4


[-- Attachment #3: qemu-xm-vcpu-set-2.patch --]
[-- Type: application/octet-stream, Size: 3441 bytes --]

From 9c88f5c3d7740c9b80069103c9395cd1a01442cb Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Sun, 18 Apr 2010 05:24:21 +0800
Subject: [PATCH] Implement 'xm vcpu-set' command for HVM guest

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>
---
 hw/piix4acpi.c |    5 +++--
 xenstore.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index 3c52c4b..1efa77d 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -749,8 +749,8 @@ static int disable_processor(GPEState *g, int cpu)
 
 void qemu_cpu_add_remove(int cpu, int state)
 {
-    if ((cpu <=0) || (cpu >= vcpus)) {
-        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", vcpus - 1);
+    if ((cpu <0) || (cpu >= vcpus)) {
+        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", vcpus - 1);
         return;
     }
 
@@ -761,6 +761,7 @@ void qemu_cpu_add_remove(int cpu, int state)
         if (!disable_processor(&gpe_state, cpu))
             return;
     }
+    fprintf(logfile, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);
 
     if (gpe_state.gpe0_en[0] & 4) {
         qemu_set_irq(sci_irq, 1);
diff --git a/xenstore.c b/xenstore.c
index 89b1938..43d30ee 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -657,6 +657,12 @@ void xenstore_parse_domain_config(int hvm_domid)
         fprintf(logfile, "Watching %s\n", buf);
     }
 
+    /* Set a watch for vcpu-set */
+    if (pasprintf(&buf, "/local/domain/%u/cpu", domid) != -1) {
+        xs_watch(xsh, buf, "vcpu-set");
+        fprintf(logfile, "Watching %s\n", buf);
+    }
+
     /* no need for ifdef CONFIG_STUBDOM, since in the qemu case
      * hvm_domid is always equal to domid */
     hvm_domid = domid;
@@ -938,6 +944,36 @@ void xenstore_record_dm_state(const char *state)
     xenstore_record_dm("state", state);
 }
 
+static void xenstore_process_vcpu_set_event(char **vec)
+{
+    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;
+}
+
 void xenstore_process_event(void *opaque)
 {
     char **vec, *offset, *bpath = NULL, *buf = NULL, *drv = NULL, *image = NULL;
@@ -958,6 +994,11 @@ void xenstore_process_event(void *opaque)
         goto out;
     }
 
+    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
+        xenstore_process_vcpu_set_event(vec);
+        goto out;
+    }
+
     /* if we are paused don't process anything else */
     if (xen_pause_requested)
         goto out;
-- 
1.5.6.4


[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
...
> We keep same (HVM) xm command --> xend server --> xenstore path as
> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
> handle at qemu side.

I've looked at your patch 2 and it's not correct because there is no
acknowledgement back to the utility which changes xenstore.  You have
to close the loop, if for no other reason than if there are two
xenstore changes in a row which the receiving qemu-dm only gets around
to dealing with after the second, it will see only the second value.

How does the PV vcpu protocol deal with this ?  Doesn't a PV guest
find out about VCPU changes from Xen ?

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): ...
>> We keep same (HVM) xm command --> xend server --> xenstore path as
>> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
>> handle at qemu side.
> 
> I've looked at your patch 2 and it's not correct because there is no
> acknowledgement back to the utility which changes xenstore.  You have
> to close the loop, if for no other reason than if there are two
> xenstore changes in a row which the receiving qemu-dm only gets around
> to dealing with after the second, it will see only the second value.

I think no problem here. In our test of the patch, 16 xenstore changes in row (vcpu0~15), and it trigger 16 event.

> 
> How does the PV vcpu protocol deal with this ?  Doesn't a PV guest
> find out about VCPU changes from Xen ?

'xm vcpu-set' command works for PV now. Nixon and Campbell implement vcpu-set PV dirver at drivers/xen/cpu_hotplug.c. CC them :)

> 

Ian, 

I noticed that qemu watch xenstore nodes and handle event in a close-loop way, 
like, usb-add/usb-del watch '/local/domain/0/device-model/domid/command' node and response xenstore with 'usb-added' / 'usb-deleted'. 
It's one way to communicate between xenstore and qemu.

However, is it the only way to communicate between qemu/xenstore, or between PV/xenstore?
I check 'xm vcpu-set' command path, it now works for PV domain in an open-loop way:
1). PV register a xenbus_watch
        static struct xenbus_watch cpu_watch = {
                .node = "cpu",
                .callback = handle_vcpu_hotplug_event};

        (void)register_xenbus_watch(&cpu_watch);
2). when xend write xenstore cpu node or its sub-level nodes, it trigger callback function handle_vcpu_hotplug_event(), then xenbus_scanf() / xenbus_read() ...

Before we implement our current patch, in fact we have 2 choices:
    A). keep same HVM 'xm command --> xend --> xenstore' path with PV, qemu watch '/local/domain/domid/cpu', trigger callback function in open-loop way (also similar as PV). The advantage is we have unify 'xm command --> xend --> xenstore' path for both PV and HVM.
    B). qemu work in close-loop way, like 'usb-add' and 'usb-del' command. The disadvantage is, we need add an hvm path at 'xm command --> xend --> xenstore', so there are 2 path for 'xm vcpu-set' command, 1 for PV and 1 for HVM. It's not beautiful. BTW, since vcpu-set need watch different xenstore node with 'usb-add''usb-del', we cannot re-use 'usb' path or function like signalDeviceModel(). It need add more code to 'xm command --> xend --> xendstore'.

Current our patch use scheme A). However, I think scheme B) also can work fine. It depend on you and let me know your decision :)


Thanks,
Jinsong

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> I think no problem here. In our test of the patch, 16 xenstore
> changes in row (vcpu0~15), and it trigger 16 event.

Firstly, testing is no way to eliminate the possibility of races.
That must be done by analysis.

Secondly, yes, you will in the current implementation get 16 watch
triggers for 16 changes (although that's not guaranteed to remain the
case).  But if you don't do xs_read in time for one of them you will
miss one of the 16 different values.

> I noticed that qemu watch xenstore nodes and handle event in a
> close-loop way, like, usb-add/usb-del watch
> '/local/domain/0/device-model/domid/command' node and response
> xenstore with 'usb-added' / 'usb-deleted'.  It's one way to
> communicate between xenstore and qemu.

Yes, that is how it should be done.

> However, is it the only way to communicate between qemu/xenstore, or between PV/xenstore?
> I check 'xm vcpu-set' command path, it now works for PV domain in an open-loop way:
> 1). PV register a xenbus_watch
>         static struct xenbus_watch cpu_watch = {
>                 .node = "cpu",
>                 .callback = handle_vcpu_hotplug_event};
> 
>         (void)register_xenbus_watch(&cpu_watch);
> 2). when xend write xenstore cpu node or its sub-level nodes, it trigger callback function handle_vcpu_hotplug_event(), then xenbus_scanf() / xenbus_read() ...

This is broken.  If for any reason multiple vcpu-set actions happen in
quick succession, before the PV guest is scheduled, the
xenbus_scanf/read will see only the last one.

The protocol should be fixed before we implement any more of it.

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

If so, things become some complicated.
I list sub-tasks to implement 'xm vcpu-set':
1. setup a reliable communication protocol between xenstore and HVM qemu/ PV driver, similar with what usb-add/usb-del use;
2. update old 'xm command --> xend --> xenstore' path according to new protocal, and keep an unify 'xm command --> xend --> xnestore' path for both HVM and PV;
3. update old PV driver to close-loop way;
4. update our HVM patch to close-loop way;

Is it OK, or something else ignored?

Thanks,
Jinsong


Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> I think no problem here. In our test of the patch, 16 xenstore
>> changes in row (vcpu0~15), and it trigger 16 event.
> 
> Firstly, testing is no way to eliminate the possibility of races.
> That must be done by analysis.
> 
> Secondly, yes, you will in the current implementation get 16 watch
> triggers for 16 changes (although that's not guaranteed to remain the
> case).  But if you don't do xs_read in time for one of them you will
> miss one of the 16 different values.
> 
>> I noticed that qemu watch xenstore nodes and handle event in a
>> close-loop way, like, usb-add/usb-del watch
>> '/local/domain/0/device-model/domid/command' node and response
>> xenstore with 'usb-added' / 'usb-deleted'.  It's one way to
>> communicate between xenstore and qemu.
> 
> Yes, that is how it should be done.
> 
>> However, is it the only way to communicate between qemu/xenstore, or
>> between PV/xenstore? 
>> I check 'xm vcpu-set' command path, it now works for PV domain in an
>> open-loop way: 1). PV register a xenbus_watch
>>         static struct xenbus_watch cpu_watch = {
>>                 .node = "cpu",
>>                 .callback = handle_vcpu_hotplug_event};
>> 
>>         (void)register_xenbus_watch(&cpu_watch);
>> 2). when xend write xenstore cpu node or its sub-level nodes, it
>> trigger callback function handle_vcpu_hotplug_event(), then
>> xenbus_scanf() / xenbus_read() ...  
> 
> This is broken.  If for any reason multiple vcpu-set actions happen in
> quick succession, before the PV guest is scheduled, the
> xenbus_scanf/read will see only the last one.
> 
> The protocol should be fixed before we implement any more of it.
> 
> Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> Is it OK, or something else ignored?

That list seems about right.  Sorry about that.  (It would have been
better to get it right the first time!)

One question: what about old PV guests ?  Do you care about them or
can we just decree that they won't support the feature ?

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> Is it OK, or something else ignored?
> 
> That list seems about right.  Sorry about that.  (It would have been
> better to get it right the first time!)
> 
> One question: what about old PV guests ?  Do you care about them or
> can we just decree that they won't support the feature ?
> 

Yes, that's a problem.

How about this solution:
For trigger PV dirver, it's easy to keep same event/node with old, though some change to 'xm command --> xend --> xenstore' path.
For response state, maybe we can error/warning log to xend.log when timeout, like 'Timed out waiting for vcpu-set action, please update your PV driver to latest version'?.
In this way, old PV guest can still work in old way (no bad than before) with warning message, suggesting user to update their PV driver.

Thanks,
Jinsong

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> How about this solution:
...
> In this way, old PV guest can still work in old way (no bad than
> before) with warning message, suggesting user to update their PV
> driver.

But, in new guests which are malfunctioning, things will not be
handled properly: the timeout will trigger and it will seem to have
worked.  (Consider a guest which is paused or is starved of CPU
because the host is very busy.)  There is also no good value for the
timeout.

I would suggest using a different xenstore key name for the new
protocol.  That way you don't get the feature with old drivers but at
least you don't make malfunctions worse with the new ones.

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> How about this solution:
> ...
>> In this way, old PV guest can still work in old way (no bad than
>> before) with warning message, suggesting user to update their PV
>> driver.
> 
> But, in new guests which are malfunctioning, things will not be
> handled properly: the timeout will trigger and it will seem to have
> worked.  (Consider a guest which is paused or is starved of CPU
> because the host is very busy.)  There is also no good value for the
> timeout.
> 
> I would suggest using a different xenstore key name for the new
> protocol.  That way you don't get the feature with old drivers but at
> least you don't make malfunctions worse with the new ones.
> 

Ian,

I'm not quite clear your idea. Per my understanding, a new xenstore key name cannot avoid the issue of old key, like timeout value, malfunction guest, ...
Currently I have no enough resource to solve all problems, so how about you solve old issue that PV driver has, and then I will update my HVM vcpu-set patch based on the new protocol?

Thanks,
Jinsong

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> I'm not quite clear your idea. Per my understanding, a new xenstore
> key name cannot avoid the issue of old key, like timeout value,
> malfunction guest, ...

How does the current arrangement deal with an attempt to "xm vcpu-set"
a PV guest which doesn't support that operation ?

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> I'm not quite clear your idea. Per my understanding, a new xenstore
>> key name cannot avoid the issue of old key, like timeout value,
>> malfunction guest, ...
> 
> How does the current arrangement deal with an attempt to "xm vcpu-set"
> a PV guest which doesn't support that operation ?
> 

Currently if PV guest doesn't support vcpu-hotplug, it then has no xenbus_watch to related xenstore nodes.
then if attempt 'xm vcpu-set', 
1). xend will still write to xenstore nodes with value "online/offline"
/local/domain/domid/cpu/0/availability
/local/domain/domid/cpu/1/availability
......
/local/domain/domid/cpu/maxvcpus-1/availability
2). PV guest has not any info about xenstore change, and will not be influenced by 'xm vcpu-set' command;

Thanks,
Jinsong

RE: [Question] vcpu-set before or after xen_pause_requested

[Jiang, Yunhong]

I'm curios do we really care about we lost the intermediate value. 
For example, considering user do vcpu-set 2 (i.e set vcpu number to 2), vcpu-set 4, and vcpu-set 2, and guest didn't take any action at all, would it be really so important? Or, if it is 2->4->3, should the xm vcpu-set itself really need care 4 vcpu is bringup in some time?

Especially consider guest (i.e. OS) will take several steps to really utilize the new added vcpu. For example, when CPU hot-added, Linux kernel will send a uevent to user space, and ueventd will try to bring up the CPU after get the event. In a system without properly setup the uevent script, the xm vcpu-set command will not cause changes to both guest kernel and xen hypervisor at all. In old kernel, even hot-remove will go-through uevent also. Also, the uevent script may combine several uevent still, depends on how the script is implemented.

I think the close loop should happen in caller to the vcpu-set, if the caller really cares about the result of the command, it should check xen's vcpu status to check if the command has take effect or not.

Any idea?

Thanks
Yunhong Jiang

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Ian Jackson
>Sent: Monday, April 19, 2010 11:48 PM
>To: Liu, Jinsong
>Cc: xen-devel; Keir Fraser; Zhai, Edwin; Jiang, Yunhong
>Subject: RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested
>
>Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
>xen_pause_requested"):
>...
>> We keep same (HVM) xm command --> xend server --> xenstore path as
>> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
>> handle at qemu side.
>
>I've looked at your patch 2 and it's not correct because there is no
>acknowledgement back to the utility which changes xenstore.  You have
>to close the loop, if for no other reason than if there are two
>xenstore changes in a row which the receiving qemu-dm only gets around
>to dealing with after the second, it will see only the second value.
>
>How does the PV vcpu protocol deal with this ?  Doesn't a PV guest
>find out about VCPU changes from Xen ?
>
>Ian.
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

RE: [Question] vcpu-set before or after xen_pause_requested

[Ian Jackson]

Jiang, Yunhong writes ("RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested"):
> For example, considering user do vcpu-set 2 (i.e set vcpu number to
> 2), vcpu-set 4, and vcpu-set 2, and guest didn't take any action at
> all, would it be really so important? Or, if it is 2->4->3, should
> the xm vcpu-set itself really need care 4 vcpu is bringup in some
> time?

This is a good point.  If you are sure that intermediate values are
not important, and that changes are idempotent, then the open loop
approach is quite fine.

Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Jiang, Yunhong]

Ian, thanks for your reply. Yes, I think the intermediate values is not important in general. If it is really important, the caller of "xm  vcpu-set" should take care of it, i.e. do xm vcpu-set, wait and check the vcpu's status (maybe from xen hypervisor) with timeout mechanism.

BTW, from the followed changeset in dom0's drivers/xen/cpu_hotplug.c , it means the xm vcpu-set may happen even before guest is fully up, at that time, it is difficult for xend to achieve the close loop.

commit d745562cc40bff60f0597004d8128fa0225cc267
Author: Ian Campbell <Ian.Campbell@citrix.com>
Date:   Thu Apr 2 13:24:28 2009 +0100

    xen: honour VCPU availability on boot

    If a VM is booted with offline VCPUs then unplug them during boot. Determining
    the availability of a VCPU requires access to XenStore which is not available
    at the point smp_prepare_cpus() is called, therefore we bring up all VCPUS
    initially and unplug the offline ones as soon as XenStore becomes available.

    Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Thanks
--jyh

>-----Original Message-----
>From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
>Sent: Wednesday, April 28, 2010 6:31 PM
>To: Jiang, Yunhong
>Cc: Liu, Jinsong; xen-devel; Keir Fraser; Zhai, Edwin
>Subject: RE: [Xen-devel] [Question] vcpu-set before or after xen_pause_requested
>
>Jiang, Yunhong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
>xen_pause_requested"):
>> For example, considering user do vcpu-set 2 (i.e set vcpu number to
>> 2), vcpu-set 4, and vcpu-set 2, and guest didn't take any action at
>> all, would it be really so important? Or, if it is 2->4->3, should
>> the xm vcpu-set itself really need care 4 vcpu is bringup in some
>> time?
>
>This is a good point.  If you are sure that intermediate values are
>not important, and that changes are idempotent, then the open loop
>approach is quite fine.
>
>Ian.

RE: [Question] vcpu-set before or after xen_pause_requested

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

Ian Jackson wrote:
> 
> Is your vcpu-set done by writing to the xenstore key
>    /local/domain/0/device-model/<domain>/command
No. Currently it has 'xm vcpu-set' command for PV domain, but not available for HVM domain.
We keep same (HVM) xm command --> xend server --> xenstore path as PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and handle at qemu side.

> 
> Please also add corresponding code to xl and libxl to do the vcpu-set
> operation.  We're hoping make xl and libxl the primary mechanism in
> 4.1.
OK, we will add xl and libxl at next patch. Attached are our patches for 'xm vcpu-set' command for HVM domain.


Ian,

We implement patches to enable 'xm vcpu-set' command for HVM domain, as attached.
Our question is, at qemu-xm-vcpu-set-2.patch, is it safe to do 'xenstore_process_vcpu_set_event()' before 'xen_pause_requested' check?
Or, is it safe for 'xm vcpu-set' command runing between 'xm save' and 'xm resume'?

Thanks,
Jinsong

[-- Attachment #2: qemu-xm-vcpu-set-1.patch --]
[-- Type: application/octet-stream, Size: 1708 bytes --]

From bcf02bd010a38e7660f4dd7b36bd618a623c3366 Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Mon, 12 Apr 2010 17:03:14 +0800
Subject: [PATCH] Update vcpu hotplug logic

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

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
---
 hw/piix4acpi.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index fb905d1..3c52c4b 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -727,16 +727,24 @@ void i440fx_init_memory_mappings(PCIDevice *d) {
     /* our implementation doesn't need this */
 }
 
-static void enable_processor(GPEState *g, int cpu)
+static int enable_processor(GPEState *g, int cpu)
 {
+    if (g->cpus_sts[cpu/8] & (1 << (cpu%8)))
+        return 0;
+
     g->gpe0_sts[0] |= 4;
     g->cpus_sts[cpu/8] |= (1 << (cpu%8));
+    return 1;
 }
 
-static void disable_processor(GPEState *g, int cpu)
+static int disable_processor(GPEState *g, int cpu)
 {
+    if (!(g->cpus_sts[cpu/8] & (1 << (cpu%8))))
+        return 0;
+
     g->gpe0_sts[0] |= 4;
     g->cpus_sts[cpu/8] &= ~(1 << (cpu%8));
+    return 1;
 }
 
 void qemu_cpu_add_remove(int cpu, int state)
@@ -746,10 +754,13 @@ void qemu_cpu_add_remove(int cpu, int state)
         return;
     }
 
-    if (state)
-        enable_processor(&gpe_state, cpu);
-    else
-        disable_processor(&gpe_state, cpu);
+    if (state) {
+        if (!enable_processor(&gpe_state, cpu))
+            return;
+    } else {
+        if (!disable_processor(&gpe_state, cpu))
+            return;
+    }
 
     if (gpe_state.gpe0_en[0] & 4) {
         qemu_set_irq(sci_irq, 1);
-- 
1.5.6.4


[-- Attachment #3: qemu-xm-vcpu-set-2.patch --]
[-- Type: application/octet-stream, Size: 3441 bytes --]

From 9c88f5c3d7740c9b80069103c9395cd1a01442cb Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Sun, 18 Apr 2010 05:24:21 +0800
Subject: [PATCH] Implement 'xm vcpu-set' command for HVM guest

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>
---
 hw/piix4acpi.c |    5 +++--
 xenstore.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index 3c52c4b..1efa77d 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -749,8 +749,8 @@ static int disable_processor(GPEState *g, int cpu)
 
 void qemu_cpu_add_remove(int cpu, int state)
 {
-    if ((cpu <=0) || (cpu >= vcpus)) {
-        fprintf(stderr, "vcpu out of range, should be [1~%d]\n", vcpus - 1);
+    if ((cpu <0) || (cpu >= vcpus)) {
+        fprintf(stderr, "vcpu out of range, should be [0~%d]\n", vcpus - 1);
         return;
     }
 
@@ -761,6 +761,7 @@ void qemu_cpu_add_remove(int cpu, int state)
         if (!disable_processor(&gpe_state, cpu))
             return;
     }
+    fprintf(logfile, "%s vcpu %d\n", state ? "Add" : "Remove", cpu);
 
     if (gpe_state.gpe0_en[0] & 4) {
         qemu_set_irq(sci_irq, 1);
diff --git a/xenstore.c b/xenstore.c
index 89b1938..43d30ee 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -657,6 +657,12 @@ void xenstore_parse_domain_config(int hvm_domid)
         fprintf(logfile, "Watching %s\n", buf);
     }
 
+    /* Set a watch for vcpu-set */
+    if (pasprintf(&buf, "/local/domain/%u/cpu", domid) != -1) {
+        xs_watch(xsh, buf, "vcpu-set");
+        fprintf(logfile, "Watching %s\n", buf);
+    }
+
     /* no need for ifdef CONFIG_STUBDOM, since in the qemu case
      * hvm_domid is always equal to domid */
     hvm_domid = domid;
@@ -938,6 +944,36 @@ void xenstore_record_dm_state(const char *state)
     xenstore_record_dm("state", state);
 }
 
+static void xenstore_process_vcpu_set_event(char **vec)
+{
+    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;
+}
+
 void xenstore_process_event(void *opaque)
 {
     char **vec, *offset, *bpath = NULL, *buf = NULL, *drv = NULL, *image = NULL;
@@ -958,6 +994,11 @@ void xenstore_process_event(void *opaque)
         goto out;
     }
 
+    if (!strcmp(vec[XS_WATCH_TOKEN], "vcpu-set")) {
+        xenstore_process_vcpu_set_event(vec);
+        goto out;
+    }
+
     /* if we are paused don't process anything else */
     if (xen_pause_requested)
         goto out;
-- 
1.5.6.4


[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel