All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libxc: refactor memory allocation functions
@ 2015-12-01 11:39 Wei Liu
  2015-12-01 11:45 ` Juergen Gross
  2015-12-01 11:58 ` Ian Campbell
  0 siblings, 2 replies; 9+ messages in thread
From: Wei Liu @ 2015-12-01 11:39 UTC (permalink / raw)
  To: Xen-devel; +Cc: Juergen Gross, Ian Jackson, Wei Liu, Ian Campbell

There were some problems with the original memory allocation functions:
1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
   xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
2. xc_dom_alloc_pad didn't call dom->allocate.

Refactor the code so that:
1. xc_dom_alloc_{segment,pad,page} end up calling
   xc_dom_chk_alloc_pages.
2. xc_dom_chk_alloc_pages calls dom->allocate.

This way we avoid scattering dom->allocate over multiple locations and
open-coding.

Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
an invalid pfn when xc_dom_chk_alloc_pages fails.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Juergen Gross <jgross@suse.com>

We don't have INVALID_PFN, maybe we need one?

Tested with Jessie with 64 bit pvgrub, Wheezy with 64 bit pvgrub, Wheezy pv
guest, Wheezy HVM guest, Wheezy HVM with qemu stubdom.

 tools/libxc/include/xc_dom.h |  2 +-
 tools/libxc/xc_dom_core.c    | 16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 2176216..2d0de8c 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -350,7 +350,7 @@ char *xc_dom_strdup(struct xc_dom_image *dom, const char *str);
 
 /* --- alloc memory pool ------------------------------------------- */
 
-int xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
+xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
 int xc_dom_alloc_segment(struct xc_dom_image *dom,
                          struct xc_dom_seg *seg, char *name,
                          xen_vaddr_t start, xen_vaddr_t size);
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 5d6c3ba..8967970 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -555,6 +555,9 @@ static int xc_dom_chk_alloc_pages(struct xc_dom_image *dom, char *name,
     dom->pfn_alloc_end += pages;
     dom->virt_alloc_end += pages * page_size;
 
+    if ( dom->allocate )
+        dom->allocate(dom);
+
     return 0;
 }
 
@@ -602,9 +605,6 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
     if ( xc_dom_chk_alloc_pages(dom, name, pages) )
         return -1;
 
-    if (dom->allocate)
-        dom->allocate(dom);
-
     /* map and clear pages */
     ptr = xc_dom_seg_to_ptr(dom, seg);
     if ( ptr == NULL )
@@ -621,18 +621,16 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
     return 0;
 }
 
-int xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
+xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
 {
-    unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
     xen_vaddr_t start;
     xen_pfn_t pfn;
 
     start = dom->virt_alloc_end;
     pfn = dom->pfn_alloc_end - dom->rambase_pfn;
-    dom->virt_alloc_end += page_size;
-    dom->pfn_alloc_end++;
-    if ( dom->allocate )
-        dom->allocate(dom);
+
+    if ( xc_dom_chk_alloc_pages(dom, name, 1) )
+        return (xen_pfn_t)-1;
 
     DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " (pfn 0x%" PRIpfn ")",
               __FUNCTION__, name, start, pfn);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 11:39 [PATCH] libxc: refactor memory allocation functions Wei Liu
@ 2015-12-01 11:45 ` Juergen Gross
  2015-12-01 12:38   ` Ian Campbell
  2015-12-01 11:58 ` Ian Campbell
  1 sibling, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2015-12-01 11:45 UTC (permalink / raw)
  To: Wei Liu, Xen-devel; +Cc: Ian Jackson, Ian Campbell

On 01/12/15 12:39, Wei Liu wrote:
> There were some problems with the original memory allocation functions:
> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
> 2. xc_dom_alloc_pad didn't call dom->allocate.
> 
> Refactor the code so that:
> 1. xc_dom_alloc_{segment,pad,page} end up calling
>    xc_dom_chk_alloc_pages.
> 2. xc_dom_chk_alloc_pages calls dom->allocate.
> 
> This way we avoid scattering dom->allocate over multiple locations and
> open-coding.
> 
> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
> an invalid pfn when xc_dom_chk_alloc_pages fails.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> 
> We don't have INVALID_PFN, maybe we need one?
> 
> Tested with Jessie with 64 bit pvgrub, Wheezy with 64 bit pvgrub, Wheezy pv
> guest, Wheezy HVM guest, Wheezy HVM with qemu stubdom.
> 
>  tools/libxc/include/xc_dom.h |  2 +-
>  tools/libxc/xc_dom_core.c    | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 2176216..2d0de8c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -350,7 +350,7 @@ char *xc_dom_strdup(struct xc_dom_image *dom, const char *str);
>  
>  /* --- alloc memory pool ------------------------------------------- */
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
>  int xc_dom_alloc_segment(struct xc_dom_image *dom,
>                           struct xc_dom_seg *seg, char *name,
>                           xen_vaddr_t start, xen_vaddr_t size);
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 5d6c3ba..8967970 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -555,6 +555,9 @@ static int xc_dom_chk_alloc_pages(struct xc_dom_image *dom, char *name,
>      dom->pfn_alloc_end += pages;
>      dom->virt_alloc_end += pages * page_size;
>  
> +    if ( dom->allocate )
> +        dom->allocate(dom);
> +
>      return 0;
>  }
>  
> @@ -602,9 +605,6 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      if ( xc_dom_chk_alloc_pages(dom, name, pages) )
>          return -1;
>  
> -    if (dom->allocate)
> -        dom->allocate(dom);
> -
>      /* map and clear pages */
>      ptr = xc_dom_seg_to_ptr(dom, seg);
>      if ( ptr == NULL )
> @@ -621,18 +621,16 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      return 0;
>  }
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
>  {
> -    unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
>      xen_vaddr_t start;
>      xen_pfn_t pfn;
>  
>      start = dom->virt_alloc_end;
>      pfn = dom->pfn_alloc_end - dom->rambase_pfn;
> -    dom->virt_alloc_end += page_size;
> -    dom->pfn_alloc_end++;
> -    if ( dom->allocate )
> -        dom->allocate(dom);
> +
> +    if ( xc_dom_chk_alloc_pages(dom, name, 1) )
> +        return (xen_pfn_t)-1;
>  
>      DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " (pfn 0x%" PRIpfn ")",
>                __FUNCTION__, name, start, pfn);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 11:39 [PATCH] libxc: refactor memory allocation functions Wei Liu
  2015-12-01 11:45 ` Juergen Gross
@ 2015-12-01 11:58 ` Ian Campbell
  2015-12-01 12:03   ` Wei Liu
  2015-12-01 12:04   ` Juergen Gross
  1 sibling, 2 replies; 9+ messages in thread
From: Ian Campbell @ 2015-12-01 11:58 UTC (permalink / raw)
  To: Wei Liu, Xen-devel; +Cc: Juergen Gross, Ian Jackson

On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
> There were some problems with the original memory allocation functions:
> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
> 2. xc_dom_alloc_pad didn't call dom->allocate.
> 
> Refactor the code so that:
> 1. xc_dom_alloc_{segment,pad,page} end up calling
>    xc_dom_chk_alloc_pages.
> 2. xc_dom_chk_alloc_pages calls dom->allocate.
> 
> This way we avoid scattering dom->allocate over multiple locations and
> open-coding.
> 
> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
> an invalid pfn when xc_dom_chk_alloc_pages fails.

Given this presumably the handful of callers ought to gain some error
handling in a followup patch?

xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
with that.

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

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

> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> 
> We don't have INVALID_PFN, maybe we need one?

We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
bill (or if not how it is distinct from the former).

> 
> Tested with Jessie with 64 bit pvgrub, Wheezy with 64 bit pvgrub, Wheezy
> pv
> guest, Wheezy HVM guest, Wheezy HVM with qemu stubdom.

Also by me in my previously failing environment =>

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

I think there are two more fixes in this space:
    libxc: correct domain builder for 64 bit guest with 32 bit tools
    libxc: use correct return type for do_memory_op()

I intend to pick up all three for commit in a moment, prod me if there are
others.

> 
>  tools/libxc/include/xc_dom.h |  2 +-
>  tools/libxc/xc_dom_core.c    | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 2176216..2d0de8c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -350,7 +350,7 @@ char *xc_dom_strdup(struct xc_dom_image *dom, const
> char *str);
>  
>  /* --- alloc memory pool ------------------------------------------- */
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
>  int xc_dom_alloc_segment(struct xc_dom_image *dom,
>                           struct xc_dom_seg *seg, char *name,
>                           xen_vaddr_t start, xen_vaddr_t size);
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 5d6c3ba..8967970 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -555,6 +555,9 @@ static int xc_dom_chk_alloc_pages(struct xc_dom_image
> *dom, char *name,
>      dom->pfn_alloc_end += pages;
>      dom->virt_alloc_end += pages * page_size;
>  
> +    if ( dom->allocate )
> +        dom->allocate(dom);
> +
>      return 0;
>  }
>  
> @@ -602,9 +605,6 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      if ( xc_dom_chk_alloc_pages(dom, name, pages) )
>          return -1;
>  
> -    if (dom->allocate)
> -        dom->allocate(dom);
> -
>      /* map and clear pages */
>      ptr = xc_dom_seg_to_ptr(dom, seg);
>      if ( ptr == NULL )
> @@ -621,18 +621,16 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      return 0;
>  }
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
>  {
> -    unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
>      xen_vaddr_t start;
>      xen_pfn_t pfn;
>  
>      start = dom->virt_alloc_end;
>      pfn = dom->pfn_alloc_end - dom->rambase_pfn;
> -    dom->virt_alloc_end += page_size;
> -    dom->pfn_alloc_end++;
> -    if ( dom->allocate )
> -        dom->allocate(dom);
> +
> +    if ( xc_dom_chk_alloc_pages(dom, name, 1) )
> +        return (xen_pfn_t)-1;
>  
>      DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " (pfn 0x%" PRIpfn ")",
>                __FUNCTION__, name, start, pfn);

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 11:58 ` Ian Campbell
@ 2015-12-01 12:03   ` Wei Liu
  2015-12-01 12:14     ` Ian Campbell
  2015-12-01 12:04   ` Juergen Gross
  1 sibling, 1 reply; 9+ messages in thread
From: Wei Liu @ 2015-12-01 12:03 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Juergen Gross, Xen-devel, Wei Liu, Ian Jackson

On Tue, Dec 01, 2015 at 11:58:44AM +0000, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
> > There were some problems with the original memory allocation functions:
> > 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
> >    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
> > 2. xc_dom_alloc_pad didn't call dom->allocate.
> > 
> > Refactor the code so that:
> > 1. xc_dom_alloc_{segment,pad,page} end up calling
> >    xc_dom_chk_alloc_pages.
> > 2. xc_dom_chk_alloc_pages calls dom->allocate.
> > 
> > This way we avoid scattering dom->allocate over multiple locations and
> > open-coding.
> > 
> > Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
> > an invalid pfn when xc_dom_chk_alloc_pages fails.
> 
> Given this presumably the handful of callers ought to gain some error
> handling in a followup patch?
> 

Yes, that's for sure.

> xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
> with that.
> 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > ---
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > 
> > We don't have INVALID_PFN, maybe we need one?
> 
> We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
> bill (or if not how it is distinct from the former).
> 

Yeah, I'm aware of those but they don't seem to fit because they are
used in MFN space.

> > 
> > Tested with Jessie with 64 bit pvgrub, Wheezy with 64 bit pvgrub, Wheezy
> > pv
> > guest, Wheezy HVM guest, Wheezy HVM with qemu stubdom.
> 
> Also by me in my previously failing environment =>
> 
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I think there are two more fixes in this space:
>     libxc: correct domain builder for 64 bit guest with 32 bit tools
>     libxc: use correct return type for do_memory_op()
> 
> I intend to pick up all three for commit in a moment, prod me if there are
> others.
> 

This one and the other two are all that I'm aware of at the moment.

Wei.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 11:58 ` Ian Campbell
  2015-12-01 12:03   ` Wei Liu
@ 2015-12-01 12:04   ` Juergen Gross
  2015-12-01 12:12     ` Wei Liu
  1 sibling, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2015-12-01 12:04 UTC (permalink / raw)
  To: Ian Campbell, Wei Liu, Xen-devel; +Cc: Ian Jackson

On 01/12/15 12:58, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
>> There were some problems with the original memory allocation functions:
>> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
>>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
>> 2. xc_dom_alloc_pad didn't call dom->allocate.
>>
>> Refactor the code so that:
>> 1. xc_dom_alloc_{segment,pad,page} end up calling
>>    xc_dom_chk_alloc_pages.
>> 2. xc_dom_chk_alloc_pages calls dom->allocate.
>>
>> This way we avoid scattering dom->allocate over multiple locations and
>> open-coding.
>>
>> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
>> an invalid pfn when xc_dom_chk_alloc_pages fails.
> 
> Given this presumably the handful of callers ought to gain some error
> handling in a followup patch?

I could do that. Wei?

> 
> xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
> with that.
> 
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>> ---
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Juergen Gross <jgross@suse.com>
>>
>> We don't have INVALID_PFN, maybe we need one?
> 
> We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
> bill (or if not how it is distinct from the former).

What about merging all of them into INVALID_FRAME?

I can create a patch(-series).


Juergen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 12:04   ` Juergen Gross
@ 2015-12-01 12:12     ` Wei Liu
  2015-12-01 14:08       ` Juergen Gross
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2015-12-01 12:12 UTC (permalink / raw)
  To: Juergen Gross; +Cc: Ian Jackson, Xen-devel, Wei Liu, Ian Campbell

On Tue, Dec 01, 2015 at 01:04:14PM +0100, Juergen Gross wrote:
> On 01/12/15 12:58, Ian Campbell wrote:
> > On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
> >> There were some problems with the original memory allocation functions:
> >> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
> >>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
> >> 2. xc_dom_alloc_pad didn't call dom->allocate.
> >>
> >> Refactor the code so that:
> >> 1. xc_dom_alloc_{segment,pad,page} end up calling
> >>    xc_dom_chk_alloc_pages.
> >> 2. xc_dom_chk_alloc_pages calls dom->allocate.
> >>
> >> This way we avoid scattering dom->allocate over multiple locations and
> >> open-coding.
> >>
> >> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
> >> an invalid pfn when xc_dom_chk_alloc_pages fails.
> > 
> > Given this presumably the handful of callers ought to gain some error
> > handling in a followup patch?
> 
> I could do that. Wei?
> 

You're welcome to pick that up.

> > 
> > xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
> > with that.
> > 
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> >> ---
> >> Cc: Ian Campbell <ian.campbell@citrix.com>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Juergen Gross <jgross@suse.com>
> >>
> >> We don't have INVALID_PFN, maybe we need one?
> > 
> > We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
> > bill (or if not how it is distinct from the former).
> 
> What about merging all of them into INVALID_FRAME?
> 

Note that INVALID_MFN is in public header (xenctrl.h) while
INVALID_P2M_ENTRY is not.

In fact (xen_pfn_t)-1 is part of guest ABI so we should properly export
and document it. The current situation is not ideal. Not sure how much
yak shaving is required though.

Wei.

> I can create a patch(-series).
> 
> 
> Juergen
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 12:03   ` Wei Liu
@ 2015-12-01 12:14     ` Ian Campbell
  0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-12-01 12:14 UTC (permalink / raw)
  To: Wei Liu; +Cc: Juergen Gross, Xen-devel, Ian Jackson

On Tue, 2015-12-01 at 12:03 +0000, Wei Liu wrote:
> 
> > > We don't have INVALID_PFN, maybe we need one?
> > 
> > We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
> > bill (or if not how it is distinct from the former).
> > 
> 
> Yeah, I'm aware of those but they don't seem to fit because they are
> used in MFN space.

Right, I wasn't sure about the latter, since given a P2M_ENTRY is an MFN I
then don't see how/why it is distinct from INVALID_MFN. Perhaps I'm missing
something (guest p2m width perhaps?), or maybe it's just historical
baggage.

Ian.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 11:45 ` Juergen Gross
@ 2015-12-01 12:38   ` Ian Campbell
  0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-12-01 12:38 UTC (permalink / raw)
  To: Juergen Gross, Wei Liu, Xen-devel; +Cc: Ian Jackson

On Tue, 2015-12-01 at 12:45 +0100, Juergen Gross wrote:
> On 01/12/15 12:39, Wei Liu wrote:
> > There were some problems with the original memory allocation functions:
> > 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
> >    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded
> > everything.
> > 2. xc_dom_alloc_pad didn't call dom->allocate.
> > 
> > Refactor the code so that:
> > 1. xc_dom_alloc_{segment,pad,page} end up calling
> >    xc_dom_chk_alloc_pages.
> > 2. xc_dom_chk_alloc_pages calls dom->allocate.
> > 
> > This way we avoid scattering dom->allocate over multiple locations and
> > open-coding.
> > 
> > Also change the return type of xc_dom_alloc_page to xen_pfn_t and
> > return
> > an invalid pfn when xc_dom_chk_alloc_pages fails.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Applied, thanks.



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libxc: refactor memory allocation functions
  2015-12-01 12:12     ` Wei Liu
@ 2015-12-01 14:08       ` Juergen Gross
  0 siblings, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2015-12-01 14:08 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Ian Campbell

On 01/12/15 13:12, Wei Liu wrote:
> On Tue, Dec 01, 2015 at 01:04:14PM +0100, Juergen Gross wrote:
>> On 01/12/15 12:58, Ian Campbell wrote:
>>> On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
>>>> There were some problems with the original memory allocation functions:
>>>> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
>>>>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
>>>> 2. xc_dom_alloc_pad didn't call dom->allocate.
>>>>
>>>> Refactor the code so that:
>>>> 1. xc_dom_alloc_{segment,pad,page} end up calling
>>>>    xc_dom_chk_alloc_pages.
>>>> 2. xc_dom_chk_alloc_pages calls dom->allocate.
>>>>
>>>> This way we avoid scattering dom->allocate over multiple locations and
>>>> open-coding.
>>>>
>>>> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
>>>> an invalid pfn when xc_dom_chk_alloc_pages fails.
>>>
>>> Given this presumably the handful of callers ought to gain some error
>>> handling in a followup patch?
>>
>> I could do that. Wei?
>>
> 
> You're welcome to pick that up.
> 
>>>
>>> xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
>>> with that.
>>>
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>>> ---
>>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>
>>>> We don't have INVALID_PFN, maybe we need one?
>>>
>>> We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
>>> bill (or if not how it is distinct from the former).
>>
>> What about merging all of them into INVALID_FRAME?
>>
> 
> Note that INVALID_MFN is in public header (xenctrl.h) while
> INVALID_P2M_ENTRY is not.

INVALID_P2M_ENTRY in libxc is defined as (xen_pfn_t)-1. I think it
would make sense to modify that to INVALID_PFN.

> In fact (xen_pfn_t)-1 is part of guest ABI so we should properly export
> and document it. The current situation is not ideal. Not sure how much
> yak shaving is required though.

As this relates to the return value checking of the domain builder
memory allocating I'll have a try, too.


Juergen

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-12-01 14:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-01 11:39 [PATCH] libxc: refactor memory allocation functions Wei Liu
2015-12-01 11:45 ` Juergen Gross
2015-12-01 12:38   ` Ian Campbell
2015-12-01 11:58 ` Ian Campbell
2015-12-01 12:03   ` Wei Liu
2015-12-01 12:14     ` Ian Campbell
2015-12-01 12:04   ` Juergen Gross
2015-12-01 12:12     ` Wei Liu
2015-12-01 14:08       ` Juergen Gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.