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

Jane Malalane posted 1 patch 2 years, 4 months ago
Failed in applying to current master (apply log)
tools/tests/resource/Makefile        |  2 +
tools/tests/resource/test-resource.c | 82 +++++++++++++++++++++++++++++++++---
2 files changed, 78 insertions(+), 6 deletions(-)
[PATCH for-4.16 v2] tests/resource: Extend to check that the grant frames are mapped correctly
Posted by Jane Malalane 2 years, 4 months ago
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>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

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)
 {
     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));
 
+    /* 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 +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);
+        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 +195,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 for-4.16 v2] tests/resource: Extend to check that the grant frames are mapped correctly
Posted by Roger Pau Monné 2 years, 4 months ago
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@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

LGTM:

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

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@xenproject.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> 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.

Re: [PATCH for-4.16 v2] tests/resource: Extend to check that the grant frames are mapped correctly
Posted by Andrew Cooper 2 years, 4 months ago
On 12/11/2021 15:16, Roger Pau Monné wrote:
> 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@citrix.com>
>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
>> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> LGTM:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> 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.

I'll fix on commit.  Thanks.

~Andrew