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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  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.

Roger.