tools/tests/resource/Makefile | 2 + tools/tests/resource/test-resource.c | 81 +++++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 6 deletions(-)
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
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
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.
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
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
© 2016 - 2024 Red Hat, Inc.