Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-06 Thread Wei Liu 
On Thu, Aug 06, 2015 at 08:45:16AM +0800, Chen, Tiejun wrote:
 On 8/5/2015 7:25 PM, Wei Liu wrote:
 On Wed, Aug 05, 2015 at 12:06:22PM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:58 +0100, Wei Liu wrote:
  On Wed, Aug 05, 2015 at 11:48:55AM +0100, Ian Campbell wrote:
   On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
  This function was called in the wrong place, because both
  libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its
  output.

 What is the effect of this call being in the wrong place?
 Presumably
 one or
 the other of those functions reaches the wrong conclusion?
   
Originally, by the time that function got called, all guest pages
were
already populated.  The end result is E820 map disagrees with what
vNUMA
says and what address ranges memory actually resides, i.e. risk of
guest
accessing region that doesn't have backing pages.
  
   Ouch. This should certainly be explained in the commit message.
  
   With that: Acked-by: Ian Campbell ian.campb...@citrix.com
  
 
  I will post v2 shortly with that information in commit message.
 
   Although perhaps we should wait for confirmation this fix doesn't
   regress
   RMRR somehow?
  
 
  I doubt it will. The code as-is is already broken for RMRR. But let's
  wait till tomorrow for Tiejun to reply.
 
  If he doesn't reply by tomorrow, I suggest we apply v2 first and
  fix up any subsequent issues later.
 
 Agreed.
 
 Actually I want to retract this patch. I confused hvm path with pv path
 and drew my conclusion when looking at both code paths.
 
 In hvm path, neither libxl__vnuma_build_vmemragen_hvm nor xc_hvm_build
 depends on output of libxl__arch_domain_construct_memmap (in fact it
 doesn't change anything). So the code is OK.
 
 In pv path, there is a path which relies on having a valid E820 map
 first, but that path 1) relies on host E820 map; 2) doesn't involve RMRR
 support.
 
 In the end, moving that function call has no effect whatsoever.
 
 Sorry I don't go into this details but seems I have nothing to do about
 this, ultimately. Right?
 

Yes. Nothing to worry about at the moment.

Wei.

 Thanks
 Tiejun
 
 
 Sorry for the noise!
 
 Wei.
 
 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Wei Liu 
This function was called in the wrong place, because both
libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.

Move the call of said function to the right place -- before the other
two functions which reply on its output.

Signed-off-by: Wei Liu wei.l...@citrix.com
---
Cc: Chen, Tiejun tiejun.c...@intel.com
Cc: Ian Campbell ian.campb...@citrix.com
Cc: Ian Jackson ian.jack...@eu.citrix.com

Discovered this issue by code inspection. This issue is not discovered
by osstest because we don't have hardware or test case to test that
code path.

Tiejun, can you confirm this is the right fix? Can you test this
change?
---
 tools/libxl/libxl_dom.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index fea..811f7da 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -960,6 +960,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 goto out;
 }
 
+if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) {
+LOG(ERROR, setting domain memory map failed);
+goto out;
+}
+
 if (info-num_vnuma_nodes != 0) {
 int i;
 
@@ -997,11 +1002,6 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 goto out;
 }
 
-if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) {
-LOG(ERROR, setting domain memory map failed);
-goto out;
-}
-
 ret = hvm_build_set_params(ctx-xch, domid, info, state-store_port,
state-store_mfn, state-console_port,
state-console_mfn, state-store_domid,
-- 
2.4.6


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Ian Campbell 
On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
 This function was called in the wrong place, because both
 libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.

What is the effect of this call being in the wrong place? Presumably one or
the other of those functions reaches the wrong conclusion?

 
 Move the call of said function to the right place -- before the other
 two functions which reply on its output.
 
 Signed-off-by: Wei Liu wei.l...@citrix.com
 ---
 Cc: Chen, Tiejun tiejun.c...@intel.com
 Cc: Ian Campbell ian.campb...@citrix.com
 Cc: Ian Jackson ian.jack...@eu.citrix.com
 
 Discovered this issue by code inspection. This issue is not discovered
 by osstest because we don't have hardware or test case to test that
 code path.
 
 Tiejun, can you confirm this is the right fix? Can you test this
 change?
 ---
  tools/libxl/libxl_dom.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)
 
 diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
 index fea..811f7da 100644
 --- a/tools/libxl/libxl_dom.c
 +++ b/tools/libxl/libxl_dom.c
 @@ -960,6 +960,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
  goto out;
  }
  
 +if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) 
 {
 +LOG(ERROR, setting domain memory map failed);
 +goto out;
 +}
 +
  if (info-num_vnuma_nodes != 0) {
  int i;
  
 @@ -997,11 +1002,6 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
  goto out;
  }
  
 -if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) 
 {
 -LOG(ERROR, setting domain memory map failed);
 -goto out;
 -}
 -
  ret = hvm_build_set_params(ctx-xch, domid, info, state-store_port,
 state-store_mfn, state-console_port,
 state-console_mfn, state-store_domid,

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Wei Liu 
On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
  This function was called in the wrong place, because both
  libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.
 
 What is the effect of this call being in the wrong place? Presumably one or
 the other of those functions reaches the wrong conclusion?

Originally, by the time that function got called, all guest pages were
already populated.  The end result is E820 map disagrees with what vNUMA
says and what address ranges memory actually resides, i.e. risk of guest
accessing region that doesn't have backing pages.

 
  
  Move the call of said function to the right place -- before the other
  two functions which reply on its output.
  
  Signed-off-by: Wei Liu wei.l...@citrix.com
  ---
  Cc: Chen, Tiejun tiejun.c...@intel.com
  Cc: Ian Campbell ian.campb...@citrix.com
  Cc: Ian Jackson ian.jack...@eu.citrix.com
  
  Discovered this issue by code inspection. This issue is not discovered
  by osstest because we don't have hardware or test case to test that
  code path.
  
  Tiejun, can you confirm this is the right fix? Can you test this
  change?
  ---
   tools/libxl/libxl_dom.c | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)
  
  diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
  index fea..811f7da 100644
  --- a/tools/libxl/libxl_dom.c
  +++ b/tools/libxl/libxl_dom.c
  @@ -960,6 +960,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
   goto out;
   }
   
  +if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) 
  {
  +LOG(ERROR, setting domain memory map failed);
  +goto out;
  +}
  +
   if (info-num_vnuma_nodes != 0) {
   int i;
   
  @@ -997,11 +1002,6 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
   goto out;
   }
   
  -if (libxl__arch_domain_construct_memmap(gc, d_config, domid, args)) 
  {
  -LOG(ERROR, setting domain memory map failed);
  -goto out;
  -}
  -
   ret = hvm_build_set_params(ctx-xch, domid, info, state-store_port,
  state-store_mfn, state-console_port,
  state-console_mfn, state-store_domid,

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Ian Campbell 
On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
 On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
  On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
   This function was called in the wrong place, because both
   libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.
  
  What is the effect of this call being in the wrong place? Presumably 
  one or
  the other of those functions reaches the wrong conclusion?
 
 Originally, by the time that function got called, all guest pages were
 already populated.  The end result is E820 map disagrees with what vNUMA
 says and what address ranges memory actually resides, i.e. risk of guest
 accessing region that doesn't have backing pages.

Ouch. This should certainly be explained in the commit message.

With that: Acked-by: Ian Campbell ian.campb...@citrix.com

Although perhaps we should wait for confirmation this fix doesn't regress
RMRR somehow?

 Move the call of said function to the right place -- before the other
   two functions which reply on its output.
   
   Signed-off-by: Wei Liu wei.l...@citrix.com
   ---
   Cc: Chen, Tiejun tiejun.c...@intel.com
   Cc: Ian Campbell ian.campb...@citrix.com
   Cc: Ian Jackson ian.jack...@eu.citrix.com
   
   Discovered this issue by code inspection. This issue is not 
   discovered
   by osstest because we don't have hardware or test case to test that
   code path.
   
   Tiejun, can you confirm this is the right fix? Can you test this
   change?
   ---
tools/libxl/libxl_dom.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
   
   diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
   index fea..811f7da 100644
   --- a/tools/libxl/libxl_dom.c
   +++ b/tools/libxl/libxl_dom.c
   @@ -960,6 +960,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t 
   domid,
goto out;
}

   +if (libxl__arch_domain_construct_memmap(gc, d_config, domid, 
   args)) 
   {
   +LOG(ERROR, setting domain memory map failed);
   +goto out;
   +}
   +
if (info-num_vnuma_nodes != 0) {
int i;

   @@ -997,11 +1002,6 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t 
   domid,
goto out;
}

   -if (libxl__arch_domain_construct_memmap(gc, d_config, domid, 
   args)) 
   {
   -LOG(ERROR, setting domain memory map failed);
   -goto out;
   -}
   -
