From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings Date: Wed, 03 Sep 2014 17:01:15 +0800 Message-ID: <5406D8DB.7020503@intel.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-4-git-send-email-tiejun.chen@intel.com> <53FC774D.8000501@citrix.com> <53FC9BB1020000780002D986@mail.emea.novell.com> <53FD3669.3070705@intel.com> <53FD9C03020000780002DEFF@mail.emea.novell.com> <53FD86E9.4020205@intel.com> <53FE92C6.6050605@intel.com> <53FEED4F020000780002E76C@mail.emea.novell.com> <53FED5AF.7080803@intel.com> <53FED802.7080707@intel.com> <53FEF9F8020000780002E7E2@mail.emea.novell.com> <53FFED5D.7040406@intel.com> <54006196020000780002EFC6@mail.emea.novell.com> <54043FEA.6080100@intel.com> <540466A8020000780002F656@mail.emea.novell.com> <540594F2.4000406@intel.com> <5405B4E1020000780002FCBC@mail.emea.novell.com> <5405A597.9040103@intel.com> <5405DF00020000780002FD52@mail.emea.novell.com> <540672A7.9050000@intel.com> <5406D1E1.3050601@intel.com> <5406F0470200007800030190@mail.emea.novell.com> <5406D856.6040406@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5406D856.6040406@intel.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 Cooper , 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/3 16:59, Chen, Tiejun wrote: > On 2014/9/3 16:41, Jan Beulich wrote: >>>>> On 03.09.14 at 10:31, wrote: >>> On 2014/9/3 9:45, Chen, Tiejun wrote: >>>> On 2014/9/2 21:15, Jan Beulich wrote: >>>>>>>> On 02.09.14 at 13:10, wrote: >>>>>> On 2014/9/2 18:15, Jan Beulich wrote: >>>>>>>>>> On 02.09.14 at 11:59, wrote: >>>>>>>> + case XENMEM_reserved_device_memory_map: >>>>>>>> + { >>>>>>>> + struct xen_mem_reserved_device_memory *xmrdm = NULL; >>>>>>>> + struct xen_mem_reserved_device_memory_map xmrdmm; >>>>>>>> + XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer; >>>>>>>> + XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) >>>>>>>> buffer_param; >>>>>>>> + const struct iommu_ops *ops = iommu_get_ops(); >>>>>>>> + unsigned int nr_entries = 0; >>>>>>>> + unsigned int i = 0; >>>>>>>> + >>>>>>>> + xmrdm = ops->get_device_reserved_memory(&nr_entries); >>>>>> >>>>>> Do we still need this iommu_ops somewhere else? >>>>> >>>>> Not this one, but another one (as I had described further down). >>>>> >>>>>>>> + if ( !nr_entries ) >>>>>> >>>>>> Do we still need this 'nr_entries' here? >>>>> >>>>> Doesn't look like so. But it's you coding this up - you ought to >>>>> clearly >>>>> see what is needed and what not. >>>>> >>>> >>>> I mean we need to get 'nr_entries' before any real operations since if >>>> that is zero, any following operations are pointless. And even that is >>>> nonzero, but actually the user don't know how many buffer should be >>>> passed, so often we may need to notify user again at the first time >>>> then >>>> the user call this again with a appropriate buffer. Right? >>>> >>>> So how do we get this 'nr_entries'? Seems we have to to go VT-D >>>> stuff to >>>> walk that list or other similar approaches. If so, why we still get >>>> those real mapping entries later by accessing VT-D stuff again? >>>> >>>> Shouldn't we figure out a approach we just call one time? >>>> >>> >>> I update this patch based on this point: >> >> I'm afraid I have to give up and instead go and implement this for >> you (which already by now would clearly have been the much less >> time consuming thing at least on my end). >> > > I think I should cover your latest comment: I mean last revision I pasted already covered your latest comment. Thanks Tiejun > > "> Just go look for other examples of similar operations: There's > > absolutely no need to know the number of entries up front. Just > > have a way for the callback function to signal its caller that it > > can abort the loop and return right away if you really need this. > > But I doubt you do, since even when the caller has provided too > > little buffer space you still want to tell it how much is needed. And > > the necessary counting would still be done in the callback function. > > > > Jan > " > #1 Don't need to know entries in advance > #2 Just copy the entries as the caller but return a -ENOBUFS > > + case XENMEM_reserved_device_memory_map: > + { > + struct xen_mem_reserved_device_memory_map xmrdmmap; > + > + if ( copy_from_guest(&xmrdmmap, arg, 1) ) > + return -EFAULT; > + > + rc = copy_drmmap_to_guest(do_copy_device_reserved_memory, > &xmrdmmap); > + > + if ( __copy_to_guest(arg, &xmrdmmap, 1) ) > + rc = -EFAULT; > + > + return rc; > + } > + > > And > > +int > +do_copy_device_reserved_memory(struct > xen_mem_reserved_device_memory_map *xmrdmmap) > +{ > + struct acpi_rmrr_unit *rmrru; > + struct xen_mem_reserved_device_memory xmrdm; > + int i = 0, rc = 0; > + XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param; > + XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer; > + > + if ( list_empty(&acpi_rmrr_units) ) > + return -ENOENT; > + > + buffer_param = guest_handle_cast(xmrdmmap->buffer, > + xen_mem_reserved_device_memory_t); > + buffer = guest_handle_from_param(buffer_param, > + xen_mem_reserved_device_memory_t); > + if ( !guest_handle_okay(buffer, xmrdmmap->nr_entries) ) > + return -EFAULT; > + > + list_for_each_entry(rmrru, &acpi_rmrr_units, list) > + { > + if ( i < xmrdmmap->nr_entries ) > + { > + xmrdm.start_pfn = rmrru->base_address >> PAGE_SHIFT; > + xmrdm.nr_pages = PAGE_ALIGN(rmrru->end_address - > + rmrru->base_address) / > + PAGE_SIZE; > + if ( __copy_to_guest_offset(buffer, i, &xmrdm, 1) ) > + return -EFAULT; > + } > + else > + rc = -ENOBUFS; > + i++; > + } > + > + xmrdmmap->nr_entries = i; > + > + return rc; > +} > + > > Thanks > Tiejun