Re: [PATCH] tests/resource: Extend to check that the grant frames are mapped correctly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 12:08, Jane Malalane wrote:
> @@ -51,18 +55,52 @@ static void test_gnttab(uint32_t domid, unsigned int 
> nr_frames)
>      res = xenforeignmemory_map_resource(
>          fh, domid, XENMEM_resource_grant_table,
>          XENMEM_resource_grant_table_id_shared, 0, size >> XC_PAGE_SHIFT,
> -        &addr, PROT_READ | PROT_WRITE, 0);
> +        (void *)&gnttab, PROT_READ | PROT_WRITE, 0);

While in testing code I'm not as concerned about casts, I think it would
be better to cast to a visibly correct type, i.e. maintaining the levels
of indirection (void **). Alternatively you could (ab)use grants here,
allowing to drop the cast, by then assigning grants to gnttab.

>      /*
>       * Failure here with E2BIG indicates Xen is missing the bugfix to map
>       * resources larger than 32 frames.
>       */
>      if ( !res )
> -        return fail("    Fail: Map %d - %s\n", errno, strerror(errno));
> +        return fail("    Fail: Map grant table %d - %s\n", errno, 
> strerror(errno));
>  
> +    /* Put each gref at a unique offset in its frame. */
> +    for ( unsigned int i = 0; i < nr_frames; i++ )
> +    {
> +        unsigned int gref = i * (XC_PAGE_SIZE / sizeof(*gnttab)) + i;
> +
> +        refs[i] = gref;
> +        domids[i] = domid;
> +
> +        gnttab[gref].domid = 0;
> +        gnttab[gref].frame = gfn;
> +        gnttab[gref].flags = GTF_permit_access;
> +    }

To make obvious that you're done with gnttab[], perhaps better unmap it
here rather than at the bottom?

> +    /* Map grants. */
> +    grants = xengnttab_map_grant_refs(gh, nr_frames, domids, refs, PROT_READ 
> | PROT_WRITE);
> +
> +    /* Failure here indicates either that the frames were not mapped
> +     * in the correct order or xenforeignmemory_map_resource() didn't
> +     * give us the frames we asked for to begin with.
> +     */

Nit: Comment style.

> @@ -123,8 +162,25 @@ static void test_domain_configurations(void)
>  
>          printf("  Created d%u\n", domid);
>  
> -        test_gnttab(domid, t->create.max_grant_frames);
> +        rc = xc_domain_setmaxmem(xch, domid, -1);

That's an unbelievably large upper bound that you set. Since you
populate ...

> +        if ( rc )
> +        {
> +            fail("  Failed to set max memory for domain: %d - %s\n",
> +                 errno, strerror(errno));
> +            goto test_done;
> +        }
> +
> +        rc = xc_domain_populate_physmap_exact(xch, domid, ARRAY_SIZE(ram), 
> 0, 0, ram);

... only a single page, can't you get away with a much smaller value?
And without engaging truncation from uint64_t to unsigned int in
XEN_DOMCTL_max_mem handling in the hypervisor (which imo would better
yield an error)?

Jan