[PATCH 0/3] Support the latest version of qemu COLO

[PATCH 0/3] Support the latest version of qemu COLO

[Zhang Chen]

Because of new version COLO has been merged by QEMU,
Some codes has changed by community comments.
So, we must update Xen COLO codes to support latest qemu.

Zhang Chen (3):
  Don't create default ioreq server
  Add Xen colo support for qemu-upstream colo codes
  Add COLO replication top-id support

 tools/libxl/libxl_colo_qdisk.c |  4 ++--
 tools/libxl/libxl_dm.c         |  8 ++++++--
 xen/arch/x86/hvm/hvm.c         | 11 -----------
 3 files changed, 8 insertions(+), 15 deletions(-)

-- 
2.7.4




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/3] Don't create default ioreq server

[Zhang Chen]

The ioreq server make colo run failed.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 xen/arch/x86/hvm/hvm.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 25dc759..8522852 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5339,17 +5339,6 @@ static int hvmop_get_param(
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-    {
-        domid_t domid;
-
-        /* May need to create server. */
-        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
-        rc = hvm_create_ioreq_server(d, domid, 1,
-                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
-        if ( rc != 0 && rc != -EEXIST )
-            goto out;
-    }
-    /*FALLTHRU*/
     default:
         a.value = d->arch.hvm_domain.params[a.index];
         break;
-- 
2.7.4




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/3] Add Xen colo support for qemu-upstream colo codes

[Zhang Chen]

Because of qemu codes update, we update Xen colo block codes

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 tools/libxl/libxl_colo_qdisk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_colo_qdisk.c b/tools/libxl/libxl_colo_qdisk.c
index d271d1f..300bff2 100644
--- a/tools/libxl/libxl_colo_qdisk.c
+++ b/tools/libxl/libxl_colo_qdisk.c
@@ -169,9 +169,9 @@ static void colo_qdisk_save_preresume(libxl__egc *egc,
     /* qmp command doesn't support the driver "nbd" */
     node = GCSPRINTF("colo_node%d",
                      libxl__device_disk_dev_number(disk->vdev, NULL, NULL));
-    cmd = GCSPRINTF("drive_add buddy driver=replication,mode=primary,"
+    cmd = GCSPRINTF("drive_add -n buddy driver=replication,mode=primary,"
                     "file.driver=nbd,file.host=%s,file.port=%d,"
-                    "file.export=%s,node-name=%s,if=none",
+                    "file.export=%s,node-name=%s",
                     host, port, export_name, node);
     ret = libxl__qmp_hmp(gc, domid, cmd, NULL);
     if (ret)
-- 
2.7.4




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/3] Add COLO replication top-id support

[Zhang Chen]

Because of qemu colo add the top-id, so we update it.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 tools/libxl/libxl_dm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ad366a8..815938e 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -826,8 +826,10 @@ static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *target_path,
          *  file.backing.backing=exportname,
          */
         drive = GCSPRINTF(
-            "if=scsi,bus=0,unit=%d,cache=writeback,driver=replication,"
+            "if=scsi,id=top-colo,bus=0,unit=%d,cache=writeback,"
+            "driver=replication,"
             "mode=secondary,"
+            "top-id=top-colo,"
             "file.driver=qcow2,"
             "file.file.filename=%s,"
             "file.backing.driver=qcow2,"
@@ -889,8 +891,10 @@ static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *target_path,
          *  file.backing.backing=exportname,
          */
         drive = GCSPRINTF(
-            "if=ide,index=%d,media=disk,cache=writeback,driver=replication,"
+            "if=ide,index=%d,id=top-colo,media=disk,cache=writeback,"
+            "driver=replication,"
             "mode=secondary,"
+            "top-id=top-colo,"
             "file.driver=qcow2,"
             "file.file.filename=%s,"
             "file.backing.driver=qcow2,"
-- 
2.7.4




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Andrew Cooper]

On 30/11/16 09:47, Zhang Chen wrote:
> The ioreq server make colo run failed.
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Nack.

You can simply "fix" a COLO issue by breaking a much more common usecase.


What actually breaks in the COLO case here?

~Andrew

> ---
>  xen/arch/x86/hvm/hvm.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 25dc759..8522852 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5339,17 +5339,6 @@ static int hvmop_get_param(
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    {
> -        domid_t domid;
> -
> -        /* May need to create server. */
> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> -        rc = hvm_create_ioreq_server(d, domid, 1,
> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> -        if ( rc != 0 && rc != -EEXIST )
> -            goto out;
> -    }
> -    /*FALLTHRU*/
>      default:
>          a.value = d->arch.hvm_domain.params[a.index];
>          break;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Wei Liu]

On Wed, Nov 30, 2016 at 05:47:50PM +0800, Zhang Chen wrote:
> The ioreq server make colo run failed.
> 
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 25dc759..8522852 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5339,17 +5339,6 @@ static int hvmop_get_param(
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    {
> -        domid_t domid;
> -
> -        /* May need to create server. */
> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> -        rc = hvm_create_ioreq_server(d, domid, 1,
> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> -        if ( rc != 0 && rc != -EEXIST )
> -            goto out;
> -    }
> -    /*FALLTHRU*/

What is broken by ioreq server? I don't think you can change the code
like this because this is definitely going to be wrong for other use
cases -- try create a guest without COLO.

If you can be more specific about what is broken in COLO we might be
able to devise a fix for you.

>      default:
>          a.value = d->arch.hvm_domain.params[a.index];
>          break;
> -- 
> 2.7.4
> 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] Add Xen colo support for qemu-upstream colo codes

[Wei Liu]

On Wed, Nov 30, 2016 at 05:47:51PM +0800, Zhang Chen wrote:
> Because of qemu codes update, we update Xen colo block codes
> 
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

COLO being an experimental feature means that you can change it at will,
so for both patches:

Acked-by: Wei Liu <wei.liu2@citrix.com>

But, this sort of patch does make me wonder how QEMU handles backward
compatibility for command line options...

> ---
>  tools/libxl/libxl_colo_qdisk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_colo_qdisk.c b/tools/libxl/libxl_colo_qdisk.c
> index d271d1f..300bff2 100644
> --- a/tools/libxl/libxl_colo_qdisk.c
> +++ b/tools/libxl/libxl_colo_qdisk.c
> @@ -169,9 +169,9 @@ static void colo_qdisk_save_preresume(libxl__egc *egc,
>      /* qmp command doesn't support the driver "nbd" */
>      node = GCSPRINTF("colo_node%d",
>                       libxl__device_disk_dev_number(disk->vdev, NULL, NULL));
> -    cmd = GCSPRINTF("drive_add buddy driver=replication,mode=primary,"
> +    cmd = GCSPRINTF("drive_add -n buddy driver=replication,mode=primary,"
>                      "file.driver=nbd,file.host=%s,file.port=%d,"
> -                    "file.export=%s,node-name=%s,if=none",
> +                    "file.export=%s,node-name=%s",
>                      host, port, export_name, node);
>      ret = libxl__qmp_hmp(gc, domid, cmd, NULL);
>      if (ret)
> -- 
> 2.7.4
> 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Wen Congyang]

At 2016/11/30 20:25, Andrew Cooper wrote:
> On 30/11/16 09:47, Zhang Chen wrote:
>> The ioreq server make colo run failed.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> Nack.
>
> You can simply "fix" a COLO issue by breaking a much more common usecase.

Yes, this patch is wrong. It is only in my git tree to let COLO work.

>
>
> What actually breaks in the COLO case here?

I have reported this BUG last year:
https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02850.html


Thanks
Wen Congyang

>
> ~Andrew
>
>> ---
>>   xen/arch/x86/hvm/hvm.c | 11 -----------
>>   1 file changed, 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 25dc759..8522852 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5339,17 +5339,6 @@ static int hvmop_get_param(
>>       case HVM_PARAM_IOREQ_PFN:
>>       case HVM_PARAM_BUFIOREQ_PFN:
>>       case HVM_PARAM_BUFIOREQ_EVTCHN:
>> -    {
>> -        domid_t domid;
>> -
>> -        /* May need to create server. */
>> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
>> -        rc = hvm_create_ioreq_server(d, domid, 1,
>> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
>> -        if ( rc != 0 && rc != -EEXIST )
>> -            goto out;
>> -    }
>> -    /*FALLTHRU*/
>>       default:
>>           a.value = d->arch.hvm_domain.params[a.index];
>>           break;
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Zhang Chen]

On 12/01/2016 09:19 PM, Wei Liu wrote:
> On Wed, Nov 30, 2016 at 05:47:50PM +0800, Zhang Chen wrote:
>> The ioreq server make colo run failed.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   xen/arch/x86/hvm/hvm.c | 11 -----------
>>   1 file changed, 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 25dc759..8522852 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5339,17 +5339,6 @@ static int hvmop_get_param(
>>       case HVM_PARAM_IOREQ_PFN:
>>       case HVM_PARAM_BUFIOREQ_PFN:
>>       case HVM_PARAM_BUFIOREQ_EVTCHN:
>> -    {
>> -        domid_t domid;
>> -
>> -        /* May need to create server. */
>> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
>> -        rc = hvm_create_ioreq_server(d, domid, 1,
>> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
>> -        if ( rc != 0 && rc != -EEXIST )
>> -            goto out;
>> -    }
>> -    /*FALLTHRU*/
> What is broken by ioreq server? I don't think you can change the code
> like this because this is definitely going to be wrong for other use
> cases -- try create a guest without COLO.

Yes, this patch is wrong for other use cases.

> If you can be more specific about what is broken in COLO we might be
> able to devise a fix for you.

My workmate have reported this BUG last year:
https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02850.html

Can you give me a fix or a detailed suggestion for this bug?


Thanks
Zhang Chen

>>       default:
>>           a.value = d->arch.hvm_domain.params[a.index];
>>           break;
>> -- 
>> 2.7.4
>>
>>
>>
>
> .
>

-- 
Thanks
zhangchen




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Konrad Rzeszutek Wilk]

.snip..
> > If you can be more specific about what is broken in COLO we might be
> > able to devise a fix for you.
> 
> My workmate have reported this BUG last year:
> https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02850.html

Paul, Andrew was asking about:

	This bug is caused by the read side effects of HVM_PARAM_IOREQ_PFN. The migration code needs a way of being able to query whether a default ioreq server exists, without creating one.

	Can you remember what the justification for the read side effects were? ISTR that it was only for qemu compatibility until the ioreq server work got in upstream. If that was the case, can we drop the read side effects now and mandate that all qemus explicitly create their ioreq servers (even if this involves creating a default ioreq server for qemu-trad)?


?

Full thread below:

[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] question about migration

To: Wen Congyang <wency@xxxxxxxxxxxxxx>
From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Date: Tue, 29 Dec 2015 11:24:14 +0000
Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>, xen devel <xen-devel@xxxxxxxxxxxxx>
Delivery-date: Tue, 29 Dec 2015 11:24:33 +0000
List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 25/12/2015 01:45, Wen Congyang wrote:
On 12/24/2015 08:36 PM, Andrew Cooper wrote:
On 24/12/15 02:29, Wen Congyang wrote:
Hi Andrew Cooper:

I rebase the COLO codes to the newest upstream xen, and test it. I found
a problem in the test, and I can reproduce this problem via the migration.

How to reproduce:
1. xl cr -p hvm_nopv
2. xl migrate hvm_nopv 192.168.3.1
You are the very first person to try a usecase like this.

It works as much as it does because of your changes to the uncooperative HVM 
domain logic.  I have said repeatedly during review, this is not necessarily a 
safe change to make without an in-depth analysis of the knock-on effects; it 
looks as if you have found the first knock-on effect.

The migration successes, but the vm doesn't run in the target machine.
You can get the reason from 'xl dmesg':
(XEN) HVM2 restore: VMCE_VCPU 1
(XEN) HVM2 restore: TSC_ADJUST 0
(XEN) HVM2 restore: TSC_ADJUST 1
(d2) HVM Loader
(d2) Detected Xen v4.7-unstable
(d2) Get guest memory maps[128] failed. (-38)
(d2) *** HVMLoader bug at e820.c:39
(d2) *** HVMLoader crashed.

The reason is that:
We don't call xc_domain_set_memory_map() in the target machine.
When we create a hvm domain:
libxl__domain_build()
      libxl__build_hvm()
          libxl__arch_domain_construct_memmap()
              xc_domain_set_memory_map()

Should we migrate the guest memory from source machine to target machine?
This bug specifically is because HVMLoader is expected to have run and turned 
the hypercall information in an E820 table in the guest before a migration 
occurs.

Unfortunately, the current codebase is riddled with such assumption and 
expectations (e.g. the HVM save code assumed that FPU context is valid when it 
is saving register state) which is a direct side effect of how it was developed.


Having said all of the above, I agree that your example is a usecase which 
should work.  It is the ultimate test of whether the migration stream contains 
enough information to faithfully reproduce the domain on the far side.  Clearly 
at the moment, this is not the case.

I have an upcoming project to work on the domain memory layout logic, because 
it is unsuitable for a number of XenServer usecases. Part of that will require 
moving it in the migration stream.
I found another migration problem in the test:
If the migration fails, we will resume it in the source side.
But the hvm guest doesn't response any more.

In my test envirionment, the migration always successses, so I

"succeeds"

use a hack way to reproduce it:
1. modify the target xen tools:

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index 258dec4..da95606 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -767,6 +767,8 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void 
*dcs_void,
          goto err;
      }
+ rc = ERROR_FAIL;
+
   err:
      check_all_finished(egc, stream, rc);
2. xl cr hvm_nopv, and wait some time(You can login to the guest)
3. xl migrate hvm_nopv 192.168.3.1

The reason it that:
We create a default ioreq server when we get the hvm param HVM_PARAM_IOREQ_PFN.
It means that: the problem occurs only when the migration fails after we get
the hvm param HVM_PARAM_IOREQ_PFN.

In the function hvm_select_ioreq_server()
If the I/O will be handed by non-default ioreq server, we will return the
non-default ioreq server. In this case, it is handed by qemu.
If the I/O will not be handed by non-default ioreq server, we will return
the default ioreq server. Before migration, we return NULL, and after migration
it is not NULL.
See the caller is hvmemul_do_io():
     case X86EMUL_UNHANDLEABLE:
     {
         struct hvm_ioreq_server *s =
             hvm_select_ioreq_server(curr->domain, &p);

         /* If there is no suitable backing DM, just ignore accesses */
         if ( !s )
         {
             rc = hvm_process_io_intercept(&null_handler, &p);
             vio->io_req.state = STATE_IOREQ_NONE;
         }
         else
         {
             rc = hvm_send_ioreq(s, &p, 0);
             if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
                 vio->io_req.state = STATE_IOREQ_NONE;
             else if ( data_is_addr )
                 rc = X86EMUL_OKAY;
         }
         break;

We send the I/O request to the default I/O request server, but no backing
DM hands it. We wil wait the I/O forever......

Hmm yes.  This needs fixing.

CC'ing Paul who did the ioreq server work.

This bug is caused by the read side effects of HVM_PARAM_IOREQ_PFN. The migration code needs a way of being able to query whether a default ioreq server exists, without creating one.

Can you remember what the justification for the read side effects were? ISTR that it was only for qemu compatibility until the ioreq server work got in upstream. If that was the case, can we drop the read side effects now and m
> 
> Can you give me a fix or a detailed suggestion for this bug?
> 
> 
> Thanks
> Zhang Chen
> 
> > >       default:
> > >           a.value = d->arch.hvm_domain.params[a.index];
> > >           break;
> > > -- 
> > > 2.7.4
> > > 
> > > 
> > > 
> > 
> > .
> > 
> 
> -- 
> Thanks
> zhangchen
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Konrad Rzeszutek Wilk
> Sent: 09 December 2016 16:14
> To: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Changlong Xie <xiecl.fnst@cn.fujitsu.com>; Wei Liu
> <wei.liu2@citrix.com>; Eddie Dong <eddie.dong@intel.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wen Congyang <wencongyang@gmail.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Yang Hongyang
> <imhy.yang@gmail.com>; Xen devel <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH 1/3] Don't create default ioreq server
> 
> .snip..
> > > If you can be more specific about what is broken in COLO we might be
> > > able to devise a fix for you.
> >
> > My workmate have reported this BUG last year:
> > https://lists.xenproject.org/archives/html/xen-devel/2015-
> 12/msg02850.html
> 
> Paul, Andrew was asking about:
> 
> 	This bug is caused by the read side effects of
> HVM_PARAM_IOREQ_PFN. The migration code needs a way of being able to
> query whether a default ioreq server exists, without creating one.
> 
> 	Can you remember what the justification for the read side effects
> were? ISTR that it was only for qemu compatibility until the ioreq server work
> got in upstream. If that was the case, can we drop the read side effects now
> and mandate that all qemus explicitly create their ioreq servers (even if this
> involves creating a default ioreq server for qemu-trad)?
> 

The read side effects are indeed because of the need to support the old qemu interface. If trad were patched then we could at least deprecate the default ioreq server but I'm not sure how long we'd need to leave it in place after that before it was removed. Perhaps it ought to be under a KCONFIG option, since it's also a bit of a security hole.

  Paul

> 
> ?
> 
> Full thread below:
> 
> [Top] [All Lists]
> [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread
> Index]
> Re: [Xen-devel] question about migration
> 
> To: Wen Congyang <wency@xxxxxxxxxxxxxx>
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Date: Tue, 29 Dec 2015 11:24:14 +0000
> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>, xen devel <xen-
> devel@xxxxxxxxxxxxx>
> Delivery-date: Tue, 29 Dec 2015 11:24:33 +0000
> List-id: Xen developer discussion <xen-devel.lists.xen.org>
> On 25/12/2015 01:45, Wen Congyang wrote:
> On 12/24/2015 08:36 PM, Andrew Cooper wrote:
> On 24/12/15 02:29, Wen Congyang wrote:
> Hi Andrew Cooper:
> 
> I rebase the COLO codes to the newest upstream xen, and test it. I found
> a problem in the test, and I can reproduce this problem via the migration.
> 
> How to reproduce:
> 1. xl cr -p hvm_nopv
> 2. xl migrate hvm_nopv 192.168.3.1
> You are the very first person to try a usecase like this.
> 
> It works as much as it does because of your changes to the uncooperative
> HVM
> domain logic.  I have said repeatedly during review, this is not necessarily a
> safe change to make without an in-depth analysis of the knock-on effects; it
> looks as if you have found the first knock-on effect.
> 
> The migration successes, but the vm doesn't run in the target machine.
> You can get the reason from 'xl dmesg':
> (XEN) HVM2 restore: VMCE_VCPU 1
> (XEN) HVM2 restore: TSC_ADJUST 0
> (XEN) HVM2 restore: TSC_ADJUST 1
> (d2) HVM Loader
> (d2) Detected Xen v4.7-unstable
> (d2) Get guest memory maps[128] failed. (-38)
> (d2) *** HVMLoader bug at e820.c:39
> (d2) *** HVMLoader crashed.
> 
> The reason is that:
> We don't call xc_domain_set_memory_map() in the target machine.
> When we create a hvm domain:
> libxl__domain_build()
>       libxl__build_hvm()
>           libxl__arch_domain_construct_memmap()
>               xc_domain_set_memory_map()
> 
> Should we migrate the guest memory from source machine to target
> machine?
> This bug specifically is because HVMLoader is expected to have run and
> turned
> the hypercall information in an E820 table in the guest before a migration
> occurs.
> 
> Unfortunately, the current codebase is riddled with such assumption and
> expectations (e.g. the HVM save code assumed that FPU context is valid
> when it
> is saving register state) which is a direct side effect of how it was developed.
> 
> 
> Having said all of the above, I agree that your example is a usecase which
> should work.  It is the ultimate test of whether the migration stream contains
> enough information to faithfully reproduce the domain on the far side.
> Clearly
> at the moment, this is not the case.
> 
> I have an upcoming project to work on the domain memory layout logic,
> because
> it is unsuitable for a number of XenServer usecases. Part of that will require
> moving it in the migration stream.
> I found another migration problem in the test:
> If the migration fails, we will resume it in the source side.
> But the hvm guest doesn't response any more.
> 
> In my test envirionment, the migration always successses, so I
> 
> "succeeds"
> 
> use a hack way to reproduce it:
> 1. modify the target xen tools:
> 
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> index 258dec4..da95606 100644
> --- a/tools/libxl/libxl_stream_read.c
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -767,6 +767,8 @@ void libxl__xc_domain_restore_done(libxl__egc
> *egc, void
> *dcs_void,
>           goto err;
>       }
> + rc = ERROR_FAIL;
> +
>    err:
>       check_all_finished(egc, stream, rc);
> 2. xl cr hvm_nopv, and wait some time(You can login to the guest)
> 3. xl migrate hvm_nopv 192.168.3.1
> 
> The reason it that:
> We create a default ioreq server when we get the hvm param
> HVM_PARAM_IOREQ_PFN.
> It means that: the problem occurs only when the migration fails after we get
> the hvm param HVM_PARAM_IOREQ_PFN.
> 
> In the function hvm_select_ioreq_server()
> If the I/O will be handed by non-default ioreq server, we will return the
> non-default ioreq server. In this case, it is handed by qemu.
> If the I/O will not be handed by non-default ioreq server, we will return
> the default ioreq server. Before migration, we return NULL, and after
> migration
> it is not NULL.
> See the caller is hvmemul_do_io():
>      case X86EMUL_UNHANDLEABLE:
>      {
>          struct hvm_ioreq_server *s =
>              hvm_select_ioreq_server(curr->domain, &p);
> 
>          /* If there is no suitable backing DM, just ignore accesses */
>          if ( !s )
>          {
>              rc = hvm_process_io_intercept(&null_handler, &p);
>              vio->io_req.state = STATE_IOREQ_NONE;
>          }
>          else
>          {
>              rc = hvm_send_ioreq(s, &p, 0);
>              if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
>                  vio->io_req.state = STATE_IOREQ_NONE;
>              else if ( data_is_addr )
>                  rc = X86EMUL_OKAY;
>          }
>          break;
> 
> We send the I/O request to the default I/O request server, but no backing
> DM hands it. We wil wait the I/O forever......
> 
> Hmm yes.  This needs fixing.
> 
> CC'ing Paul who did the ioreq server work.
> 
> This bug is caused by the read side effects of HVM_PARAM_IOREQ_PFN. The
> migration code needs a way of being able to query whether a default ioreq
> server exists, without creating one.
> 
> Can you remember what the justification for the read side effects were?
> ISTR that it was only for qemu compatibility until the ioreq server work got in
> upstream. If that was the case, can we drop the read side effects now and m
> >
> > Can you give me a fix or a detailed suggestion for this bug?
> >
> >
> > Thanks
> > Zhang Chen
> >
> > > >       default:
> > > >           a.value = d->arch.hvm_domain.params[a.index];
> > > >           break;
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > >
> > >
> > > .
> > >
> >
> > --
> > Thanks
> > zhangchen
> >
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Konrad Rzeszutek Wilk]

On Fri, Dec 09, 2016 at 04:43:58PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> > Konrad Rzeszutek Wilk
> > Sent: 09 December 2016 16:14
> > To: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>; Paul Durrant
> > <Paul.Durrant@citrix.com>
> > Cc: Changlong Xie <xiecl.fnst@cn.fujitsu.com>; Wei Liu
> > <wei.liu2@citrix.com>; Eddie Dong <eddie.dong@intel.com>; Andrew
> > Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> > <Ian.Jackson@citrix.com>; Wen Congyang <wencongyang@gmail.com>; Paul
> > Durrant <Paul.Durrant@citrix.com>; Yang Hongyang
> > <imhy.yang@gmail.com>; Xen devel <xen-devel@lists.xenproject.org>
> > Subject: Re: [Xen-devel] [PATCH 1/3] Don't create default ioreq server
> > 
> > .snip..
> > > > If you can be more specific about what is broken in COLO we might be
> > > > able to devise a fix for you.
> > >
> > > My workmate have reported this BUG last year:
> > > https://lists.xenproject.org/archives/html/xen-devel/2015-
> > 12/msg02850.html
> > 
> > Paul, Andrew was asking about:
> > 
> > 	This bug is caused by the read side effects of
> > HVM_PARAM_IOREQ_PFN. The migration code needs a way of being able to
> > query whether a default ioreq server exists, without creating one.
> > 
> > 	Can you remember what the justification for the read side effects
> > were? ISTR that it was only for qemu compatibility until the ioreq server work
> > got in upstream. If that was the case, can we drop the read side effects now
> > and mandate that all qemus explicitly create their ioreq servers (even if this
> > involves creating a default ioreq server for qemu-trad)?
> > 
> 
> The read side effects are indeed because of the need to support the old qemu interface. If trad were patched then we could at least deprecate the default ioreq server but I'm not sure how long we'd need to leave it in place after that before it was removed. Perhaps it ought to be under a KCONFIG option, since it's also a bit of a security hole.
> 

So.. what can be done about to make COLO work?

>   Paul
> 
> > 
> > ?
> > 
> > Full thread below:
> > 
> > [Top] [All Lists]
> > [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread
> > Index]
> > Re: [Xen-devel] question about migration
> > 
> > To: Wen Congyang <wency@xxxxxxxxxxxxxx>
> > From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Date: Tue, 29 Dec 2015 11:24:14 +0000
> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>, xen devel <xen-
> > devel@xxxxxxxxxxxxx>
> > Delivery-date: Tue, 29 Dec 2015 11:24:33 +0000
> > List-id: Xen developer discussion <xen-devel.lists.xen.org>
> > On 25/12/2015 01:45, Wen Congyang wrote:
> > On 12/24/2015 08:36 PM, Andrew Cooper wrote:
> > On 24/12/15 02:29, Wen Congyang wrote:
> > Hi Andrew Cooper:
> > 
> > I rebase the COLO codes to the newest upstream xen, and test it. I found
> > a problem in the test, and I can reproduce this problem via the migration.
> > 
> > How to reproduce:
> > 1. xl cr -p hvm_nopv
> > 2. xl migrate hvm_nopv 192.168.3.1
> > You are the very first person to try a usecase like this.
> > 
> > It works as much as it does because of your changes to the uncooperative
> > HVM
> > domain logic.  I have said repeatedly during review, this is not necessarily a
> > safe change to make without an in-depth analysis of the knock-on effects; it
> > looks as if you have found the first knock-on effect.
> > 
> > The migration successes, but the vm doesn't run in the target machine.
> > You can get the reason from 'xl dmesg':
> > (XEN) HVM2 restore: VMCE_VCPU 1
> > (XEN) HVM2 restore: TSC_ADJUST 0
> > (XEN) HVM2 restore: TSC_ADJUST 1
> > (d2) HVM Loader
> > (d2) Detected Xen v4.7-unstable
> > (d2) Get guest memory maps[128] failed. (-38)
> > (d2) *** HVMLoader bug at e820.c:39
> > (d2) *** HVMLoader crashed.
> > 
> > The reason is that:
> > We don't call xc_domain_set_memory_map() in the target machine.
> > When we create a hvm domain:
> > libxl__domain_build()
> >       libxl__build_hvm()
> >           libxl__arch_domain_construct_memmap()
> >               xc_domain_set_memory_map()
> > 
> > Should we migrate the guest memory from source machine to target
> > machine?
> > This bug specifically is because HVMLoader is expected to have run and
> > turned
> > the hypercall information in an E820 table in the guest before a migration
> > occurs.
> > 
> > Unfortunately, the current codebase is riddled with such assumption and
> > expectations (e.g. the HVM save code assumed that FPU context is valid
> > when it
> > is saving register state) which is a direct side effect of how it was developed.
> > 
> > 
> > Having said all of the above, I agree that your example is a usecase which
> > should work.  It is the ultimate test of whether the migration stream contains
> > enough information to faithfully reproduce the domain on the far side.
> > Clearly
> > at the moment, this is not the case.
> > 
> > I have an upcoming project to work on the domain memory layout logic,
> > because
> > it is unsuitable for a number of XenServer usecases. Part of that will require
> > moving it in the migration stream.
> > I found another migration problem in the test:
> > If the migration fails, we will resume it in the source side.
> > But the hvm guest doesn't response any more.
> > 
> > In my test envirionment, the migration always successses, so I
> > 
> > "succeeds"
> > 
> > use a hack way to reproduce it:
> > 1. modify the target xen tools:
> > 
> > diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> > index 258dec4..da95606 100644
> > --- a/tools/libxl/libxl_stream_read.c
> > +++ b/tools/libxl/libxl_stream_read.c
> > @@ -767,6 +767,8 @@ void libxl__xc_domain_restore_done(libxl__egc
> > *egc, void
> > *dcs_void,
> >           goto err;
> >       }
> > + rc = ERROR_FAIL;
> > +
> >    err:
> >       check_all_finished(egc, stream, rc);
> > 2. xl cr hvm_nopv, and wait some time(You can login to the guest)
> > 3. xl migrate hvm_nopv 192.168.3.1
> > 
> > The reason it that:
> > We create a default ioreq server when we get the hvm param
> > HVM_PARAM_IOREQ_PFN.
> > It means that: the problem occurs only when the migration fails after we get
> > the hvm param HVM_PARAM_IOREQ_PFN.
> > 
> > In the function hvm_select_ioreq_server()
> > If the I/O will be handed by non-default ioreq server, we will return the
> > non-default ioreq server. In this case, it is handed by qemu.
> > If the I/O will not be handed by non-default ioreq server, we will return
> > the default ioreq server. Before migration, we return NULL, and after
> > migration
> > it is not NULL.
> > See the caller is hvmemul_do_io():
> >      case X86EMUL_UNHANDLEABLE:
> >      {
> >          struct hvm_ioreq_server *s =
> >              hvm_select_ioreq_server(curr->domain, &p);
> > 
> >          /* If there is no suitable backing DM, just ignore accesses */
> >          if ( !s )
> >          {
> >              rc = hvm_process_io_intercept(&null_handler, &p);
> >              vio->io_req.state = STATE_IOREQ_NONE;
> >          }
> >          else
> >          {
> >              rc = hvm_send_ioreq(s, &p, 0);
> >              if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
> >                  vio->io_req.state = STATE_IOREQ_NONE;
> >              else if ( data_is_addr )
> >                  rc = X86EMUL_OKAY;
> >          }
> >          break;
> > 
> > We send the I/O request to the default I/O request server, but no backing
> > DM hands it. We wil wait the I/O forever......
> > 
> > Hmm yes.  This needs fixing.
> > 
> > CC'ing Paul who did the ioreq server work.
> > 
> > This bug is caused by the read side effects of HVM_PARAM_IOREQ_PFN. The
> > migration code needs a way of being able to query whether a default ioreq
> > server exists, without creating one.
> > 
> > Can you remember what the justification for the read side effects were?
> > ISTR that it was only for qemu compatibility until the ioreq server work got in
> > upstream. If that was the case, can we drop the read side effects now and m
> > >
> > > Can you give me a fix or a detailed suggestion for this bug?
> > >
> > >
> > > Thanks
> > > Zhang Chen
> > >
> > > > >       default:
> > > > >           a.value = d->arch.hvm_domain.params[a.index];
> > > > >           break;
> > > > > --
> > > > > 2.7.4
> > > > >
> > > > >
> > > > >
> > > >
> > > > .
> > > >
> > >
> > > --
> > > Thanks
> > > zhangchen
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Paul Durrant]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
[snip]
> > >
> > > 	This bug is caused by the read side effects of
> > > HVM_PARAM_IOREQ_PFN. The migration code needs a way of being
> able to
> > > query whether a default ioreq server exists, without creating one.
> > >
> > > 	Can you remember what the justification for the read side effects
> > > were? ISTR that it was only for qemu compatibility until the ioreq server
> work
> > > got in upstream. If that was the case, can we drop the read side effects
> now
> > > and mandate that all qemus explicitly create their ioreq servers (even if
> this
> > > involves creating a default ioreq server for qemu-trad)?
> > >
> >
> > The read side effects are indeed because of the need to support the old
> qemu interface. If trad were patched then we could at least deprecate the
> default ioreq server but I'm not sure how long we'd need to leave it in place
> after that before it was removed. Perhaps it ought to be under a KCONFIG
> option, since it's also a bit of a security hole.
> >
> 
> So.. what can be done about to make COLO work?
> 

Andrew tells me there is a new boolean in Xen which can be used to determine whether the domain is under construction or not. QEMU should always be kicked off when the domain is under construction so we can limit the read side effect using that Boolean. Thus, when domain-comes along later and needs to query the magic page pfns, we don't magically get a default ioreq server created when we didn't want one. I'll send a patch... should be a one-liner.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] Add Xen colo support for qemu-upstream colo codes

[Wei Liu]

On Thu, Dec 01, 2016 at 01:20:18PM +0000, Wei Liu wrote:
> On Wed, Nov 30, 2016 at 05:47:51PM +0800, Zhang Chen wrote:
> > Because of qemu codes update, we update Xen colo block codes
> > 
> > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> 
> COLO being an experimental feature means that you can change it at will,
> so for both patches:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 

Now that COLO is fixed on HV side, I've pushed these two toolstack
patches.

I adjusted the commit subject and text a bit while committing.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Ian Jackson]

Paul Durrant writes ("RE: [Xen-devel] [PATCH 1/3] Don't create default ioreq server"):
> The read side effects are indeed because of the need to support the
> old qemu interface. If trad were patched then we could at least
> deprecate the default ioreq server but I'm not sure how long we'd
> need to leave it in place after that before it was removed.

(I see further discussion has obviated the need to deal with this,
but:)

You mean to patch trad to make a few more hypercalls to explicitly
create an ioreq server ?  Would it have to register all of its memory
regions ?  If it wouldn't, then the change to qemu-trad wouldn't be
too hard.

qemu-trad is fairly closely tied to the Xen releases so we wouldn't
have to support the old way for very long.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] Don't create default ioreq server

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:ian.jackson@eu.citrix.com]
> Sent: 12 December 2016 18:04
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Zhang Chen
> <zhangchen.fnst@cn.fujitsu.com>; Changlong Xie
> <xiecl.fnst@cn.fujitsu.com>; Wei Liu <wei.liu2@citrix.com>; Eddie Dong
> <eddie.dong@intel.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; Wen Congyang
> <wencongyang@gmail.com>; Yang Hongyang <imhy.yang@gmail.com>; Xen
> devel <xen-devel@lists.xenproject.org>
> Subject: RE: [Xen-devel] [PATCH 1/3] Don't create default ioreq server
> 
> Paul Durrant writes ("RE: [Xen-devel] [PATCH 1/3] Don't create default ioreq
> server"):
> > The read side effects are indeed because of the need to support the
> > old qemu interface. If trad were patched then we could at least
> > deprecate the default ioreq server but I'm not sure how long we'd
> > need to leave it in place after that before it was removed.
> 
> (I see further discussion has obviated the need to deal with this,
> but:)
> 
> You mean to patch trad to make a few more hypercalls to explicitly
> create an ioreq server ?  Would it have to register all of its memory
> regions ?  If it wouldn't, then the change to qemu-trad wouldn't be
> too hard.
> 

No, it would have to explicitly register all IO regions too... and understand new-fangled PCI config space ioreqs.

  Paul

> qemu-trad is fairly closely tied to the Xen releases so we wouldn't
> have to support the old way for very long.
> 
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] Add Xen colo support for qemu-upstream colo codes

[Zhang Chen]

On 12/12/2016 06:37 PM, Wei Liu wrote:
> On Thu, Dec 01, 2016 at 01:20:18PM +0000, Wei Liu wrote:
>> On Wed, Nov 30, 2016 at 05:47:51PM +0800, Zhang Chen wrote:
>>> Because of qemu codes update, we update Xen colo block codes
>>>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> COLO being an experimental feature means that you can change it at will,
>> so for both patches:
>>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>
> Now that COLO is fixed on HV side, I've pushed these two toolstack
> patches.
>
> I adjusted the commit subject and text a bit while committing.

I'm glad to hear it.

Thanks
Zhang Chen

> Wei.
>
>
> .
>

-- 
Thanks
zhangchen




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel