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 16:59:02 +0800 Message-ID: <5406D856.6040406@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5406F0470200007800030190@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 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: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: "> 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