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

["Chen, Tiejun" <tiejun.chen@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 for your comments and looks I should address them now.

> 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.
Sorry I can't found any case to what you're talking.

So are you saying this?

if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE &&
    !d_config->num_pcidevs)
Or

@@ -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.
Fixed.

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

> > +    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?
Thanks
TIejun

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