Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

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

On 2014/8/27 10:40, Chen, Tiejun wrote:
> On 2014/8/27 10:20, Ian Campbell wrote:
>> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
>>> On 2014/8/27 4:36, Ian Campbell wrote:
>>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>>
>>>>> +    /* We should check if mmio range is out of RMRR mapping.
>>>>> +     *
>>>>> +     * Assume we have one entry if not enough we'll expand.
>>>>> +     */
>>>>
>>>> The usual approach with such hypervisor interfaces (which I suppose
>>>> xc_reserved_device_memory_map turns into) is to first call it with NULL
>>>> to get the required size and then allocate a suitable buffer and call a
>>>> second time.
>>>
>>> Ofentimes, RMRR should be rare with one or two entries, even zero.
>>
>> It's not clear to me what number you are saying is the norm here.
>
> In my broadwell platform we just have two entries.
>
>>
>> Even if some N is common today what guarantee is there that N won't grow
>> or shrink with the next generation of systems?
>
> As I understand RMRR may be legacy, and this should go away.
>
> What is RMRR?
> -------------
>
> There are some devices the BIOS controls, for e.g USB devices to perform
> PS2 emulation. The regions of memory used for these devices are marked
> reserved in the e820 map. When we turn on DMA translation, DMA to those
> regions will fail. Hence BIOS uses RMRR to specify these regions along
> with devices that need to access these regions. OS is expected to setup
> unity mappings for these regions for these devices to access these regions.
>
>>
>>>   So I
>>> think its reasonable to start posting one entry since this can cover
>>> such a scenario the platform really owns one entry.
>>
>> Making the call twice is not terribly expensive (nor is this a hot path)
>> and it allows you to avoid the reallocation and recall and the twisty
>> error handling structure which that implies.
>>
>
> As I understand when we call one given hypercall, we may know that
> possible numbers to issue that. Then we can get the appropriate number
> via the returned value if that is not enough. I think its better than we
> always issue twice hypercall unconditionally :)
>
> But if you persist in this fixed twice-call mechanism, I can try to
> rework out this implementation :)
>

And actually, I think current implementation is already as you expect. 
Please check patch #3,

@@ -4842,6 +4843,55 @@ long arch_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
          return rc;
      }

+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_mem_reserved_device_memory_map map;
+        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) 
buffer_param;
+        unsigned int i = 0;
+        struct xen_mem_reserved_device_memory rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( !acpi_rmrr_unit_entries )
+                return -ENOENT;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( map.nr_entries < acpi_rmrr_unit_entries )
+        {
+            map.nr_entries = acpi_rmrr_unit_entries;
+            if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }

And turn back, here I just set map.nr_entries = 1, not '0' as you image.

+    /* Assume we have one entry if not enough we'll expand.*/
+    uint32_t nr_entries = 1;
+
+    /* We should check if mmio range is out of RMRR mapping. */
+    if ( (map = malloc(nr_entries *
+                       sizeof(xen_mem_reserved_device_memory_t))) == NULL )

So you really hope I should set map.nr_entries = 0 firstly?

Thanks
Tiejun