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

2015-07-22 Thread Ian Campbell 
On Wed, 2015-07-22 at 17:18 +0800, Chen, Tiejun wrote:
 Thanks for your clarification to me.
 
  The solution to this is to be systematic in how you handle the 
  email
  based review of a series, not to add a further to the reviewer by 
  using
  IRC as well.
  
  For example, here is how I handle review of a series I am working 
  on:
  
  I keep all the replies to a series I'm working on marked unread. 
  When I
  come to rework the series I take the first reply to the first patch 
  and
  start a draft reply to it quoting the entire review.
  
  I then go through the comments one by one and either:
  
 * make the _complete_ code change, including adding the 
  Changes
   in vN bit to the commit log and delete that comment from 
  the
   reply
 
 Are you saying this case of resending this particular patch online?

No, I am talking about making the change in my local git tree. Only
once I have addressed _all_ of the review of the whole series would I
resent a complete new version of the series for further review.

 
  or
 * write a reply in my draft to that particular comment which 
  does
   one or more of:
  
 * Explain that I don't understand the suggestion,
   probably asking questions to try and clarify what 
  was
   being asked for.
 
 Yes, in this case we're arguing, I was really trying to send a sample 
 of 
 this code fragment to ask this before I sent out the complete change.

I was not talking about any specific case here.

  At the end of this process _every_ bit of review feedback is addressed
  either by making the requested change or by responding explaining the
  reason why the change hasn't been made. This is absolutely crucial. You
  should never silently ignore a piece of review, even if you don't
 
 I should double check each line but sometimes this doesn't mean I can 
 
 understand everything correctly to do right thing as you expect. But 
 this is really not what I intend to do.

I have outlined my strategy for dealing with review which helps
avoid/minimise the error rate (i.e. failing to address comments) when
dealing with reviews. I hope it will be helpful for you in forming your
own strategy.

Ian.

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

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

2015-07-22 Thread Chen, Tiejun 

Thanks for your clarification to me.


The solution to this is to be systematic in how you handle the email
based review of a series, not to add a further to the reviewer by using
IRC as well.

For example, here is how I handle review of a series I am working on:

I keep all the replies to a series I'm working on marked unread. When I
come to rework the series I take the first reply to the first patch and
start a draft reply to it quoting the entire review.

I then go through the comments one by one and either:

   * make the _complete_ code change, including adding the Changes
 in vN bit to the commit log and delete that comment from the
 reply


Are you saying this case of resending this particular patch online?


or
   * write a reply in my draft to that particular comment which does
 one or more of:

   * Explain that I don't understand the suggestion,
 probably asking questions to try and clarify what was
 being asked for.


Yes, in this case we're arguing, I was really trying to send a sample of 
this code fragment to ask this before I sent out the complete change.



   * Explain, with reasons, why I disagree with the
 suggestion
   * Explain, with reasons, why I only implemented part of
 the suggestion.

Only then do I move on to the next comment in that mail and repeat. At
the end I've either deleted all the comments from my draft (because
I've fully implemented everything) so the draft can be discarded or I
have an email to send explaining what I've not done and why. Only now
do I mark the original review email as read.

Then I move on to the next reply to that thread in my mail box and
repeat until I have been through every mail in the thread and addressed
_all_ of the comments.

At the end of this process _every_ bit of review feedback is addressed
either by making the requested change or by responding explaining the
reason why the change hasn't been made. This is absolutely crucial. You
should never silently ignore a piece of review, even if you don't


I should double check each line but sometimes this doesn't mean I can 
understand everything correctly to do right thing as you expect. But 
this is really not what I intend to do.


Thanks
Tiejun


understand it or disagree with it, always reply and explain why you
haven't.

If the review was particularly long or complex I will then do a second
pass through the review comments and check that every comment is either
mentioned in a Changes in vN changelog comment or I have replied to
it.

I do all of that before posting the next version. IMHO until a
contributor has shown they are diligent about addressing review
comments they should _never_ send out a series which only has review
partially addressed.

The presence of an IRC channel in no way changes the requirement to be
systematic and thorough when dealing with email review.

Ian.

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



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

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

2015-07-22 Thread Ian Jackson 
Chen, Tiejun writes (Re: [Xen-devel] [v10][PATCH 11/16] tools/libxl: detect 
and avoid conflicts with RDM):
  I then go through the comments one by one and either:
 
 * make the _complete_ code change, including adding the Changes
   in vN bit to the commit log and delete that comment from the
   reply
 
 Are you saying this case of resending this particular patch online?

Here is an example of what Ian C is talking about: You failed to
address all my comments before posting v10.  That caused me to post
this:

  This is now the third time I have posted that text.  It is the fifth
  request or clarification I have had to make about this very small
  area.  I have to say that I'm finding this rather frustrating.

You should have been systematic.  Like Ian C suggests.  I do it that
way too.  Instead, you repeatedly left things undone.



 Yes, in this case we're arguing, I was really trying to send a sample of 
 this code fragment to ask this before I sent out the complete change.

No, your v10 series was not a sample.


It is of course OK to post a sample.  But a reviewer will not read
such a sample thoroughly; they will not look for all problems.  The
reviewer will look at the sample, so that the reviewer can understand
your words.  They will consider the aspects of the sample that are
being discussed.  They will not consider other aspects of the sample.

You did post such a sample in 55ae2bb1.9030...@intel.com.  I read
the sample, to understand what your words meant.

Then later, you complained that:

  But indeed, before I post this whole patch online I also picked up
  this chunk of code to ask you to take a look that.

Ie you complained that I did not thoroughly review your sample.


You cannot have this both ways.  When you are posting a sample, it
is purely to illuminate the particular discussion.  When you are
posting a full patch for review, you must have addressed _every_
previous comment.


Ian.

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

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

2015-07-22 Thread Ian Campbell 
On Wed, 2015-07-22 at 08:33 +0800, Chen, Tiejun wrote:
 On 2015/7/21 23:57, Ian Jackson wrote:
 
  It's true that I didn't comment on the frat that you had half-done one
  of the things I had requested.  It is of course a waste of my time to
  be constantly re-reviewing half-done changes.

 Next time I should see each line of all comments carefully. Maybe its
 good way to use IRC to take your quick advice in advance,

The solution to this is to be systematic in how you handle the email
based review of a series, not to add a further to the reviewer by using
IRC as well.

For example, here is how I handle review of a series I am working on:

I keep all the replies to a series I'm working on marked unread. When I
come to rework the series I take the first reply to the first patch and
start a draft reply to it quoting the entire review.

I then go through the comments one by one and either:

  * make the _complete_ code change, including adding the Changes
in vN bit to the commit log and delete that comment from the
reply
or
  * write a reply in my draft to that particular comment which does
one or more of:

  * Explain that I don't understand the suggestion,
probably asking questions to try and clarify what was
being asked for.
  * Explain, with reasons, why I disagree with the
suggestion
  * Explain, with reasons, why I only implemented part of
the suggestion.

Only then do I move on to the next comment in that mail and repeat. At
the end I've either deleted all the comments from my draft (because
I've fully implemented everything) so the draft can be discarded or I
have an email to send explaining what I've not done and why. Only now
do I mark the original review email as read.

Then I move on to the next reply to that thread in my mail box and
repeat until I have been through every mail in the thread and addressed
_all_ of the comments.

At the end of this process _every_ bit of review feedback is addressed
either by making the requested change or by responding explaining the
reason why the change hasn't been made. This is absolutely crucial. You
should never silently ignore a piece of review, even if you don't
understand it or disagree with it, always reply and explain why you
haven't.

If the review was particularly long or complex I will then do a second
pass through the review comments and check that every comment is either
mentioned in a Changes in vN changelog comment or I have replied to
it.

I do all of that before posting the next version. IMHO until a
contributor has shown they are diligent about addressing review
comments they should _never_ send out a series which only has review
partially addressed.

The presence of an IRC channel in no way changes the requirement to be
systematic and thorough when dealing with email review.

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

Just to your example,

libxl_domain_config cfg;
cfg.stuff = blah;
cfg.rdm.strategy = HOST;

libxl_domain_create_new(cfg, domid);
libxl_domain_destroy(domid);

Here looks you mean d_config-rdms would be changed, right? Currently
this shouldn't be allowed. But I think we need to further discussion
make this case clear after feature freeze since we didn't have this kind
of assumption in our previous design.

libxl_domain_create_new(cfg, domid);


This response of yours does not lead me to think you have understood
what I am saying, but I agree that this can be dealt with later (if


Indeed, I'm not a fan to Xen tools so I can't picture what this real 
scenario would happen. So if I'm misunderstanding what you mean, just 
please correct me. Or if you still think its hard to explain this to me, 
just tell me what I should do. I think this make your life easy.


Thanks
Tiejun


indeed it needs to be dealt with at all).

Ian.



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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 +static void
 +add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
 +  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
 +{
 +d_config-num_rdms++;
 +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;
 +}


But, I wrote:

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

Note the last paragraph.

This is now the third time I have posted that text.  It is the fifth
request or clarification I have had to make about this very small
area.  I have to say that I'm finding this rather frustrating.

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

On 2015/7/21 23:09, Ian Jackson wrote:

Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM):

+static void
+add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
+  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
+{
+d_config-num_rdms++;
+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;
+}



But, I wrote:

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

Note the last paragraph.

This is now the third time I have posted that text.  It is the fifth
request or clarification I have had to make about this very small
area.  I have to say that I'm finding this rather frustrating.


Sorry, I just ignore the line in brackets since I always think this kind 
of thing is often not a big deal, and next time I should pay more 
attention to the (). But indeed, before I post this whole patch online I 
also picked up this chunk of code to ask you to take a look that. This 
manner means I'm not very sure if I'm addressing this properly. But I 
didn't get a further response, so I guess that should work for you and 
then I posted the whole online.


Now back on our problem,

static void
add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
{
d_config-rdms = libxl__realloc(NOGC, d_config-rdms,
(d_config-num_rdms+1) * sizeof(libxl_device_rdm));

d_config-rdms[d_config-num_rdms].start = rdm_start;
d_config-rdms[d_config-num_rdms].size = rdm_size;
d_config-rdms[d_config-num_rdms].policy = rdm_policy;
d_config-num_rdms++;
}

Does this work for you? If I'm still wrong, please correct this function 
directly to cost you less.


Thanks
Tiejun

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

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

2015-07-21 Thread Ian Jackson 
Ian Jackson writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
 conflicts with RDM):
  Sorry, I just ignore the line in brackets since I always think this kind 
  of thing is often not a big deal, and next time I should pay more 
  attention to the (). But indeed, before I post this whole patch online I 
  also picked up this chunk of code to ask you to take a look that. This 
  manner means I'm not very sure if I'm addressing this properly. But I 
  didn't get a further response, so I guess that should work for you and 
  then I posted the whole online.
 
 You are talking about 55ae2bb1.9030...@intel.com I guess.  I replied
 to that with several comments about your prose and about the
 computation of the new set of rdms.
 
 It's true that I didn't comment on the frat that you had half-done one
 fact
 of the things I had requested.  It is of course a waste of my time to
 be constantly re-reviewing half-done changes.

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

On 2015/7/21 23:57, Ian Jackson wrote:

Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM):

Sorry, I just ignore the line in brackets since I always think this kind
of thing is often not a big deal, and next time I should pay more
attention to the (). But indeed, before I post this whole patch online I
also picked up this chunk of code to ask you to take a look that. This
manner means I'm not very sure if I'm addressing this properly. But I
didn't get a further response, so I guess that should work for you and
then I posted the whole online.


You are talking about 55ae2bb1.9030...@intel.com I guess.  I replied
to that with several comments about your prose and about the
computation of the new set of rdms.

It's true that I didn't comment on the frat that you had half-done one
of the things I had requested.  It is of course a waste of my time to
be constantly re-reviewing half-done changes.


Next time I should see each line of all comments carefully. Maybe its 
good way to use IRC to take your quick advice in advance, and I hope 
this make you feel better. Anyway, sorry to bring this kind of 
inconvenience.





Now back on our problem,

static void
add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
{
  d_config-rdms = libxl__realloc(NOGC, d_config-rdms,
  (d_config-num_rdms+1) * sizeof(libxl_device_rdm));

  d_config-rdms[d_config-num_rdms].start = rdm_start;
  d_config-rdms[d_config-num_rdms].size = rdm_size;
  d_config-rdms[d_config-num_rdms].policy = rdm_policy;
  d_config-num_rdms++;
}

Does this work for you? If I'm still wrong, please correct this function
directly to cost you less.


Yes, that is what I meant.



Good to know.

Thanks
Tiejun

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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 [Ian Jackson:]
  That is, an address would be reserved if it was reserved in any of the
  rdm regions implied by the config.
 
 Are you saying this point?
 
 The union of two sets A and B is the set of elements which are in A, in 
 B, or in both A and B.

Yes.

  The explicitly specified regions might overlap with the computed ones,
  without being identical. Computing the union would not be entirely
  trivial.
 
 Just to your example,
 
libxl_domain_config cfg;
cfg.stuff = blah;
cfg.rdm.strategy = HOST;
 
libxl_domain_create_new(cfg, domid);
libxl_domain_destroy(domid);
 
 Here looks you mean d_config-rdms would be changed, right? Currently 
 this shouldn't be allowed. But I think we need to further discussion 
 make this case clear after feature freeze since we didn't have this kind 
 of assumption in our previous design.
 
