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

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

[Jane Malalane]

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@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
 tools/tests/resource/Makefile        |  2 +
 tools/tests/resource/test-resource.c | 81 +++++++++++++++++++++++++++++++++---
 2 files changed, 77 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..fa4ca6217f 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)
 {
     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,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);
 
     /*
      * 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;
+    }
+
+    /* 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.
+     */
+    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));
 }
 
 static void test_domain_configurations(void)
@@ -107,6 +145,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 +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);
+        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);
+        if ( rc )
+        {
+            fail("  Failed to populate physmap domain: %d - %s\n",
+                 errno, strerror(errno));
+            goto test_done;
+        }
+
+        test_gnttab(domid, t->create.max_grant_frames, ram[0]);
 
+    test_done:
         rc = xc_domain_destroy(xch, domid);
         if ( rc )
             fail("  Failed to destroy domain: %d - %s\n",
@@ -138,13 +194,26 @@ int main(int argc, char **argv)
 
     xch = xc_interface_open(NULL, NULL, 0);
     fh = xenforeignmemory_open(NULL, 0);
+    gh = xengnttab_open(NULL, 0);
 
     if ( !xch )
         err(1, "xc_interface_open");
     if ( !fh )
         err(1, "xenforeignmemory_open");
+    if ( !gh )
+        err(1, "xengnttab_open");
 
     test_domain_configurations();
 
     return !!nr_failures;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.11.0

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

[Jan Beulich]

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

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

[Andrew Cooper]

On 18/10/2021 11:08, 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@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

Ian:  I'd like this to be considered for 4.16.  It is extending an
existing test case with better error detection.

It was a task I didn't get around to at the time, because of the 4.15
release freeze...  How time flies.

Anyway, it is very low risk to the release, and 0 risk for anyone who
doesn't run the tests...

~Andrew

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

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH for-4.16] tests/resource: Extend to check that the grant frames are mapped correctly"):
> On 18/10/2021 11:08, 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@citrix.com>
> > Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> 
> Ian:  I'd like this to be considered for 4.16.  It is extending an
> existing test case with better error detection.
> 
> It was a task I didn't get around to at the time, because of the 4.15
> release freeze...  How time flies.
> 
> Anyway, it is very low risk to the release, and 0 risk for anyone who
> doesn't run the tests...

Of course.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Ian.

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

[Andrew Cooper]

On 18/10/2021 12:01, Jan Beulich wrote:
> On 18.10.2021 12:08, Jane Malalane wrote:
>
>>      /*
>>       * 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?

This is just test code.  We could unmap it earlier, but that makes it
irritating if you ever need to insert printk()'s.

>> @@ -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?

Yes, but again, this is test code.

Furthermore, there are other plans for further testing which would mean
1 wouldn't be appropriate here.  All we want is "don't choke on limits
while we're performing testing".

~Andrew