From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v8][PATCH 07/17] hvmloader/util: get reserved device memory maps Date: Mon, 08 Dec 2014 16:09:28 +0800 Message-ID: <54855CB8.7010009@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-8-git-send-email-tiejun.chen@intel.com> <20141202200100.GG357@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141202200100.GG357@laptop.dumpdata.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: Konrad Rzeszutek Wilk Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/12/3 4:01, Konrad Rzeszutek Wilk wrote: > On Mon, Dec 01, 2014 at 05:24:25PM +0800, Tiejun Chen wrote: >> We need to use reserved device memory maps with multiple times, so >> provide just one common function should be friend. > > We need to call reserved device memory maps hypercall > (XENMEM_reserved_device_memory_map) many times, hence provide one common function. Rephrased and thanks. > >> >> Signed-off-by: Tiejun Chen >> --- >> tools/firmware/hvmloader/util.c | 59 +++++++++++++++++++++++++++++++++++++++++ >> tools/firmware/hvmloader/util.h | 2 ++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c >> index 80d822f..dd81fb6 100644 >> --- a/tools/firmware/hvmloader/util.c >> +++ b/tools/firmware/hvmloader/util.c >> @@ -22,11 +22,14 @@ >> #include "config.h" >> #include "hypercall.h" >> #include "ctype.h" >> +#include "errno.h" >> #include >> #include >> #include >> #include >> >> +struct xen_reserved_device_memory *rdm_map; >> + >> void wrmsr(uint32_t idx, uint64_t v) >> { >> asm volatile ( >> @@ -828,6 +831,62 @@ int hpet_exists(unsigned long hpet_base) >> return ((hpet_id >> 16) == 0x8086); >> } >> >> +static int >> +get_reserved_device_memory_map(struct xen_reserved_device_memory entries[], >> + uint32_t *max_entries) >> +{ >> + int rc; >> + struct xen_reserved_device_memory_map xrdmmap = { >> + .domid = DOMID_SELF, >> + .nr_entries = *max_entries >> + }; >> + >> + set_xen_guest_handle(xrdmmap.buffer, entries); >> + >> + rc = hypercall_memory_op(XENMEM_reserved_device_memory_map, &xrdmmap); >> + *max_entries = xrdmmap.nr_entries; > > Don't you want to check rc first before altering 'max_entries' ? - *max_entries = xrdmmap.nr_entries; + if ( rc == -ENOBUFS ) + *max_entries = xrdmmap.nr_entries; > >> + >> + return rc; >> +} >> + >> +/* >> + * Getting all reserved device memory map info in case of hvmloader. >> + * We just return zero for any failed cases, and this means we >> + * can't further handle any reserved device memory. > > That does not sound like the right error value. Why not a proper > return value? At worst you can put 'nr_entries' as an parameter > and return the error value. Any caller to use hvm_get_reserved_device_memory_map() don't care that real return value, the caller can work just with a return value as unsigned int. '0' means we have nothing to do, '>0' should be good to handle. > >> + */ >> +unsigned int hvm_get_reserved_device_memory_map(void) >> +{ >> + static unsigned int nr_entries = 0; >> + int rc = get_reserved_device_memory_map(rdm_map, &nr_entries); >> + >> + if ( rc == -ENOBUFS ) >> + { >> + rdm_map = mem_alloc(nr_entries*sizeof(struct xen_reserved_device_memory), > > That '*' being squashed looks wrong. Just make it bigger and don't worry about > the 80 line. - rdm_map = mem_alloc(nr_entries*sizeof(struct xen_reserved_device_memory), + rdm_map = mem_alloc(nr_entries * sizeof(struct xen_reserved_device_memory), Thanks Tiejun > >> + 0); >> + if ( rdm_map ) >> + { >> + rc = get_reserved_device_memory_map(rdm_map, &nr_entries); >> + if ( rc ) >> + { >> + printf("Could not get reserved dev memory info on domain"); >> + return 0; >> + } >> + } >> + else >> + { >> + printf("No space to get reserved dev memory maps!\n"); >> + return 0; >> + } >> + } >> + else if ( rc ) >> + { >> + printf("Could not get reserved dev memory info on domain"); >> + return 0; >> + } >> + >> + return nr_entries; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h >> index a70e4aa..e4f1851 100644 >> --- a/tools/firmware/hvmloader/util.h >> +++ b/tools/firmware/hvmloader/util.h >> @@ -241,6 +241,8 @@ int build_e820_table(struct e820entry *e820, >> unsigned int bios_image_base); >> void dump_e820_table(struct e820entry *e820, unsigned int nr); >> >> +unsigned int hvm_get_reserved_device_memory_map(void); >> + >> #ifndef NDEBUG >> void perform_tests(void); >> #else >> -- >> 1.9.1 >> >