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

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

[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.liu2@citrix.com>
---
Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@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 5555fea..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

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

[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.liu2@citrix.com>
> ---
> Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@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 5555fea..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,

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

[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.liu2@citrix.com>
> > ---
> > Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@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 5555fea..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,

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

[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.campbell@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.liu2@citrix.com>
> > > ---
> > > Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@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 5555fea..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,

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

[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.campbell@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.

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

[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.campbell@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.

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

[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.campbell@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.

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

[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.campbell@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.
>
>

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

[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.campbell@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.
> >
> >