| Message ID | 20211018100848.10612-1-jane.malalane@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests/resource: Extend to check that the grant frames are mapped correctly | expand |
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 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: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
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: + */
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(-)