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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 12, 2021 at 02:48:21PM +0000, Jane Malalane wrote:
> Previously, we checked that we could map 40 pages with nothing
> complaining. Now we're adding extra logic to check that those 40
> frames are "correct".
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

There are a couple of lines that exceed the 80 column limit that we
have for other pieces of code, not sure if there's some kind of
exception for tests. If so it might be easy to either resend or fix at
commit.

> ---
> CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> v2:
>  * Cast the gnttab pointer to the correct type (void **)
>  * Fixed comment style
> ---
>  tools/tests/resource/Makefile        |  2 +
>  tools/tests/resource/test-resource.c | 82 
> +++++++++++++++++++++++++++++++++---
>  2 files changed, 78 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/tests/resource/Makefile b/tools/tests/resource/Makefile
> index 1c3aee4ff7..b3cd70c06d 100644
> --- a/tools/tests/resource/Makefile
> +++ b/tools/tests/resource/Makefile
> @@ -31,10 +31,12 @@ CFLAGS += -Werror
>  CFLAGS += $(CFLAGS_xeninclude)
>  CFLAGS += $(CFLAGS_libxenctrl)
>  CFLAGS += $(CFLAGS_libxenforeginmemory)
> +CFLAGS += $(CFLAGS_libxengnttab)
>  CFLAGS += $(APPEND_CFLAGS)
>  
>  LDFLAGS += $(LDLIBS_libxenctrl)
>  LDFLAGS += $(LDLIBS_libxenforeignmemory)
> +LDFLAGS += $(LDLIBS_libxengnttab)
>  LDFLAGS += $(APPEND_LDFLAGS)
>  
>  %.o: Makefile
> diff --git a/tools/tests/resource/test-resource.c 
> b/tools/tests/resource/test-resource.c
> index 1caaa60e62..286d338c1f 100644
> --- a/tools/tests/resource/test-resource.c
> +++ b/tools/tests/resource/test-resource.c
> @@ -6,6 +6,7 @@
>  
>  #include <xenctrl.h>
>  #include <xenforeignmemory.h>
> +#include <xengnttab.h>
>  #include <xen-tools/libs.h>
>  
>  static unsigned int nr_failures;
> @@ -17,13 +18,16 @@ static unsigned int nr_failures;
>  
>  static xc_interface *xch;
>  static xenforeignmemory_handle *fh;
> +static xengnttab_handle *gh;
>  
> -static void test_gnttab(uint32_t domid, unsigned int nr_frames)
> +static void test_gnttab(uint32_t domid, unsigned int nr_frames, unsigned 
> long gfn)

Nit: i think this line exceeds the 80 column limit.

>  {
>      xenforeignmemory_resource_handle *res;
> -    void *addr = NULL;
> +    grant_entry_v1_t *gnttab;
>      size_t size;
>      int rc;
> +    uint32_t refs[nr_frames], domids[nr_frames];
> +    void *grants;
>  
>      printf("  Test grant table\n");
>  
> @@ -51,18 +55,53 @@ 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);
>  
>      /*
>       * 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));

Likewise.

>  
> +    /* 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;
> +    }
> +
> +    /* Map grants. */
> +    grants = xengnttab_map_grant_refs(gh, nr_frames, domids, refs, PROT_READ 
> | PROT_WRITE);

Here.

> +
> +    /*
> +     * 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.
> +     */
> +    if ( grants == NULL )
> +    {
> +        fail("    Fail: Map grants %d - %s\n", errno, strerror(errno));
> +        goto out;
> +    }
> +
> +    /* Unmap grants. */
> +    rc = xengnttab_unmap(gh, grants, nr_frames);
> +
> +    if ( rc )
> +        fail("    Fail: Unmap grants %d - %s\n", errno, strerror(errno));
> +
> +    /* Unmap grant table. */
> + out:
>      rc = xenforeignmemory_unmap_resource(fh, res);
>      if ( rc )
> -        return fail("    Fail: Unmap %d - %s\n", errno, strerror(errno));
> +        return fail("    Fail: Unmap grant table %d - %s\n", errno, 
> strerror(errno));

Here.

>  }
>  
>  static void test_domain_configurations(void)
> @@ -107,6 +146,7 @@ static void test_domain_configurations(void)
>          struct test *t = &tests[i];
>          uint32_t domid = 0;
>          int rc;
> +        xen_pfn_t ram[1] = { 0 };
>  
>          printf("Test %s\n", t->name);
>  
> @@ -123,8 +163,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);
> +        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);

And here also.

Thanks, Roger.