* [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.