From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map Date: Tue, 02 Sep 2014 09:34:05 +0100 Message-ID: <54059D1D020000780002FBA5@mail.emea.novell.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-7-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1409050980-21933-7-git-send-email-tiejun.chen@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org >>> On 26.08.14 at 13:02, wrote: > libxc can expose how many reserved device memory entries > hvmloader should get. And '0' means that doesn't exist so > we can skip this check. I assume you introduce this without consumer to limit patch size. In such a case title _and_ description should be more meaningful as to what this really does and what it's intended use is. > @@ -370,6 +375,9 @@ static int setup_guest(xc_interface *xch, > rc = check_rmrr_overlap(xch, mmio_start, mmio_start); I skipped several tools side patches, but here I see that somewhere you still left the term "RMRR" in the code, when you were asked before to use more abstract naming (and this of course not only extended to the public interface). > if ( rc < 0 ) > goto error_out; > + /* Always return entries number. */ > + else The "else" here is bogus considering the "goto" above. > --- a/xen/include/public/hvm/hvm_info_table.h > +++ b/xen/include/public/hvm/hvm_info_table.h > @@ -67,6 +67,9 @@ struct hvm_info_table { > > /* Bitmap of which CPUs are online at boot time. */ > uint8_t vcpu_online[(HVM_MAX_VCPUS + 7)/8]; > + > + /* How many reserved device memory does this domain have? */ > + uint32_t nr_reserved_device_memory_map; > }; This being defacto a private interface between tools and hvmloader I wonder if it wouldn't be better to put this before the (in the future) eventually growing vcpu_online[]. Jan