Re: [Xen-devel] [v6][PATCH 3/7] tools/libxc: check if mmio BAR is out of reserved device memory maps

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

On 2014/9/11 23:38, Jan Beulich wrote:
> > > > On 10.09.14 at 07:49, <tiejun.chen@xxxxxxxxx> wrote:
> > +static int check_rdm_overlap(xc_interface *xch, uint64_t mmio_start,
> > +                             uint64_t mmio_size)
> > +{
> > +    struct xen_mem_reserved_device_memory *map = NULL;
> > +    uint64_t rdm_start = 0, rdm_end = 0;
> > +    unsigned int i = 0;
> > +    int rc = 0;
> > +    /* 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 RDM mapping. */
> > +    if ( (map = malloc(nr_entries *
> > +                       sizeof(xen_mem_reserved_device_memory_t))) == NULL )
> > +    {
> > +        PERROR("Could not allocate memory for map.");
> > +        return -1;
> > +    }
> > +    rc = xc_reserved_device_memory_map(xch, map, &nr_entries);
> > +    if ( rc < 0 )
> > +    {
> > +        /* DRM doesn't exist. */
> > +        if ( rc == -ENOENT )
> > +            rc = 0;
> > +        else if ( rc == -ENOBUFS)
> I think you'd be better off handling both if() levels we're in here
> with a single switch().
Okay.

> > +        {
> > +            free(map);
> > +            /* Now we need more space to map all RDM mappings. */
> > +            if ( (map = malloc(nr_entries *
> > +                               sizeof(xen_mem_reserved_device_memory_t))) == 
> > NULL )
> > +            {
> > +                PERROR("Could not allocate memory for map.");
> > +                return -1;
> > +            }
> > +            rc = xc_reserved_device_memory_map(xch, map, &nr_entries);
> > +            if ( rc < 0 )
> > +            {
> > +                PERROR("Could not get DRM info on domain");
> Please change to RDM here and elsewhere.
Okay.

> > +                free(map);
> > +                return rc;
> > +            }
> > +        }
> > +        else
> > +            PERROR("Could not get RDM info on domain");
> Not returning here means looping for almost 4G iterations below.
>
> > +    }
> > +
> > +    for ( i = 0; i < rc; i++ )
> > +    {
> > +        rdm_start = map[i].start_pfn << PAGE_SHIFT;
> > +        rdm_end = rdm_start + map[i].nr_pages * PAGE_SIZE;
> > +        if ( check_mmio_hole(rdm_start, map[i].nr_pages * PAGE_SIZE,
> First of all please be consistent: Either always use "<< PAGE_SHIFT"
> or always use "* PAGE_SIZE". And then what you're doing here won't
> do what you intend to in a 32-bit toolstack (due to the lack of casts).
> Plus finally I think using XC_PAGE_* would be the better choice here
> even if PAGE_* are #define-d as aliases thereof.
>
> > +                             mmio_start, mmio_size) )
> Can you really use check_mmio_hole() here? I'm not certain that
> its two one-offs are intentional (to allow for a gap between MMIO
> and RAM), but these are certainly harmful to you.
I don't understand what you mean here. But here we should check if RMRR 
is overlapping the whole MMIO,
/*
 * Check whether there exists mmio hole in the specified memory range.
 * Returns 1 if exists, else returns 0.
 */
static int check_mmio_hole(uint64_t start, uint64_t memsize,
                           uint64_t mmio_start, uint64_t mmio_size)
{
    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
        return 0;
    else
        return 1;
}

> > +        {
> > +            PERROR("MMIO: [%lx]<->[%lx] overlap DRM [%lx]<->[%lx]\n",
> Please use a more conventional representation here, e.g.
>
> "MMIO [%lx,%lx) overlaps RDM [%lx,%lx)\n"
Okay.

I tried to address your comments but I assume its correct to use 'errno' 
directly as I'm asking in another email:
@@ -54,6 +54,9 @@

 #define VGA_HOLE_SIZE (0x20)

+/* Record reserved device memory. */
+static struct xen_mem_reserved_device_memory *xmrdm = NULL;
+
 static int modules_init(struct xc_hvm_build_args *args,
                         uint64_t vend, struct elf_binary *elf,
                         uint64_t *mstart_out, uint64_t *mend_out)
@@ -239,6 +242,70 @@ static int check_mmio_hole(uint64_t start, uint64_t 
memsize,
         return 1;
 }

+/*
+ * Check whether there exists mmio overplap with the reserved device
+ * memory map
+ */
+static int check_rdm_overlap(xc_interface *xch, uint64_t mmio_start,
+                             uint64_t mmio_size)
+{
+    uint64_t rdm_start = 0, rdm_end = 0;
+    unsigned int i = 0;
+    int rc = 0;
+    /* 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 RDM mapping. */
+    if (!xmrdm) {
+        if ((xmrdm = malloc(nr_entries *
+                            sizeof(xen_mem_reserved_device_memory_t))) 
== NULL) {
+            PERROR("Could not allocate memory for map.");
+            return -1;
+        }
+
+        rc = xc_reserved_device_memory_map(xch, xmrdm, &nr_entries);
+        if (rc < 0 ) {
+            switch (errno) {
+            case ENOENT: /* RDM doesn't exist. */
+                rc = 0;
+                break;
+            case ENOBUFS: /* Need more space */
+                free(xmrdm);
+                /* Now we need more space to map all RDM mappings. */
+                if ((xmrdm = malloc(nr_entries *
+ 
sizeof(xen_mem_reserved_device_memory_t))) == NULL) {
+                    PERROR("Could not allocate memory for map.");
+                    return -1;
+                }
+                rc = xc_reserved_device_memory_map(xch, xmrdm, 
&nr_entries);
+                if (rc) {
+                    PERROR("Could not get RDM info on domain");
+                    free(xmrdm);
+                    return rc;
+                }
+                break;
+            default: /* Failed to get RDM */
+                PERROR("Could not get RDM info on domain");
+                return rc;
+            }
+        }
+    }
+
+    for (i = 0; i < rc; i++) {
+        rdm_start = xmrdm[i].start_pfn << XC_PAGE_SHIFT;
+        rdm_end = rdm_start + (xmrdm[i].nr_pages << XC_PAGE_SHIFT);
+        if (check_mmio_hole(rdm_start, (xmrdm[i].nr_pages << 
XC_PAGE_SHIFT),
+                            mmio_start, mmio_size) ) {
+            PERROR("MMIO: [%lx,%lx) overlap RDM [%lx,%lx)\n",
+                   mmio_start, (mmio_start + mmio_size), rdm_start, 
rdm_end);
+            free(xmrdm);
+            return -1;
+        }
+    }
+
+    return rc;
+}
+
 static int setup_guest(xc_interface *xch,
                        uint32_t dom, struct xc_hvm_build_args *args,
                        char *image, unsigned long image_size)

Thanks
Tiejun

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