From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 2/5] xen:x86: introduce a new hypercall to get RMRR mappings Date: Tue, 12 Aug 2014 18:55:41 +0800 Message-ID: <53E9F2AD.1090505@intel.com> References: <1407409371-31728-1-git-send-email-tiejun.chen@intel.com> <1407409371-31728-3-git-send-email-tiejun.chen@intel.com> <53E50CB4020000780002AB34@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: <53E50CB4020000780002AB34@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, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/8/8 23:45, Jan Beulich wrote: >>>> On 07.08.14 at 13:02, wrote: >> + case XENMEM_RMRR_memory_map: >> + { >> + struct memory_map_context ctxt; > > ??? > >> + XEN_GUEST_HANDLE(e820entry_t) buffer; >> + XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param; >> + unsigned int i; >> + >> + rc = xsm_machine_memory_map(XSM_PRIV); > > Are you sure? Can (and should) this really not be exposed to semi- > privileged domains? Will fixed. > >> + if ( rc ) >> + return rc; >> + >> + if ( copy_from_guest(&ctxt.map, arg, 1) ) >> + return -EFAULT; >> + if ( ctxt.map.nr_entries < rmrr_e820.nr_map + 1 ) >> + return -EINVAL; > > So how would the caller know how many entries are needed? > >> + >> + buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t); >> + buffer = guest_handle_from_param(buffer_param, e820entry_t); >> + if ( !guest_handle_okay(buffer, ctxt.map.nr_entries) ) >> + return -EFAULT; >> + >> + for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < rmrr_e820.nr_map; ++i, ++ctxt.n ) > > i and ctxt.n are redundant. > >> + { >> + if ( __copy_to_guest_offset(buffer, ctxt.n, rmrr_e820.map + i, 1) ) >> + return -EFAULT; >> + } >> + >> + ctxt.map.nr_entries = ctxt.n; >> + >> + if ( __copy_to_guest(arg, &ctxt.map, 1) ) > > __copy_field_to_guest() if all you need to copy back is a single field. I will try to address all comments in next revision. > >> --- a/xen/arch/x86/x86_64/compat/mm.c >> +++ b/xen/arch/x86/x86_64/compat/mm.c >> @@ -132,6 +132,14 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> break; >> } >> >> + case XENMEM_RMRR_memory_map: >> + { >> + if ( copy_to_guest(arg, &rmrr_e820, 1) ) >> + return -EFAULT; >> + >> + return 0; >> + } > > Pointless braces. And how come this is so much simpler than the > native version? Just hvmloader would walk this with a hypercall, and with a test I don't see any issue here. If you think this is not correct, please comment this in next revision. > >> --- a/xen/include/public/memory.h >> +++ b/xen/include/public/memory.h >> @@ -523,7 +523,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t); >> >> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ >> >> -/* Next available subop number is 26 */ >> +/* >> + * Returns the RMRR memory map as it was when the domain >> + * was started. >> + */ >> +#define XENMEM_RMRR_memory_map 26 >> +typedef struct e820map rmrr_e820_t; >> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t); > > Again just as a general remark: What in the world does the "e820" > in here mean? I will redefine a struct to represent this to avoid any confusion. Thanks Tiejun > > Jan > >