Re: [RFC][PATCH 04/13] tools/libxl: detect and avoid conflicts with RDM

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

On 2015/4/15 21:10, Ian Jackson wrote:
> Tiejun Chen writes ("[RFC][PATCH 04/13] 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 RAM.
>

Thanks for your review.

> I'm not really qualified to understand all of this, because I'm not an
> x86 expert - I don't even know what RDM is.  But this does all seem
> very complicated.  Can I have a second opinion from an x86 expert ?

I hope Kevin's reply is fine to you. But if you still have further 
question let me know and I'd like to make this point clear to you :)

>
> I had a quick look at the libxl code and it at the very least needs
> updating to conform to tools/libxl/CODING_STYLE.
>

I took a look at some libxl codes and found some obvious code style issues.

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8a5f589..ff40c65 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -97,7 +97,7 @@ const char *libxl__domain_device_model(libxl__gc *gc,
  static int check_rdm_hole(uint64_t start, uint64_t memsize,
                            uint64_t rdm_start, uint64_t rdm_size)
  {
-    if ( start + memsize <= rdm_start || start >= rdm_start + rdm_size )
+    if (start + memsize <= rdm_start || start >= rdm_start + rdm_size)
          return 0;
      else
          return 1;
@@ -163,7 +163,8 @@ int libxl__domain_device_check_rdm(libxl__gc *gc,
          if (!nr_entries)
              continue;

-        /* Need to check whether this entry is already saved in the array.
+        /*
+         * Need to check whether this entry is already saved in the array.
           * This could come from two cases:
           *
           *   - user may configure to get all RMRRs in this platform, which
@@ -207,7 +208,8 @@ int libxl__domain_device_check_rdm(libxl__gc *gc,
      /* Fix highmem. */
      if (args->mem_size > args->lowmem_size)
          highmem_end += (args->mem_size - args->lowmem_size);
-    /* Next step is to check and avoid potential conflict between RDM 
entries
+    /*
+     * Next step is to check and avoid potential conflict between RDM 
entries
       * and guest RAM. To avoid intrusive impact to existing memory layout
       * {lowmem, mmio, highmem} which is passed around various function 
blocks,
       * below conflicts are not handled which are rare and handling 
them would

Is this good? I know Jan had some comments on this patch as well so 
actually something needs to be changed but here just lets focus on code 
style firstly :)

Thanks
Tiejun