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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Tiejun Chen writes ("[v10][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.
...

Thanks.  I think this patch is nearly there.

> +static int
> +libxl__xc_device_get_rdm(libxl__gc *gc,
...
> +    *xrdm = libxl__malloc(gc,
> +                          *nr_entries * 
> sizeof(xen_reserved_device_memory_t));

Sorry for not spotting this before, but this should use GCNEW_ARRAY.

> +}
> +
> +#define pfn_to_paddr(x) ((uint64_t)x << XC_PAGE_SHIFT)
> +static void

Missing blank line after the #define - although I think this #define
could profitably be moved to libxl_internal.h.  If you do keep it here
please move it further up the file, to at least before any function
or struct definition.


Also the #define is missing safety ( ) around x.

> +set_rdm_entries(libxl__gc *gc, libxl_domain_config *d_config,
> +                uint64_t rdm_start, uint64_t rdm_size, int rdm_policy,
> +                unsigned int nr_entries)
> +{
> +    assert(nr_entries);
> +
> +    d_config->num_rdms = nr_entries;
> +    d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
> +                            d_config->num_rdms * sizeof(libxl_device_rdm));
> +
> +    d_config->rdms[d_config->num_rdms - 1].start = rdm_start;
> +    d_config->rdms[d_config->num_rdms - 1].size = rdm_size;
> +    d_config->rdms[d_config->num_rdms - 1].policy = rdm_policy;
> +}

Thanks for breaking this out.  I think though that the division of
labour between this function and the call site is confusing.

Can I suggest a function

   void add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
                 uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)

which assumes that d_config->num_rdms is set correctly, and increments
it ?

(Please put the increment at the end so that the assignments are to
->rdms[d_config->num_rdms], or perhaps make a convenience alias.)


> +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)
> +{
...
> +    /* Query all RDM entries in this platform */
> +    if (strategy == LIBXL_RDM_RESERVE_STRATEGY_HOST) {
...
> +    } else {
> +        d_config->num_rdms = 0;
> +    }

Does this not override the domain configuration's num_rdms ?  I don't
think that is correct.

If the domain configuration has rdms and num_rdms already set, then
the strategy should presumably be ignored.  (Passing the same domain
configuration struct to libxl_domain_create_new, after destroying the
domain, ought to work, even after the first call has modified it.)


Can you please also wrap at least the already-word-wrapped comments to
70 (or maybe 75) columns ?  What you have lookes like this when I
quote it for review:

> +     * Next step is to check and avoid potential conflict between RDM entri\
es
> +     * and guest RAM. To avoid intrusive impact to existing memory layout
> +     * {lowmem, mmio, highmem} which is passed around various function bloc\
ks,
> +     * below conflicts are not handled which are rare and handling them wou\
ld


Thanks,
Ian.

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