Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
On Feb 22, 2007, at 5:07 PM, Ryan Harper wrote: * Hollis Blanchard <[EMAIL PROTECTED]> [2007-02-22 15:01]: On Wed, 2007-02-21 at 18:17 -0500, Ryan Harper wrote: 4 files changed, 72 insertions(+) xen/arch/powerpc/domain.c| 60 ++ xen/arch/powerpc/domain_build.c |5 +++ xen/include/asm-powerpc/domain.h |3 + xen/include/asm-powerpc/shadow.h |4 ++ # HG changeset patch # User Ryan Harper <[EMAIL PROTECTED]> # Date 1172103252 21600 # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502 # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28 [PATCH] xen: implement guest_physmap_max_mem for ppc Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain.c --- a/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 +++ b/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 @@ -33,6 +33,7 @@ #include #include #include +#include #include "rtas.h" #include "exceptions.h" @@ -347,3 +348,62 @@ void idle_loop(void) do_softirq(); } } + +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) Could you rename "new_max" to "new_max_pages" so we can keep the units straight? (I know they use "new_max" in the XEN_DOMCTL_max_mem handler.) Yep. +{ +ulong *p2m_array = NULL; +ulong *p2m_old = NULL; +ulong p2m_pages; +ulong copy_end = 0; + +/* we don't handle shrinking max_pages */ +if ( new_max < d->max_pages ) +{ +printk("Can't shrink %d max_mem\n", d->domain_id); +return -EINVAL; +} We won't be called in this case, so this test can be removed. OK. +/* our work is done here */ +if ( new_max == d->max_pages ) +return 0; + +/* check new_max pages is 16MiB aligned */ +if ( new_max & ((1<<12)-1) ) +{ +printk("Must be 16MiB aligned increments\n"); +return -EACCES; +} The 16MB thing is because the 970's large page size is 16MB, and the kernel uses large pages to map its text. That said, I don't see why this should be enforced by Xen when setting max_mem (if ever). Stylistically, I also object to the literals used here. Great. I told myself that I was going to replace the literals since I was guessing that there might be a common check like cpu_aligned(). +/* make a p2m array of new_max mfns */ +p2m_pages = (new_max * sizeof(ulong)) >> PAGE_SHIFT; +/* XXX: could use domheap anonymously */ +p2m_array = alloc_xenheap_pages(get_order_from_pages (p2m_pages)); It is a shame that these pages will be naturally aligned, which is unecessary. +if ( p2m_array == NULL ) +return -ENOMEM; I think the Xen heap is on the small side. Do you have an idea of how much memory we have available? I suppose we can change it later if we exhaust the heap. In the later "heap mail-thread" I suggest that you add nr_pages*sizeof (p2m_pages[]) to the xenheap calculation, that will make sure the xenheap is a good size. IIRC, when dom0 boots with 192MB (the default) I usually see ~19MB of heap available in the boot messages on js20. Looking at js21, I see: (XEN) Xen Heap: 135MiB (138548KiB) RMA different size on js21? We had talked about using ints for the p2m array, since that would only limit us to 44 bits of physical memory. Did you decide to use longs instead? No, just being lazy. I wanted to get the patches out for comment ASAP but I forgot to note that we were going to use u32 in the mails. I still plan to switch p2m array to smaller size. +/* copy old mappings into new array if any */ +if ( d->arch.p2m != NULL ) +{ +/* mark where the copy will stop (which page) */ +copy_end = d->max_pages; + +/* memcpy takes size in bytes */ +memcpy(p2m_array, d->arch.p2m, (d->max_pages * sizeof (ulong))); + +/* keep a pointer to the old array */ +p2m_old = d->arch.p2m; +} This memcpy could be pretty slow; might be better if we could make this a continuation some day. If you agree, could you add a comment to that effect? Good point. And in that case it should be a copy_page() loop. -JX ___ Xen-ppc-devel mailing list Xen-ppc-devel@lists.xensource.com http://lists.xensource.com/xen-ppc-devel
Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
* Hollis Blanchard <[EMAIL PROTECTED]> [2007-02-22 15:01]: > On Wed, 2007-02-21 at 18:17 -0500, Ryan Harper wrote: > > 4 files changed, 72 insertions(+) > > xen/arch/powerpc/domain.c| 60 > > ++ > > xen/arch/powerpc/domain_build.c |5 +++ > > xen/include/asm-powerpc/domain.h |3 + > > xen/include/asm-powerpc/shadow.h |4 ++ > > > > > > # HG changeset patch > > # User Ryan Harper <[EMAIL PROTECTED]> > > # Date 1172103252 21600 > > # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502 > > # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28 > > [PATCH] xen: implement guest_physmap_max_mem for ppc > > > > Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> > > > > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain.c > > --- a/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 > > +++ b/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 > > @@ -33,6 +33,7 @@ > > #include > > #include > > #include > > +#include > > #include "rtas.h" > > #include "exceptions.h" > > > > @@ -347,3 +348,62 @@ void idle_loop(void) > > do_softirq(); > > } > > } > > + > > +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) > > Could you rename "new_max" to "new_max_pages" so we can keep the units > straight? (I know they use "new_max" in the XEN_DOMCTL_max_mem handler.) Yep. > > > +{ > > +ulong *p2m_array = NULL; > > +ulong *p2m_old = NULL; > > +ulong p2m_pages; > > +ulong copy_end = 0; > > + > > +/* we don't handle shrinking max_pages */ > > +if ( new_max < d->max_pages ) > > +{ > > +printk("Can't shrink %d max_mem\n", d->domain_id); > > +return -EINVAL; > > +} > > We won't be called in this case, so this test can be removed. OK. > > +/* our work is done here */ > > +if ( new_max == d->max_pages ) > > +return 0; > > + > > +/* check new_max pages is 16MiB aligned */ > > +if ( new_max & ((1<<12)-1) ) > > +{ > > +printk("Must be 16MiB aligned increments\n"); > > +return -EACCES; > > +} > > The 16MB thing is because the 970's large page size is 16MB, and the > kernel uses large pages to map its text. That said, I don't see why this > should be enforced by Xen when setting max_mem (if ever). Stylistically, > I also object to the literals used here. Great. I told myself that I was going to replace the literals since I was guessing that there might be a common check like cpu_aligned(). > > > +/* make a p2m array of new_max mfns */ > > +p2m_pages = (new_max * sizeof(ulong)) >> PAGE_SHIFT; > > +/* XXX: could use domheap anonymously */ > > +p2m_array = alloc_xenheap_pages(get_order_from_pages(p2m_pages)); > > +if ( p2m_array == NULL ) > > +return -ENOMEM; > > I think the Xen heap is on the small side. Do you have an idea of how > much memory we have available? I suppose we can change it later if we > exhaust the heap. IIRC, when dom0 boots with 192MB (the default) I usually see ~19MB of heap available in the boot messages on js20. Looking at js21, I see: (XEN) Xen Heap: 135MiB (138548KiB) RMA different size on js21? > > We had talked about using ints for the p2m array, since that would only > limit us to 44 bits of physical memory. Did you decide to use longs > instead? No, just being lazy. I wanted to get the patches out for comment ASAP but I forgot to note that we were going to use u32 in the mails. I still plan to switch p2m array to smaller size. > > > +/* copy old mappings into new array if any */ > > +if ( d->arch.p2m != NULL ) > > +{ > > +/* mark where the copy will stop (which page) */ > > +copy_end = d->max_pages; > > + > > +/* memcpy takes size in bytes */ > > +memcpy(p2m_array, d->arch.p2m, (d->max_pages * sizeof(ulong))); > > + > > +/* keep a pointer to the old array */ > > +p2m_old = d->arch.p2m; > > +} > > This memcpy could be pretty slow; might be better if we could make this > a continuation some day. If you agree, could you add a comment to that > effect? Good point. > > > +/* mark remaining (or all) mfn as INVALID_MFN, memset takes size in > > bytes */ > > +memset(p2m_array+copy_end, (int)INVALID_MFN, > > + (((ulong)(new_max - d->max_pages)) * sizeof(ulong))); > > Here you're initializing the array of longs with an int. Since > INVALID_MFN happens to be uniform (0x), it will work out, but I > don't think it's ideal coding practice Right, I guess that should have been a for loop. The fact that I had to cast that should have been a hint. > > > +/* set p2m pointer */ > > +d->arch.p2m = p2m_array; > > + > > +/* free old p2m array if present */ > > +if ( p2m_old ) > > +free_xenheap_pages(d->arch.p2m, > > get_order_from_pages(d->max_pages)); > > + > > +return 0; > > +} > > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain_
Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
On Wed, 2007-02-21 at 18:17 -0500, Ryan Harper wrote: > 4 files changed, 72 insertions(+) > xen/arch/powerpc/domain.c| 60 ++ > xen/arch/powerpc/domain_build.c |5 +++ > xen/include/asm-powerpc/domain.h |3 + > xen/include/asm-powerpc/shadow.h |4 ++ > > > # HG changeset patch > # User Ryan Harper <[EMAIL PROTECTED]> > # Date 1172103252 21600 > # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502 > # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28 > [PATCH] xen: implement guest_physmap_max_mem for ppc > > Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> > > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain.c > --- a/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 > +++ b/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #include "rtas.h" > #include "exceptions.h" > > @@ -347,3 +348,62 @@ void idle_loop(void) > do_softirq(); > } > } > + > +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) Could you rename "new_max" to "new_max_pages" so we can keep the units straight? (I know they use "new_max" in the XEN_DOMCTL_max_mem handler.) > +{ > +ulong *p2m_array = NULL; > +ulong *p2m_old = NULL; > +ulong p2m_pages; > +ulong copy_end = 0; > + > +/* we don't handle shrinking max_pages */ > +if ( new_max < d->max_pages ) > +{ > +printk("Can't shrink %d max_mem\n", d->domain_id); > +return -EINVAL; > +} We won't be called in this case, so this test can be removed. > +/* our work is done here */ > +if ( new_max == d->max_pages ) > +return 0; > + > +/* check new_max pages is 16MiB aligned */ > +if ( new_max & ((1<<12)-1) ) > +{ > +printk("Must be 16MiB aligned increments\n"); > +return -EACCES; > +} The 16MB thing is because the 970's large page size is 16MB, and the kernel uses large pages to map its text. That said, I don't see why this should be enforced by Xen when setting max_mem (if ever). Stylistically, I also object to the literals used here. > +/* make a p2m array of new_max mfns */ > +p2m_pages = (new_max * sizeof(ulong)) >> PAGE_SHIFT; > +/* XXX: could use domheap anonymously */ > +p2m_array = alloc_xenheap_pages(get_order_from_pages(p2m_pages)); > +if ( p2m_array == NULL ) > +return -ENOMEM; I think the Xen heap is on the small side. Do you have an idea of how much memory we have available? I suppose we can change it later if we exhaust the heap. We had talked about using ints for the p2m array, since that would only limit us to 44 bits of physical memory. Did you decide to use longs instead? > +/* copy old mappings into new array if any */ > +if ( d->arch.p2m != NULL ) > +{ > +/* mark where the copy will stop (which page) */ > +copy_end = d->max_pages; > + > +/* memcpy takes size in bytes */ > +memcpy(p2m_array, d->arch.p2m, (d->max_pages * sizeof(ulong))); > + > +/* keep a pointer to the old array */ > +p2m_old = d->arch.p2m; > +} This memcpy could be pretty slow; might be better if we could make this a continuation some day. If you agree, could you add a comment to that effect? > +/* mark remaining (or all) mfn as INVALID_MFN, memset takes size in > bytes */ > +memset(p2m_array+copy_end, (int)INVALID_MFN, > + (((ulong)(new_max - d->max_pages)) * sizeof(ulong))); Here you're initializing the array of longs with an int. Since INVALID_MFN happens to be uniform (0x), it will work out, but I don't think it's ideal coding practice. > +/* set p2m pointer */ > +d->arch.p2m = p2m_array; > + > +/* free old p2m array if present */ > +if ( p2m_old ) > +free_xenheap_pages(d->arch.p2m, get_order_from_pages(d->max_pages)); > + > +return 0; > +} > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain_build.c > --- a/xen/arch/powerpc/domain_build.c Wed Feb 21 18:14:12 2007 -0600 > +++ b/xen/arch/powerpc/domain_build.c Wed Feb 21 18:14:12 2007 -0600 > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -159,6 +160,10 @@ int construct_dom0(struct domain *d, > if (dom0_nrpages < CONFIG_MIN_DOM0_PAGES) > dom0_nrpages = CONFIG_MIN_DOM0_PAGES; > } > + > +/* set DOM0 max mem, triggering p2m table creation */ > +if ( (guest_physmap_max_mem(d, dom0_nrpages)) != 0 ) > +panic("Failed to set DOM0 max mem value\n"); > > /* By default DOM0 is allocated all available memory. */ > d->max_pages = dom0_nrpages; > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/include/asm-powerpc/domain.h > --- a/xen/include/asm-powerpc/domain.hWed Feb 21 18:14:12 2007 -0600 > +++ b/xen/include/asm-powerpc/domain.hWed Feb 21 18:14:12 2007 -0600 > @@ -46,6 +46,9 @@ struct ar
Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
* Christian Ehrhardt <[EMAIL PROTECTED]> [2007-02-22 07:40]: > >>+int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) > >>+{ > >>+ulong *p2m_array = NULL; > >>+ulong *p2m_old = NULL; > >>+ulong p2m_pages; > >>+ulong copy_end = 0; > >>+ > >>+/* we don't handle shrinking max_pages */ > >>+if ( new_max < d->max_pages ) > >>+{ > >>+printk("Can't shrink %d max_mem\n", d->domain_id); > >> > > > >Just some wording, but maybe printk("Can't shrink max_mem of domain > >%d\n", d->domain_id); would prevent some users from mis-interpreting > >the number as an unit of memory size. Yeah, definitely worth being more specific. > >>+/* free old p2m array if present */ > >>+if ( p2m_old ) > >>+free_xenheap_pages(d->arch.p2m, > >>get_order_from_pages(d->max_pages)); > >> > >Maybe I just don't get it right because I'm new in this area, but if > >an old mapping exists you do here summarized: > >1. save old d->arch.p2m in p2m_old > >2. create a new p2m_array including the old mapping as copy > >3. assigning the new array to d->arch.p2m > >4. ?? if an old mapping was present you free the new one in > >d->arch.p2m ?? > >=> this would leave one unreferenced heap allocation in memory > >(p2m_old) without a chance to free it after the local variable p2m_old > >disappears and the actively used table d->arch.p2m point to freed heap. > >I assume the freeing code should be > > > >+/* free old p2m array if present */ > >+if ( p2m_old ) > >+free_xenheap_pages(p2m_old, get_order_from_pages(d->max_pages)); Yep, good catch. Thanks. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx (512) 838-9253 T/L: 678-9253 [EMAIL PROTECTED] ___ Xen-ppc-devel mailing list Xen-ppc-devel@lists.xensource.com http://lists.xensource.com/xen-ppc-devel
Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
Christian Ehrhardt wrote: Ryan Harper wrote: 4 files changed, 72 insertions(+) xen/arch/powerpc/domain.c| 60 ++ xen/arch/powerpc/domain_build.c |5 +++ xen/include/asm-powerpc/domain.h |3 + xen/include/asm-powerpc/shadow.h |4 ++ # HG changeset patch # User Ryan Harper <[EMAIL PROTECTED]> # Date 1172103252 21600 # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502 # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28 [PATCH] xen: implement guest_physmap_max_mem for ppc Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain.c --- a/xen/arch/powerpc/domain.cWed Feb 21 18:14:12 2007 -0600 +++ b/xen/arch/powerpc/domain.cWed Feb 21 18:14:12 2007 -0600 @@ -33,6 +33,7 @@ #include #include #include +#include #include "rtas.h" #include "exceptions.h" @@ -347,3 +348,62 @@ void idle_loop(void) do_softirq(); } } + +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) +{ +ulong *p2m_array = NULL; +ulong *p2m_old = NULL; +ulong p2m_pages; +ulong copy_end = 0; + +/* we don't handle shrinking max_pages */ +if ( new_max < d->max_pages ) +{ +printk("Can't shrink %d max_mem\n", d->domain_id); Just some wording, but maybe printk("Can't shrink max_mem of domain %d\n", d->domain_id); would prevent some users from mis-interpreting the number as an unit of memory size. +return -EINVAL; +} + +/* our work is done here */ +if ( new_max == d->max_pages ) +return 0; + +/* check new_max pages is 16MiB aligned */ +if ( new_max & ((1<<12)-1) ) +{ +printk("Must be 16MiB aligned increments\n"); +return -EACCES; +} + +/* make a p2m array of new_max mfns */ +p2m_pages = (new_max * sizeof(ulong)) >> PAGE_SHIFT; +/* XXX: could use domheap anonymously */ +p2m_array = alloc_xenheap_pages(get_order_from_pages(p2m_pages)); +if ( p2m_array == NULL ) +return -ENOMEM; + +/* copy old mappings into new array if any */ +if ( d->arch.p2m != NULL ) +{ +/* mark where the copy will stop (which page) */ +copy_end = d->max_pages; + +/* memcpy takes size in bytes */ +memcpy(p2m_array, d->arch.p2m, (d->max_pages * sizeof(ulong))); + +/* keep a pointer to the old array */ +p2m_old = d->arch.p2m; +} + +/* mark remaining (or all) mfn as INVALID_MFN, memset takes size in bytes */ +memset(p2m_array+copy_end, (int)INVALID_MFN, + (((ulong)(new_max - d->max_pages)) * sizeof(ulong))); + +/* set p2m pointer */ +d->arch.p2m = p2m_array; + +/* free old p2m array if present */ +if ( p2m_old ) +free_xenheap_pages(d->arch.p2m, get_order_from_pages(d->max_pages)); Maybe I just don't get it right because I'm new in this area, but if an old mapping exists you do here summarized: 1. save old d->arch.p2m in p2m_old 2. create a new p2m_array including the old mapping as copy 3. assigning the new array to d->arch.p2m 4. ?? if an old mapping was present you free the new one in d->arch.p2m ?? => this would leave one unreferenced heap allocation in memory (p2m_old) without a chance to free it after the local variable p2m_old disappears and the actively used table d->arch.p2m point to freed heap. I assume the freeing code should be +/* free old p2m array if present */ +if ( p2m_old ) +free_xenheap_pages(p2m_old, get_order_from_pages(d->max_pages)); >> One more question, should there not be an update of the max_pages value in the domain structure before returning to ensure next time this (or other) function is called it can operate on the new size of the mapping (e.g. in pfn2mfn you would not get a mfn for all mappings that are located in the area now extended because there is a if(pfnmax_pages) and there are a lot of comparisons based on max_pages around there)? As suggestion add an extra line before the return like: + d->max_pages = new_max; And at last a question because I don't know the environment where this function is called from. May this function be called concurrently for the same "struct domain" ? If it would be possible you should add some locking to prevent races. << I checked the other patches you submitted within this set and found locking and the update of d->max_pages in "[PATCH 1 of 6] [PATCH] xen: add arch hook for max_mem hcall" so please skip the last two points (marked with >> <<), sorry for bothering you about that. + +return 0; +} diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain_build.c --- a/xen/arch/powerpc/domain_build.cWed Feb 21 18:14:12 2007 -0600 +++ b/xen/arch/powerpc/domain_build.cWed Feb 21 18:14:12 2007 -0600 @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -159,6 +160,10 @@ int construct_dom0(struct domain *d, if (dom0_nrpa
Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
Ryan Harper wrote: 4 files changed, 72 insertions(+) xen/arch/powerpc/domain.c| 60 ++ xen/arch/powerpc/domain_build.c |5 +++ xen/include/asm-powerpc/domain.h |3 + xen/include/asm-powerpc/shadow.h |4 ++ # HG changeset patch # User Ryan Harper <[EMAIL PROTECTED]> # Date 1172103252 21600 # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502 # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28 [PATCH] xen: implement guest_physmap_max_mem for ppc Signed-off-by: Ryan Harper <[EMAIL PROTECTED]> diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain.c --- a/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 +++ b/xen/arch/powerpc/domain.c Wed Feb 21 18:14:12 2007 -0600 @@ -33,6 +33,7 @@ #include #include #include +#include #include "rtas.h" #include "exceptions.h" @@ -347,3 +348,62 @@ void idle_loop(void) do_softirq(); } } + +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max) +{ +ulong *p2m_array = NULL; +ulong *p2m_old = NULL; +ulong p2m_pages; +ulong copy_end = 0; + +/* we don't handle shrinking max_pages */ +if ( new_max < d->max_pages ) +{ +printk("Can't shrink %d max_mem\n", d->domain_id); Just some wording, but maybe printk("Can't shrink max_mem of domain %d\n", d->domain_id); would prevent some users from mis-interpreting the number as an unit of memory size. +return -EINVAL; +} + +/* our work is done here */ +if ( new_max == d->max_pages ) +return 0; + +/* check new_max pages is 16MiB aligned */ +if ( new_max & ((1<<12)-1) ) +{ +printk("Must be 16MiB aligned increments\n"); +return -EACCES; +} + +/* make a p2m array of new_max mfns */ +p2m_pages = (new_max * sizeof(ulong)) >> PAGE_SHIFT; +/* XXX: could use domheap anonymously */ +p2m_array = alloc_xenheap_pages(get_order_from_pages(p2m_pages)); +if ( p2m_array == NULL ) +return -ENOMEM; + +/* copy old mappings into new array if any */ +if ( d->arch.p2m != NULL ) +{ +/* mark where the copy will stop (which page) */ +copy_end = d->max_pages; + +/* memcpy takes size in bytes */ +memcpy(p2m_array, d->arch.p2m, (d->max_pages * sizeof(ulong))); + +/* keep a pointer to the old array */ +p2m_old = d->arch.p2m; +} + +/* mark remaining (or all) mfn as INVALID_MFN, memset takes size in bytes */ +memset(p2m_array+copy_end, (int)INVALID_MFN, + (((ulong)(new_max - d->max_pages)) * sizeof(ulong))); + +/* set p2m pointer */ +d->arch.p2m = p2m_array; + +/* free old p2m array if present */ +if ( p2m_old ) +free_xenheap_pages(d->arch.p2m, get_order_from_pages(d->max_pages)); Maybe I just don't get it right because I'm new in this area, but if an old mapping exists you do here summarized: 1. save old d->arch.p2m in p2m_old 2. create a new p2m_array including the old mapping as copy 3. assigning the new array to d->arch.p2m 4. ?? if an old mapping was present you free the new one in d->arch.p2m ?? => this would leave one unreferenced heap allocation in memory (p2m_old) without a chance to free it after the local variable p2m_old disappears and the actively used table d->arch.p2m point to freed heap. I assume the freeing code should be +/* free old p2m array if present */ +if ( p2m_old ) +free_xenheap_pages(p2m_old, get_order_from_pages(d->max_pages)); One more question, should there not be an update of the max_pages value in the domain structure before returning to ensure next time this (or other) function is called it can operate on the new size of the mapping (e.g. in pfn2mfn you would not get a mfn for all mappings that are located in the area now extended because there is a if(pfnmax_pages) and there are a lot of comparisons based on max_pages around there)? As suggestion add an extra line before the return like: + d->max_pages = new_max; And at last a question because I don't know the environment where this function is called from. May this function be called concurrently for the same "struct domain" ? If it would be possible you should add some locking to prevent races. + +return 0; +} diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/arch/powerpc/domain_build.c --- a/xen/arch/powerpc/domain_build.c Wed Feb 21 18:14:12 2007 -0600 +++ b/xen/arch/powerpc/domain_build.c Wed Feb 21 18:14:12 2007 -0600 @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -159,6 +160,10 @@ int construct_dom0(struct domain *d, if (dom0_nrpages < CONFIG_MIN_DOM0_PAGES) dom0_nrpages = CONFIG_MIN_DOM0_PAGES; } + +/* set DOM0 max mem, triggering p2m table creation */ +if ( (guest_physmap_max_mem(d, dom0_nrpages)) != 0 ) +panic("Failed to set DOM0 max mem value\n"); /* By default DOM0 is allocated all available memory.