Re: [PATCH v3 09/32] libxl: switch HVM domain building to use xc_dom_* helpers

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

El 28/07/15 a les 13.22, Wei Liu ha escrit:
> On Fri, Jul 03, 2015 at 01:34:47PM +0200, Roger Pau Monne wrote:
>> Now that we have all the code in place HVM domain building in libxl can be
>> switched to use the xc_dom_* family of functions, just like they are used in
>> order to build PV guests.
>>
>> 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>
> 
> Mostly looks good. Some nits below.

Thanks for the review.

>> ---
>>  tools/libxl/libxl_dom.c      | 224 +++++++++++++++++++++++++------------------
>>  tools/libxl/libxl_internal.h |   2 +-
>>  tools/libxl/libxl_vnuma.c    |  12 ++-
>>  3 files changed, 139 insertions(+), 99 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 2bae277..480b7e7 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -609,6 +609,64 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>>      return rc;
>>  }
>>  
>> +static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>> +             libxl_domain_build_info *info, libxl__domain_build_state *state,
>> +             struct xc_dom_image *dom)
>> +{
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> 
> No need to have this line.

But ctx is needed...

>> +    uint64_t mem_kb;
>> +    int ret;
>> +
>> +    if ( (ret = xc_dom_boot_xen_init(dom, ctx->xch, domid)) != 0 ) {
                                              ^ here.
>> +        LOGE(ERROR, "xc_dom_boot_xen_init failed");
>> +        goto out;
>> +    }
>> +#ifdef GUEST_RAM_BASE
>> +    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_rambase failed");
>> +        goto out;
>> +    }
>> +#endif
>> +    if ( (ret = xc_dom_parse_image(dom)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_parse_image failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, state, dom)) != 0 ) {
>> +        LOGE(ERROR, "libxl__arch_domain_init_hw_description failed");
>> +        goto out;
>> +    }
>> +
>> +    mem_kb = dom->container_type == XC_DOM_HVM_CONTAINER ?
>> +             (info->max_memkb - info->video_memkb) : info->target_memkb;
>> +    if ( (ret = xc_dom_mem_init(dom, mem_kb / 1024)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_mem_init failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = xc_dom_boot_mem_init(dom)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_boot_mem_init failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, info, dom)) != 0 ) {
>> +        LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = xc_dom_build_image(dom)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_build_image failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = xc_dom_boot_image(dom)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_boot_image failed");
>> +        goto out;
>> +    }
>> +    if ( (ret = xc_dom_gnttab_init(dom)) != 0 ) {
>> +        LOGE(ERROR, "xc_dom_gnttab_init failed");
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    return ret;
> 
> Please translate libxc error code to libxl error code.

Done, I have however just done:

return ret != 0 ? ERROR_FAIL : 0;

Not sure if we need/want a more complex error code translation here.

>> +}
>> +
>>  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>               libxl_domain_build_info *info, libxl__domain_build_state *state)
>>  {
>> @@ -699,48 +757,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>              dom->vnode_to_pnode[i] = info->vnuma_nodes[i].pnode;
>>      }
>>  
> [...]
>>      }
>>  
>> @@ -919,50 +943,60 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>>                libxl__domain_build_state *state)
>>  {
>>      libxl_ctx *ctx = libxl__gc_owner(gc);
>> -    struct xc_hvm_build_args args = {};
>> -    int ret, rc = ERROR_FAIL;
>> -    uint64_t mmio_start, lowmem_end, highmem_end;
>> +    int ret;
> 
> This should be rc.

Right, we don't call any libxc functions directly any longer, so only rc
should be used.

Roger.