Re: [PATCH 4/5] libxc/arm: allocate xenstore and console pages

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 26 Jun 2012, Ian Campbell wrote:
> On Fri, 2012-06-22 at 17:09 +0100, Stefano Stabellini wrote:
> > Allocate two additional pages at the end of the guest physical memory
> > for xenstore and console.
> > Set HVM_PARAM_STORE_PFN and HVM_PARAM_CONSOLE_PFN to the corresponding
> > values.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxc/xc_dom_arm.c |   32 ++++++++++++++++++++++----------
> >  1 files changed, 22 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > index bb86139..df2eefe 100644
> > --- a/tools/libxc/xc_dom_arm.c
> > +++ b/tools/libxc/xc_dom_arm.c
> > @@ -25,6 +25,10 @@
> >  #include "xg_private.h"
> >  #include "xc_dom.h"
> >  
> > +#define NR_MAGIC_PAGES 2
> > +#define CONSOLE_PFN_OFFSET 0
> > +#define XENSTORE_PFN_OFFSET 1
> > +
> >  /* ------------------------------------------------------------------------ */
> >  /*
> >   * arm guests are hybrid and start off with paging disabled, therefore no
> > @@ -47,12 +51,6 @@ static int setup_pgtables_arm(struct xc_dom_image *dom)
> >  static int alloc_magic_pages(struct xc_dom_image *dom)
> >  {
> >      DOMPRINTF_CALLED(dom->xch);
> > -    /* XXX
> > -     *   dom->p2m_guest
> > -     *   dom->start_info_pfn
> > -     *   dom->xenstore_pfn
> > -     *   dom->console_pfn
> > -     */
> >      return 0;
> >  }
> >  
> > @@ -127,18 +125,19 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >  {
> >      int rc;
> >      xen_pfn_t pfn, allocsz, i;
> > +    xen_pfn_t store_pfn, console_pfn;
> >  
> >      fprintf(stderr, "%s: tot pages %"PRI_xen_pfn" rambase %"PRI_xen_pfn"\n", __func__,
> >              dom->total_pages, dom->rambase_pfn);
> >  
> >      dom->shadow_enabled = 1;
> >  
> > -    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
> > +    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * (dom->total_pages + NR_MAGIC_PAGES));
> >  
> >      fprintf(stderr, "%s: setup p2m from %"PRI_xen_pfn" for %"PRI_xen_pfn" pages\n", __func__,
> >              dom->rambase_pfn, dom->total_pages );
> >      /* setup initial p2m */
> > -    for ( pfn = 0; pfn < dom->total_pages; pfn++ )
> > +    for ( pfn = 0; pfn < (dom->total_pages + NR_MAGIC_PAGES); pfn++ )
> >          dom->p2m_host[pfn] = pfn + dom->rambase_pfn;
> >  
> >      fprintf(stderr, "%s: init'd p2m_host[0] = %"PRI_xen_pfn"\n", __func__, dom->p2m_host[0]);
> > @@ -148,10 +147,10 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >  
> >      /* allocate guest memory */
> >      for ( i = rc = allocsz = 0;
> > -          (i < dom->total_pages) && !rc;
> > +          (i < dom->total_pages + NR_MAGIC_PAGES) && !rc;
> >            i += allocsz )
> >      {
> > -        allocsz = dom->total_pages - i;
> > +        allocsz = (dom->total_pages + NR_MAGIC_PAGES) - i;
> 
> All these "+ NR_MAGIC_PAGES" are a bit troublesome looking.
> 
> Do these pages need to be in p2m_host or would it be fine to just insert
> them into the guest p2m individually outside the main allocation logic?

I think it makes sense for them to be in p2m_host. In fact if we try to
allocate them later, wouldn't we have the problem of having to extend
the guest p2m? We might as well do it here.


> Otherwise perhaps simply int total_pages = dom->total_pages + NR... and
> use throughout?

Yes, I can do that.


> >          if ( allocsz > 1024*1024 )
> >              allocsz = 1024*1024;
> >          fprintf(stderr, "alloc %"PRI_xen_pfn" at offset %"PRI_xen_pfn" which is %"PRI_xen_pfn"\n",
> > @@ -168,6 +167,19 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >      fprintf(stderr, "%s: popl'd p2m_host[%"PRI_xen_pfn"] = %"PRI_xen_pfn"\n",
> >              __func__, dom->total_pages-1, dom->p2m_host[dom->total_pages-1]);
> 
> I really need to scrub the debug printfs from my patch...
> 
> >  
> > +    console_pfn = dom->rambase_pfn + dom->total_pages + CONSOLE_PFN_OFFSET;
> > +    store_pfn = dom->rambase_pfn + dom->total_pages + XENSTORE_PFN_OFFSET;
> > +
> > +    xc_clear_domain_page(dom->xch, dom->guest_domid, console_pfn);
> > +    xc_clear_domain_page(dom->xch, dom->guest_domid, store_pfn);
> > +    xc_set_hvm_param(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
> > +            console_pfn);
> > +    xc_set_hvm_param(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> > +            store_pfn);
> > +
> > +    fprintf(stderr, "%s: console_pfn=%"PRI_xen_pfn" xenstore_pfn=%"PRI_xen_pfn"\n",
> > +            __func__, console_pfn, store_pfn);
> 
> ... and so do you ;-)
 
OK :)