libxl_domain_create_new(cfg, domid);

This response of yours does not lead me to think you have understood
what I am saying, but I agree that this can be dealt with later (if
indeed it needs to be dealt with at all).

Ian.

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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 Indeed, I'm not a fan to Xen tools so I can't picture what this real 
 scenario would happen. So if I'm misunderstanding what you mean, just 
 please correct me. Or if you still think its hard to explain this to me, 
 just tell me what I should do. I think this make your life easy.

Please ignore this line of discussion.

Instead, please simply make it so that if there are any rdms specified
in the domain config, they are used instead of the automatically
gathered information (from strategy and devices).

Thanks,
Ian.

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

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

2015-07-21 Thread Ian Jackson 
I wrote:
 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.)

But even your latest patch adds the host rdm's (from the strategy
algorithm) to the array, unconditionally.

I think you need to think more clearly about what happens when the
caller *both* supplies some rdms, and sets strategy=host.

Certainly if this happens and the set of rdms supplied is the same as
that which would be generated by the strategy, the set should not be
duplicated.

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

I hope the following can address all comments below:


diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 2f8e590..a4bd2a1 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -407,7 +407,7 @@ int libxl__domain_build(libxl__gc *gc,

 switch (info-type) {
 case LIBXL_DOMAIN_TYPE_HVM:
-ret = libxl__build_hvm(gc, domid, info, state);
+ret = libxl__build_hvm(gc, domid, d_config, state);
 if (ret)
 goto out;

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 634b8d2..ba852fe 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -92,6 +92,276 @@ const char *libxl__domain_device_model(libxl__gc *gc,
 return dm;
 }

+static int
+libxl__xc_device_get_rdm(libxl__gc *gc,
+ uint32_t flag,
+ uint16_t seg,
+ uint8_t bus,
+ uint8_t devfn,
+ unsigned int *nr_entries,
+ struct xen_reserved_device_memory **xrdm)
+{
+int rc = 0, r;
+
+/*
+ * We really can't presume how many entries we can get in advance.
+ */
+*nr_entries = 0;
+r = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+  NULL, nr_entries);
+assert(r = 0);
+/* 0 means we have no any rdm entry. */
+if (!r) goto out;
+
+if (errno != ENOBUFS) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+GCNEW_ARRAY(*xrdm, *nr_entries);
+r = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+  *xrdm, nr_entries);
+if (r)
+rc = ERROR_FAIL;
+
+ out:
+if (rc) {
+*nr_entries = 0;
+*xrdm = NULL;
+LOG(ERROR, Could not get reserved device memory maps.\n);
+}
+return rc;
+}
+
+/*
+ * Check whether there exists rdm hole in the specified memory range.
+ * Returns true if exists, else returns false.
+ */
+static bool overlaps_rdm(uint64_t start, uint64_t memsize,
+ uint64_t rdm_start, uint64_t rdm_size)
+{
+return (start + memsize  rdm_start)  (start  rdm_start + rdm_size);
+}
+
+static void
+add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
+  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
+{
+assert(d_config-num_rdms);
+
+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;
+}
+
+/*
+ * Check reported RDM regions and handle potential gfn conflicts according
+ * to user preferred policy.
+ *
+ * RDM can reside in address space beyond 4G theoretically, but we never
+ * see this in real world. So in order to avoid breaking highmem layout
+ * we don't solve highmem conflict. Note this means highmem rmrr could
+ * still be supported if no conflict.
+ *
+ * But in the case of lowmem, RDM probably scatter the whole RAM space.
+ * Especially multiple RDM entries would worsen this to lead a complicated
+ * memory layout. And then its hard to extend hvm_info_table{} to work
+ * hvmloader out. So here we're trying to figure out a simple solution to
+ * avoid breaking existing layout. So when a conflict occurs,
+ *
+ * #1. Above a predefined boundary (default 2G)
+ * - Move lowmem_end below reserved region to solve conflict;
+ *
+ * #2. Below a predefined boundary (default 2G)
+ * - Check strict/relaxed policy.
+ * strict policy leads to fail libxl.
+ * relaxed policy issue a warning message and also mask this entry
+ * INVALID to indicate we shouldn't expose this entry to hvmloader.
+ * Note when both policies are specified on a given region, the per-device
+ * policy should override the global policy.
+ */
+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)
+{
+int i, j, conflict, rc;
+struct xen_reserved_device_memory *xrdm = NULL;
+uint32_t strategy = d_config-b_info.u.hvm.rdm.strategy;
+uint16_t seg;
+uint8_t bus, devfn;
+uint64_t rdm_start, rdm_size;
+uint64_t highmem_end = args-highmem_end ? args-highmem_end : 
(1ull32);

+
+/* Might not expose rdm. */
+if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE 
+!d_config-num_pcidevs)
+return 0;
+
+/* Query all RDM entries in this platform */
+if (strategy == LIBXL_RDM_RESERVE_STRATEGY_HOST) {
+unsigned int nr_entries;
+
+/* Collect all rdm info if exist. */
+rc = libxl__xc_device_get_rdm(gc, PCI_DEV_RDM_ALL,
+  0, 0, 0, nr_entries, xrdm);
+

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 I would add this check at the beginning of this function:
 
 assert(!d_config-num_rdms  !d_config-rdms).

As Ian Campbell and I have explained, that is not OK.

The caller may choose to pass both some rdms and strategy=host.

The most obvious case is something like the following code

  libxl_domain_config cfg;
  cfg.stuff = blah;
  cfg.rdm.strategy = HOST;

  libxl_domain_create_new(cfg, domid);
  libxl_domain_destroy(domid);
  libxl_domain_create_new(cfg, domid);

which must work (and the second domain should have identical
configuration to the first).

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

The domain destroy would not change cfg at all.


Okay.




If not, I should double check this duplication so what about a return in
the case of duplicating one entry?


What I am looking for is a *decision* about what the right behaviour
is, backed up by a *rationale*.

The most obvious answer to me would be that if an rdms array is
specified, the strategy should be ignored.  That is how the automatic
numa placement API works.


I'm not familiar with this.

Do you mean this?

if (d_config-num_rdms)
strategy = LIBXL_RDM_RESERVE_STRATEGY_IGNORE;

But except this global strategy, we also have per-device setting so 
maybe something like this?


if (d_config-num_rdms)
return 0;



But another answer would be to take the union of the specified
regions.  That would be more complicated, because the union would have
to be computed.


  if (d_config-rdms[i].start == rdm_start)
  return;


That doesn't, of course, compute the union.



Sorry I don't understand what the take the union of the specified 
regions is here.


Thanks
Tiejun


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

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

2015-07-21 Thread Chen, Tiejun 

On 2015/7/21 19:11, Ian Jackson wrote:

Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM):

