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:59, Chen, Tiejun wrote:
> 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:
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel