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 17:01, Chen, Tiejun wrote:
> 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@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:
>
> I mean last revision I pasted already covered your latest comment.

I guess finally you will "go and implement this" as you said. So thanks 
for your time and guidance on this series of RFC patches.

Once you finish it, please tell me then I can test them.

Thanks
Tiejun

>
> 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