[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jane Malalane <jane.malalane@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 18 Oct 2021 13:01:53 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=9QspbvaET/fKIId+6qOGtqo2qsD2x/bCrhnclhYEg84=; b=QiqX1q6GchtBqCyYAlCLq7C7pl8MfUxZdinBphIfmxf0E7ahz1eedL77MRhyHNpFS9mV/MD8bYacuX0Oa5dnrAn4+yZY8+Tksz8aborFM3mpQvrWm8OgYA70CIoNRFs03dJZ6tbVQ32VchVb/7PaGv154OGqIGpL5baSmPCCxP2dFBeNDBia/ZHsjc7T7qNYPoiqWfriugMom0Mk5B5EBgNUG0QW6HXQJyKy495uk91hNbDWtMKoQP52qTVsiUhPzujOxY7ebLjiU24Z75qx4Bf8/P4fzVzUilslR9T88LiKutGB3dL3T8dM6ZOiGNcHr1bG0jX0cIzMuN/qJLkHfQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lrrUCFZko0PGWC4GwYuPtPDK7Gi3dkCM1+m4KG1yKCMCDERvNKznt9MuacgBlq5tkgje0/iyf3pctSr/wf9CD6+tLBhKQEG2IJNiUBQOGxloE7sgdUWomuxtwVTlBnHlBXIfxWRQaBTQW2mvVpY6ybhyLbTAI/IDJJr1HDxDJpxd2xaRgus1bN5H8TOZK4rgJboswyRW/XQ7YnYFOJAVgfegniVR9NhQUt1Dy4oCUERWilfFU6fQpxOwaypUnWa3NWCom//qOdy31xoi2cSoO1df/nz89xKphrwXww5hg41wR6YuynVOi7/Jf8sKagXyDI74+iwwNUGOdpuBOd1xZQ==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 18 Oct 2021 11:02:05 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.