Re: [Xen-devel] [PATCH v4 08/31] libxc: rework BSP initialization

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Aug 07, 2015 at 02:04:21PM +0200, Roger Pau Monné wrote:
> El 07/08/15 a les 13.31, Wei Liu ha escrit:
> > On Fri, Aug 07, 2015 at 12:17:45PM +0200, Roger Pau Monne wrote:
> >> Place the calls to xc_vcpu_setcontext and the allocation of the hypercall
> >> buffer into the arch-specific vcpu hooks. This is needed for the next 
> >> patch,
> >> so x86 HVM guests can initialize the BSP using XEN_DOMCTL_sethvmcontext
> >> instead of XEN_DOMCTL_setvcpucontext.
> >>
> >> This patch should not introduce any functional change.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> ---
> >>  tools/libxc/include/xc_dom.h |  2 +-
> >>  tools/libxc/xc_dom_arm.c     | 22 +++++++++++++++++-----
> >>  tools/libxc/xc_dom_boot.c    | 23 +----------------------
> >>  tools/libxc/xc_dom_x86.c     | 26 ++++++++++++++++++++------
> >>  4 files changed, 39 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> >> index 5c1bb0f..0245d24 100644
> >> --- a/tools/libxc/include/xc_dom.h
> >> +++ b/tools/libxc/include/xc_dom.h
> >> @@ -221,7 +221,7 @@ struct xc_dom_arch {
> >>      /* arch-specific data structs setup */
> >>      int (*start_info) (struct xc_dom_image * dom);
> >>      int (*shared_info) (struct xc_dom_image * dom, void *shared_info);
> >> -    int (*vcpu) (struct xc_dom_image * dom, void *vcpu_ctxt);
> >> +    int (*vcpu) (struct xc_dom_image * dom);
> >>      int (*bootearly) (struct xc_dom_image * dom);
> >>      int (*bootlate) (struct xc_dom_image * dom);
> >>  
> >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >> index 7548dae..8865097 100644
> >> --- a/tools/libxc/xc_dom_arm.c
> >> +++ b/tools/libxc/xc_dom_arm.c
> >> @@ -119,9 +119,10 @@ static int shared_info_arm(struct xc_dom_image *dom, 
> >> void *ptr)
> >>  
> >>  /* 
> >> ------------------------------------------------------------------------ */
> >>  
> >> -static int vcpu_arm32(struct xc_dom_image *dom, void *ptr)
> >> +static int vcpu_arm32(struct xc_dom_image *dom)
> >>  {
> >> -    vcpu_guest_context_t *ctxt = ptr;
> >> +    vcpu_guest_context_any_t any_ctx;
> >> +    vcpu_guest_context_t *ctxt = &any_ctx.c;
> >>  
> > 
> > I think you still need to allocate hypercall safe buffer here, as well
> > as in other vcpu_* functions.
> 
> xc_vcpu_setcontext already bounces the buffer before doing the
> hypercall, so I think it's safe.
> 
> >>      DOMPRINTF_CALLED(dom->xch);
> >>  
> >> @@ -154,12 +155,18 @@ static int vcpu_arm32(struct xc_dom_image *dom, void 
> >> *ptr)
> >>      DOMPRINTF("Initial state CPSR %#"PRIx32" PC %#"PRIx32,
> >>             ctxt->user_regs.cpsr, ctxt->user_regs.pc32);
> >>  
> >> -    return 0;
> >> +    rc = xc_vcpu_setcontext(dom->xch, dom->guest_domid, 0, &any_ctx);
> >> +    if ( rc != 0 )
> >> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> >> +                     "%s: SETVCPUCONTEXT failed (rc=%d)", __func__, rc);
> >> +
> >> +    return rc;
> >>  }
> >>  
> > [...]
> >> -    ctxt = xc_hypercall_buffer_alloc(dom->xch, ctxt, sizeof(*ctxt));
> >> -    if ( ctxt == NULL )
> >> -        return -1;
> >> -
> >>      DOMPRINTF_CALLED(dom->xch);
> >>  
> >>      /* misc stuff*/
> >> @@ -259,13 +241,10 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
> >>          return rc;
> >>  
> >>      /* let the vm run */
> >> -    memset(ctxt, 0, sizeof(*ctxt));
> >> -    if ( (rc = dom->arch_hooks->vcpu(dom, ctxt)) != 0 )
> >> +    if ( (rc = dom->arch_hooks->vcpu(dom)) != 0 )
> >>          return rc;
> >>      xc_dom_unmap_all(dom);
> > 
> > This is not your problem, but this xc_dom_unmap_all is really
> > suspicious.
> 
> I agree, xc_dom_release already calls xc_dom_unmap_all. I guess it's not
> a big issue since xc_dom_boot_image is almost the last function called
> when doing domain creation, but I'm also not that familiar with libxc in
> order to figure out if there's a reason for calling xc_dom_unmap_all here.
> 

Yeah, let's worry about that later.

Wei.

> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel