[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 for your comments and looks I should address them now.


+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\
+    if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_\

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

Sorry I can't found any case to what you're talking.

So are you saying this?


@@ -143,6 +143,15 @@ static bool overlaps_rdm(uint64_t start, uint64_t memsize,

+ * Check whether any rdm should be exposed..
+ * Returns true if needs, else returns false.
+ */
+static bool exposes_rdm(uint32_t strategy, int num_pcidevs)
+    return strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !num_pcidevs;
  * Check reported RDM regions and handle potential gfn conflicts according
  * to user preferred policy.
@@ -182,7 +191,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc,
uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull<<32);

     /* Might not expose rdm. */
- if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_pcidevs)
+    if (exposes_rdm(strategy, d_config->num_pcidevs))
         return 0;

     /* Query all RDM entries in this platform */

+        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.

Do you mean I should merge them as one as possible?

But seems not be possible because we have seveal combinations of these two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or pci devices are also passes through.

#1. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST but without any devices

So it appropriately needs this libxl__realloc() here. The second libxl__realloc() doesn't take any effect.

#2. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST with one or more devices

Actually we don't need the second libxl__realloc(). This is same as #1.

#3. strategy != LIBXL_RDM_RESERVE_STRATEGY_HOST also with one or more devices

So we just need the second libxl__realloc() later. The fist libxl__realloc() doesn't be called at all. Especially, its very possible we're going to handle less RDMs, compared to LIBXL_RDM_RESERVE_STRATEGY_HOST.

+    } 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

Like this?

#define AAAA(x) ((uint64_t)x << XC_PAGE_SHIFT)

Sorry I can't figure out a good name here :) Any suggestions?

+    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.)

I can do this but according to tools/libxl/CODING_STYLE, looks I should first post a separate patch to fix this code style issue, and then rebase my patch, right?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.