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

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel