From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map Date: Thu, 04 Sep 2014 10:07:45 +0800 Message-ID: <5407C971.3090807@intel.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-7-git-send-email-tiejun.chen@intel.com> <54059D1D020000780002FBA5@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54059D1D020000780002FBA5@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich 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 2014/9/2 16:34, Jan Beulich wrote: >>>> 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. This patch involves two files, one is xen file, xen/include/public/hvm/hvm_info_table.h, and a tools file, tools/libxc/xc_hvm_build_x86.c. So I'm considering to split with two small patches like this: #1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map In hvm_info_table this field represents the number of all device memory maps. It will be convenient to expose such a information to VM. #2 Construct this field in tools/libxc/xc_hvm_build_x86.c tools/libxc: construct nr_device_reserved_memory_map While building hvm info, libxc is responsible for constructing this number after check_drm_overlap(). Is it reasonable to use different patches covering xen internal and tools, respectively? Or just one is already fine? > >> @@ -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). I will replace those info with 'device reserved memory maps' and s/RMRR/DRM. > >> if ( rc < 0 ) >> goto error_out; >> + /* Always return entries number. */ >> + else > > The "else" here is bogus considering the "goto" above. Remove this 'else'. > >> --- 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[]. > Any latest consideration? Could we push line above 'uint8_t vcpu_online[(HVM_MAX_VCPUS + 7)/8];'? And I just think it may be reasonable and convenient to expose this info in hvm_info_table since this is a fixed value that we should record for all VMs. Thanks Tiejun