ret = hvm_build_set_params(ctx-xch, domid, info, state
   -store_port,
   state-store_mfn, state
   -console_port,
   state-console_mfn, state
   -store_domid,

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Wei Liu 
On Wed, Aug 05, 2015 at 11:48:55AM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
  On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
   On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
This function was called in the wrong place, because both
libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.
   
   What is the effect of this call being in the wrong place? Presumably 
   one or
   the other of those functions reaches the wrong conclusion?
  
  Originally, by the time that function got called, all guest pages were
  already populated.  The end result is E820 map disagrees with what vNUMA
  says and what address ranges memory actually resides, i.e. risk of guest
  accessing region that doesn't have backing pages.
 
 Ouch. This should certainly be explained in the commit message.
 
 With that: Acked-by: Ian Campbell ian.campb...@citrix.com
 

I will post v2 shortly with that information in commit message.

 Although perhaps we should wait for confirmation this fix doesn't regress
 RMRR somehow?
 

I doubt it will. The code as-is is already broken for RMRR. But let's
wait till tomorrow for Tiejun to reply.

If he doesn't reply by tomorrow, I suggest we apply v2 first and
fix up any subsequent issues later.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Ian Campbell 
On Wed, 2015-08-05 at 11:58 +0100, Wei Liu wrote:
 On Wed, Aug 05, 2015 at 11:48:55AM +0100, Ian Campbell wrote:
  On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
   On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
 This function was called in the wrong place, because both
 libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its 
 output.

What is the effect of this call being in the wrong place? 
Presumably 
one or
the other of those functions reaches the wrong conclusion?
   
   Originally, by the time that function got called, all guest pages 
   were
   already populated.  The end result is E820 map disagrees with what 
   vNUMA
   says and what address ranges memory actually resides, i.e. risk of 
   guest
   accessing region that doesn't have backing pages.
  
  Ouch. This should certainly be explained in the commit message.
  
  With that: Acked-by: Ian Campbell ian.campb...@citrix.com
  
 
 I will post v2 shortly with that information in commit message.
 
  Although perhaps we should wait for confirmation this fix doesn't 
  regress
  RMRR somehow?
  
 
 I doubt it will. The code as-is is already broken for RMRR. But let's
 wait till tomorrow for Tiejun to reply.
 
 If he doesn't reply by tomorrow, I suggest we apply v2 first and
 fix up any subsequent issues later.

Agreed.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Wei Liu 
On Wed, Aug 05, 2015 at 12:06:22PM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:58 +0100, Wei Liu wrote:
  On Wed, Aug 05, 2015 at 11:48:55AM +0100, Ian Campbell wrote:
   On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
 On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
  This function was called in the wrong place, because both
  libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its 
  output.
 
 What is the effect of this call being in the wrong place? 
 Presumably 
 one or
 the other of those functions reaches the wrong conclusion?

Originally, by the time that function got called, all guest pages 
were
already populated.  The end result is E820 map disagrees with what 
vNUMA
says and what address ranges memory actually resides, i.e. risk of 
guest
accessing region that doesn't have backing pages.
   
   Ouch. This should certainly be explained in the commit message.
   
   With that: Acked-by: Ian Campbell ian.campb...@citrix.com
   
  
  I will post v2 shortly with that information in commit message.
  
   Although perhaps we should wait for confirmation this fix doesn't 
   regress
   RMRR somehow?
   
  
  I doubt it will. The code as-is is already broken for RMRR. But let's
  wait till tomorrow for Tiejun to reply.
  
  If he doesn't reply by tomorrow, I suggest we apply v2 first and
  fix up any subsequent issues later.
 
 Agreed.

Actually I want to retract this patch. I confused hvm path with pv path
and drew my conclusion when looking at both code paths.

In hvm path, neither libxl__vnuma_build_vmemragen_hvm nor xc_hvm_build
depends on output of libxl__arch_domain_construct_memmap (in fact it
doesn't change anything). So the code is OK.

In pv path, there is a path which relies on having a valid E820 map
first, but that path 1) relies on host E820 map; 2) doesn't involve RMRR
support.

In the end, moving that function call has no effect whatsoever.

Sorry for the noise!

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

2015-08-05 Thread Chen, Tiejun 

On 8/5/2015 7:25 PM, Wei Liu wrote:

On Wed, Aug 05, 2015 at 12:06:22PM +0100, Ian Campbell wrote:

On Wed, 2015-08-05 at 11:58 +0100, Wei Liu wrote:
 On Wed, Aug 05, 2015 at 11:48:55AM +0100, Ian Campbell wrote:
  On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
   On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
 This function was called in the wrong place, because both
 libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its
 output.
   
What is the effect of this call being in the wrong place?
Presumably
one or
the other of those functions reaches the wrong conclusion?
  
   Originally, by the time that function got called, all guest pages
   were
   already populated.  The end result is E820 map disagrees with what
   vNUMA
   says and what address ranges memory actually resides, i.e. risk of
   guest
   accessing region that doesn't have backing pages.
 
  Ouch. This should certainly be explained in the commit message.
 
  With that: Acked-by: Ian Campbell ian.campb...@citrix.com
 

 I will post v2 shortly with that information in commit message.

  Although perhaps we should wait for confirmation this fix doesn't
  regress
  RMRR somehow?
 

 I doubt it will. The code as-is is already broken for RMRR. But let's
 wait till tomorrow for Tiejun to reply.

 If he doesn't reply by tomorrow, I suggest we apply v2 first and
 fix up any subsequent issues later.

Agreed.


Actually I want to retract this patch. I confused hvm path with pv path
and drew my conclusion when looking at both code paths.

In hvm path, neither libxl__vnuma_build_vmemragen_hvm nor xc_hvm_build
depends on output of libxl__arch_domain_construct_memmap (in fact it
doesn't change anything). So the code is OK.

In pv path, there is a path which relies on having a valid E820 map
first, but that path 1) relies on host E820 map; 2) doesn't involve RMRR
support.

In the end, moving that function call has no effect whatsoever.


Sorry I don't go into this details but seems I have nothing to do about 
this, ultimately. Right?


Thanks
Tiejun



Sorry for the noise!

Wei.




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel