Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/9/3 16:41, Jan Beulich wrote:
>>>> On 03.09.14 at 10:31, <tiejun.chen@intel.com> 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, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> 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