Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc

[Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>]

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 <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)
> > +{
> > +    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(pfn<d->max_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.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)
> > +
> >  extern void shadow_drop_references(
> >      struct domain *d, struct page_info *page);
> >
> >
> > _______________________________________________
> > Xen-ppc-devel mailing list
> > Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-ppc-devel
> >   
--

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
+49 7031/16-3385
Ehrhardt@xxxxxxxxxxxxxxxxxxx
Ehrhardt@xxxxxxxxxx

IBM Deutschland Entwicklung GmbH
Vorsitzender des Aufsichtsrats: Johann Weihen 
Geschäftsführung: Herbert Kircher 
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel