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

["Chen, Tiejun" <tiejun.chen@intel.com>]

> 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