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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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().

> +        {
> +            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.

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

> +        {
> +            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"

Jan


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