|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc
* Hollis Blanchard <hollisb@xxxxxxxxxx> [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 <ryanh@xxxxxxxxxx>
> > # Date 1172103252 21600
> > # Node ID 35fd77200dff7e73fe3959b5dbfa6088c691c502
> > # Parent 84ec1b4d5cd50cc9d49202eb978a4715c4780e28
> > [PATCH] xen: implement guest_physmap_max_mem for ppc
> >
> > Signed-off-by: Ryan Harper <ryanh@xxxxxxxxxx>
> >
> > 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 <asm/htab.h>
> > #include <asm/current.h>
> > #include <asm/hcalls.h>
> > +#include <asm/shadow.h>
> > #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 (0xffffffff), 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_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 <xen/shadow.h>
> > #include <xen/domain.h>
> > #include <xen/version.h>
> > +#include <xen/shadow.h>
> > #include <asm/processor.h>
> > #include <asm/papr.h>
> > #include <public/arch-powerpc.h>
> > @@ -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.h Wed Feb 21 18:14:12 2007 -0600
> > +++ b/xen/include/asm-powerpc/domain.h Wed Feb 21 18:14:12 2007 -0600
> > @@ -46,6 +46,9 @@ struct arch_domain {
> >
> > /* I/O-port access bitmap mask. */
> > u8 *iobmp_mask; /* Address of IO bitmap mask, or NULL. */
> > +
> > + /* P2M mapping array */
> > + ulong *p2m;
> >
> > uint large_page_sizes;
> > uint large_page_order[4];
> > diff -r 84ec1b4d5cd5 -r 35fd77200dff xen/include/asm-powerpc/shadow.h
> > --- a/xen/include/asm-powerpc/shadow.h Wed Feb 21 18:14:12 2007 -0600
> > +++ b/xen/include/asm-powerpc/shadow.h Wed Feb 21 18:14:12 2007 -0600
> > @@ -37,6 +37,10 @@ extern void guest_physmap_remove_page(
> > extern void guest_physmap_remove_page(
> > struct domain *d, unsigned long gpfn, unsigned long mfn);
> >
> > +int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max);
> > +#undef guest_physmap_max_mem
> > +#define guest_physmap_max_mem(d, n) do_guest_physmap_max_mem(d, n)
> > +
>
> I hate this undef and define. Here's what I want:
>
> asm-x86/shadow.h
> #define guest_physmap_max_mem(d, n) (0)
> asm-ia64/shadow.h
> #define guest_physmap_max_mem(d, n) (0)
> asm-powerpc/shadow.h
> extern int guest_physmap_max_mem(struct domain *d, unsigned long
> new_max);
Yep. On my list to change; meant to indicate that I planned to change
that in the final patch set.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253 T/L: 678-9253
ryanh@xxxxxxxxxx
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |