Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Tiejun Chen writes ("[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM"):
> While building a VM, HVM domain builder provides struct hvm_info_table{}
> to help hvmloader. Currently it includes two fields to construct guest
> e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should
> check them to fix any conflict with RDM.
...
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> Reviewed-by: Kevin Tian <kevint.tian@xxxxxxxxx>

I have found a few things in this patch which I would like to see
improved.  See below.

Given how late I am with this review, I do not feel that I should be
nacking it at this time.  You have a tools ack from Wei, so my
comments are not a blocker for this series.

But if you need to respin, please take these comments into account,
and consider which are feasible to fix in the time available.  If you
are respinning this series targeting Xen 4.7 or later, please address
all of the points I make below.

Thanks.


> +int libxl__domain_device_construct_rdm(libxl__gc *gc,
> +                                       libxl_domain_config *d_config,
> +                                       uint64_t rdm_mem_boundary,
> +                                       struct xc_hvm_build_args *args)
...
> +    uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull\
<<32);
...
> +    if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_\
pcidevs)

There are quite a few of these long lines, which should be wrapped.
See tools/libxl/CODING_STYLE.

> +        d_config->num_rdms = nr_entries;
> +        d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
> +                        d_config->num_rdms * sizeof(libxl_device_rdm));

This code is remarkably similar to a function later on which adds an
rdm.  Please can you factor it out.

> +    } else
> +        d_config->num_rdms = 0;

Please can you put { } around the else block too.  I don't think this
mixed style is good.

> +        for (j = 0; j < d_config->num_rdms; j++) {
> +            if (d_config->rdms[j].start ==
> +                         (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT)

This construct
   (uint64_t)some_pfn << XC_PAGE_SHIFT
appears an awful lot.

I would prefer it if it were done in an inline function (or maybe a
macro).


> +    libxl_domain_build_info *const info = &d_config->b_info;
> +    /*
> +     * Currently we fix this as 2G to guarantte how to handle
                                         ^^^^^^^^^

Should read "guarantee".

> +    ret = libxl__domain_device_construct_rdm(gc, d_config,
> +                                             rdm_mem_boundary,
> +                                             &args);
> +    if (ret) {
> +        LOG(ERROR, "checking reserved device memory failed");
> +        goto out;
> +    }

`rc' should be used here rather than `ret'.  (It is unfortunate that
this function has poor style already, but it would be best not to make
it worse.)


Thanks,
Ian.

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