I would add this check at the beginning of this function:

assert(!d_config-num_rdms  !d_config-rdms).


As Ian Campbell and I have explained, that is not OK.

The caller may choose to pass both some rdms and strategy=host.

The most obvious case is something like the following code

   libxl_domain_config cfg;
   cfg.stuff = blah;
   cfg.rdm.strategy = HOST;

   libxl_domain_create_new(cfg, domid);
   libxl_domain_destroy(domid);


I'm not sure about this procedure, so do you mean this doesn't free 
d_config-num_rdms/d_config-rdms?


If not, I should double check this duplication so what about a return in 
the case of duplicating one entry?


static void
add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
{
if (d_config-rdms)
{
unsigned int i;

for (i = 0; i  d_config-num_rdms; i++)
{
if (d_config-rdms[i].start == rdm_start)
return;
}
}

d_config-num_rdms++;
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
Tiejun


   libxl_domain_create_new(cfg, domid);

which must work (and the second domain should have identical
configuration to the first).

Ian.



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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 On 2015/7/21 19:11, Ian Jackson wrote:
  The most obvious case is something like the following code
 
 libxl_domain_config cfg;
 cfg.stuff = blah;
 cfg.rdm.strategy = HOST;
 
 libxl_domain_create_new(cfg, domid);
 libxl_domain_destroy(domid);
 
 I'm not sure about this procedure, so do you mean this doesn't free 
 d_config-num_rdms/d_config-rdms?

The domain destroy would not change cfg at all.

 If not, I should double check this duplication so what about a return in 
 the case of duplicating one entry?

What I am looking for is a *decision* about what the right behaviour
is, backed up by a *rationale*.

The most obvious answer to me would be that if an rdms array is
specified, the strategy should be ignored.  That is how the automatic
numa placement API works.

But another answer would be to take the union of the specified
regions.  That would be more complicated, because the union would have
to be computed.

  if (d_config-rdms[i].start == rdm_start)
  return;

That doesn't, of course, compute the union.

Ian.

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

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

2015-07-21 Thread Chen, Tiejun 

On 2015/7/21 19:11, Ian Jackson wrote:

Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM):

I would add this check at the beginning of this function:

assert(!d_config-num_rdms  !d_config-rdms).


As Ian Campbell and I have explained, that is not OK.

The caller may choose to pass both some rdms and strategy=host.


If you're talking to create the domain twice I think this should be 
reasonable,


if (d_config-num_rdms) return 0;

Because RDMs are always associated to the platform so we should keep the 
same array. I mean same configuration shouldn't change this point.


Thanks
Tiejun



The most obvious case is something like the following code

   libxl_domain_config cfg;
   cfg.stuff = blah;
   cfg.rdm.strategy = HOST;

   libxl_domain_create_new(cfg, domid);
   libxl_domain_destroy(domid);
   libxl_domain_create_new(cfg, domid);

which must work (and the second domain should have identical
configuration to the first).

Ian.



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

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

2015-07-21 Thread Chen, Tiejun 

But d_config is a libxl_domain_config which is supplied by libxl's
caller.  It might contain some rdms.


I guess this line make you or other guys confused so lets delete this
line directly.


I don't think I am very confused.


And if you still worry about something, I can add assert() at the
beginning of this function like this,

assert(!d_config-num_rdms  !d_config-rdms).


If you are sure that this assertion is correct, then that would be
proper.

But as I say above, I don't think it is.



Based on Campbell' explanation I think you guys are raising a reasonable 
concern. We shouldn't clear that over there arbitrarily.


Thanks
Tiejun


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

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

2015-07-21 Thread Chen, Tiejun 

  I think the confusion here is that the d_config-rdms array (which

num_rdms is the length of) is in the public API (because it is in
libxl_types.idl) but is apparently only being used in this series as an
internal state for the domain build process (i.e. xl doesn't ever add
anything to the array rdms).

Tiejun, is that an accurate summary?


Yes.



If the field is in the public API then the possibility of something
being passed in their must be considered now, even if this particular
series adds no such calls, since we cannot prevent 3rd party users of
libxl adding such configuration.

Is the possibility of the toolstack (i.e. the caller of libxl) supplying
an array of rdm regions seems to be being left aside for future work or
it not intended to ever support that?


Its very possible so you're right.
Thanks
Tiejun



Ian.




And if you still worry about something, I can add assert() at the
beginning of this function like this,

assert(!d_config-num_rdms  !d_config-rdms).


If you are sure that this assertion is correct, then that would be
proper.

But as I say above, I don't think it is.

Ian.






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

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

2015-07-21 Thread Chen, Tiejun 

On 2015/7/21 20:33, Ian Jackson wrote:

Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts 
with RDM):

[Ian Jackson:]

The most obvious answer to me would be that if an rdms array is
specified, the strategy should be ignored.  That is how the automatic
numa placement API works.


I'm not familiar with this.

Do you mean this?

  if (d_config-num_rdms)
  strategy = LIBXL_RDM_RESERVE_STRATEGY_IGNORE;

But except this global strategy, we also have per-device setting so
maybe something like this?

  if (d_config-num_rdms)
  return 0;


This latter was the kind of approach I had in mind.  I think that will
do for 4.6.



Thanks for your guideline on this patch, and also I realize lots of 
things need to be improved. So I'm going to make a TODO list publically 
after this feature freeze.


Now just for this patch, please take a final review (I just hope so.)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 2f8e590..a4bd2a1 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -407,7 +407,7 @@ int libxl__domain_build(libxl__gc *gc,

 switch (info-type) {
 case LIBXL_DOMAIN_TYPE_HVM:
-ret = libxl__build_hvm(gc, domid, info, state);
+ret = libxl__build_hvm(gc, domid, d_config, state);
 if (ret)
 goto out;

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 634b8d2..3afa735 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -92,6 +92,280 @@ const char *libxl__domain_device_model(libxl__gc *gc,
 return dm;
 }

+static int
+libxl__xc_device_get_rdm(libxl__gc *gc,
+ uint32_t flag,
+ uint16_t seg,
+ uint8_t bus,
+ uint8_t devfn,
+ unsigned int *nr_entries,
+ struct xen_reserved_device_memory **xrdm)
+{
+int rc = 0, r;
+
+/*
+ * We really can't presume how many entries we can get in advance.
+ */
+*nr_entries = 0;
+r = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+  NULL, nr_entries);
+assert(r = 0);
+/* 0 means we have no any rdm entry. */
+if (!r) goto out;
+
+if (errno != ENOBUFS) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+GCNEW_ARRAY(*xrdm, *nr_entries);
+r = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+  *xrdm, nr_entries);
+if (r)
+rc = ERROR_FAIL;
+
+ out:
+if (rc) {
+*nr_entries = 0;
+*xrdm = NULL;
+LOG(ERROR, Could not get reserved device memory maps.\n);
+}
+return rc;
+}
+
+/*
+ * Check whether there exists rdm hole in the specified memory range.
+ * Returns true if exists, else returns false.
+ */
+static bool overlaps_rdm(uint64_t start, uint64_t memsize,
+ uint64_t rdm_start, uint64_t rdm_size)
+{
+return (start + memsize  rdm_start)  (start  rdm_start + rdm_size);
+}
+
+static void
+add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config,
+  uint64_t rdm_start, uint64_t rdm_size, int rdm_policy)
+{
+d_config-num_rdms++;
+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;
+}
+
+/*
+ * Check reported RDM regions and handle potential gfn conflicts according
+ * to user preferred policy.
+ *
+ * RDM can reside in address space beyond 4G theoretically, but we never
+ * see this in real world. So in order to avoid breaking highmem layout
+ * we don't solve highmem conflict. Note this means highmem rmrr could
+ * still be supported if no conflict.
+ *
+ * But in the case of lowmem, RDM probably scatter the whole RAM space.
+ * Especially multiple RDM entries would worsen this to lead a complicated
+ * memory layout. And then its hard to extend hvm_info_table{} to work
+ * hvmloader out. So here we're trying to figure out a simple solution to
+ * avoid breaking existing layout. So when a conflict occurs,
+ *
+ * #1. Above a predefined boundary (default 2G)
+ * - Move lowmem_end below reserved region to solve conflict;
+ *
+ * #2. Below a predefined boundary (default 2G)
+ * - Check strict/relaxed policy.
+ * strict policy leads to fail libxl.
+ * relaxed policy issue a warning message and also mask this entry
+ * INVALID to indicate we shouldn't expose this entry to hvmloader.
+ * Note when both policies are specified on a given region, the per-device
+ * policy should override the global policy.
+ */
+int libxl__domain_device_construct_rdm(libxl__gc *gc,
+   libxl_domain_config *d_config,
+

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

2015-07-21 Thread Chen, Tiejun 

But another answer would be to take the union of the specified
regions.  That would be more complicated, because the union would have
to be computed.


   if (d_config-rdms[i].start == rdm_start)
   return;


That doesn't, of course, compute the union.


Sorry I don't understand what the take the union of the specified
regions is here.


A region is a subset of the address space.  I meant the set union:
   https://en.wikipedia.org/wiki/Union_%28set_theory%29

That is, an address would be reserved if it was reserved in any of the
rdm regions implied by the config.


Are you saying this point?

The union of two sets A and B is the set of elements which are in A, in 
B, or in both A and B.




The explicitly specified regions might overlap with the computed ones,
without being identical. Computing the union would not be entirely
trivial.



Just to your example,

  libxl_domain_config cfg;
  cfg.stuff = blah;
  cfg.rdm.strategy = HOST;

  libxl_domain_create_new(cfg, domid);
  libxl_domain_destroy(domid);

Here looks you mean d_config-rdms would be changed, right? Currently 
this shouldn't be allowed. But I think we need to further discussion 
make this case clear after feature freeze since we didn't have this kind 
of assumption in our previous design.


  libxl_domain_create_new(cfg, domid);

Thanks
Tiejun

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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 If you're talking to create the domain twice I think this should be 
 reasonable,

Right.

 if (d_config-num_rdms) return 0;

Yes.

 Because RDMs are always associated to the platform so we should keep the 
 same array. I mean same configuration shouldn't change this point.

Yes.

Thanks,
Ian.

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

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

2015-07-21 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 [Ian Jackson:]
  The most obvious answer to me would be that if an rdms array is
  specified, the strategy should be ignored.  That is how the automatic
  numa placement API works.
 
 I'm not familiar with this.
 
 Do you mean this?
 
  if (d_config-num_rdms)
  strategy = LIBXL_RDM_RESERVE_STRATEGY_IGNORE;
 
 But except this global strategy, we also have per-device setting so 
 maybe something like this?
 
  if (d_config-num_rdms)
  return 0;

This latter was the kind of approach I had in mind.  I think that will
do for 4.6.


  But another answer would be to take the union of the specified
  regions.  That would be more complicated, because the union would have
  to be computed.
 
if (d_config-rdms[i].start == rdm_start)
return;
 
  That doesn't, of course, compute the union.
 
 Sorry I don't understand what the take the union of the specified 
 regions is here.

A region is a subset of the address space.  I meant the set union:
  https://en.wikipedia.org/wiki/Union_%28set_theory%29

That is, an address would be reserved if it was reserved in any of the
rdm regions implied by the config.

The explicitly specified regions might overlap with the computed ones,
without being identical. Computing the union would not be entirely
trivial.


Ian.

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

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

2015-07-20 Thread Chen, Tiejun 


Note I need more time to address others.


+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


We don't have the specific num_rdms parameter in .cfg so I don't 
understand what you mean here.


Thanks
Tiejun


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





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

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

2015-07-20 Thread Ian Jackson 
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@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-07-20 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and avoid 
conflicts with RDM):
 Note I need more time to address others.

Right.  But thanks for coming back quickly with this question:

  +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
 
 We don't have the specific num_rdms parameter in .cfg so I don't 
 understand what you mean here.

The domain configuration specified to libxl might contain some rdms.
Then num_rdms in the incoming config would be nonzero.

So I think there are two problems here:

1. If that were the case you would leak the application's rdms array.

2. Anyway, if the caller specifies such an array you should use it.
   (Fixing this would avoid (1) in any case.)

Ian.

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

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

2015-07-20 Thread Chen, Tiejun 

+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


We don't have the specific num_rdms parameter in .cfg so I don't
understand what you mean here.


The domain configuration specified to libxl might contain some rdms.
Then num_rdms in the incoming config would be nonzero.


We never set d_config-num_rdms/d_config-rdms before we goes inside 
libxl__domain_device_construct_rdm(). And actually 
libxl__domain_device_construct_rdm is only one place to set 
d_config-num_rdms/d_config-rdms.


I guess this line make you or other guys confused so lets delete this 
line directly.


And if you still worry about something, I can add assert() at the 
beginning of this function like this,


assert(!d_config-num_rdms  !d_config-rdms).

Thanks
Tiejun



So I think there are two problems here:

1. If that were the case you would leak the application's rdms array.

2. Anyway, if the caller specifies such an array you should use it.
(Fixing this would avoid (1) in any case.)

Ian.



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

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

2015-07-20 Thread Ian Jackson 
Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and
avoid conflicts with RDM):
 [Ian Jackson:]
  The domain configuration specified to libxl might contain some rdms.
  Then num_rdms in the incoming config would be nonzero.
 
 We never set d_config-num_rdms/d_config-rdms before we goes inside 
 libxl__domain_device_construct_rdm(). And actually 
 libxl__domain_device_construct_rdm is only one place to set 
 d_config-num_rdms/d_config-rdms.

But d_config is a libxl_domain_config which is supplied by libxl's
caller.  It might contain some rdms.

 I guess this line make you or other guys confused so lets delete this 
 line directly.

I don't think I am very confused.

 And if you still worry about something, I can add assert() at the 
 beginning of this function like this,
 
 assert(!d_config-num_rdms  !d_config-rdms).

If you are sure that this assertion is correct, then that would be
proper.

But as I say above, I don't think it is.

Ian.

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

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

2015-07-20 Thread Ian Campbell 
On Mon, 2015-07-20 at 16:24 +0100, Ian Jackson wrote:
 Chen, Tiejun writes (Re: [v10][PATCH 11/16] tools/libxl: detect and
 avoid conflicts with RDM):
  [Ian Jackson:]
   The domain configuration specified to libxl might contain some rdms.
   Then num_rdms in the incoming config would be nonzero.
  
  We never set d_config-num_rdms/d_config-rdms before we goes inside 
  libxl__domain_device_construct_rdm(). And actually 
  libxl__domain_device_construct_rdm is only one place to set 
  d_config-num_rdms/d_config-rdms.
 
 But d_config is a libxl_domain_config which is supplied by libxl's
 caller.  It might contain some rdms.
 
  I guess this line make you or other guys confused so lets delete this 
  line directly.
 
 I don't think I am very confused.

I think the confusion here is that the d_config-rdms array (which
num_rdms is the length of) is in the public API (because it is in
libxl_types.idl) but is apparently only being used in this series as an
internal state for the domain build process (i.e. xl doesn't ever add
anything to the array rdms).

Tiejun, is that an accurate summary?

If the field is in the public API then the possibility of something
being passed in their must be considered now, even if this particular
series adds no such calls, since we cannot prevent 3rd party users of
libxl adding such configuration.

Is the possibility of the toolstack (i.e. the caller of libxl) supplying
an array of rdm regions seems to be being left aside for future work or
it not intended to ever support that?

Ian.

 
  And if you still worry about something, I can add assert() at the 
  beginning of this function like this,
  
  assert(!d_config-num_rdms  !d_config-rdms).
 
 If you are sure that this assertion is correct, then that would be
 proper.
 
 But as I say above, I don't think it is.
 
 Ian.



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