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

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

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

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