Re: [PATCH] tests/resource: Verify that Xen can deal with invalid resource type

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 17.02.25 23:09, Andrew Cooper wrote:

Hello.

> On 17/02/2025 8:48 pm, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > The sign of the presence of a corresponding bugfix is an error
> > returned on querying the size of an unknown for Xen resource type.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> > Refer https://patchew.org/Xen/20250217102741.4122367-1-olekstysh@xxxxxxxxx/
> > Current patch should go in with Xen fix from the link above.
> >
> > 1. w/ fix in Xen:
> >
> > + ./tests/resource/test-resource
> > XENMEM_acquire_resource tests
> > Test x86 PV
> >    Created d1
> >    Test grant table
> > (XEN) [    8.702293] common/grant_table.c:1909:d0v1 Expanding d1 grant table 
> > from 1 to 32 frames
> > (XEN) [    8.704499] common/grant_table.c:1909:d0v1 Expanding d1 grant table 
> > from 32 to 40 frames
> > Test x86 PVH
> >    Created d2
> >    Test grant table
> > (XEN) [    8.717210] common/grant_table.c:1909:d0v1 Expanding d2 grant table 
> > from 1 to 32 frames
> > (XEN) [    8.719477] common/grant_table.c:1909:d0v1 Expanding d2 grant table 
> > from 32 to 40 frames
> >   [ ok ]
> >   [ ok ]
> > Welcome to Alpine Linux 3.18
> > Kernel 6.1.19 on an x86_64 (/dev/hvc0)
> >
> > 2. w/o fix in Xen:
> >
> > + ./tests/resource/test-resource
> > XENMEM_acquire_resource tests
> > Test x86 PV(XEN) [    9.839691] common/grant_table.c:1909:d0v0 Expanding d0 
> > grant table from 1 to 2 frames
> >    Created d1
> >    Test grant table
> > (XEN) [    9.846621] common/grant_table.c:1909:d0v1 Expanding d1 grant table 
> > from 1 to 32 frames
> > (XEN) [    9.848796] common/grant_table.c:1909:d0v1 Expanding d1 grant table 
> > from 32 to 40 frames
> >      Fail: Expected error on an invalid resource type, got success
> > Test x86 PVH
> >    Created d2
> >    Test grant table
> > (XEN) [    9.865235] common/grant_table.c:1909:d0v1 Expanding d2 grant table 
> > from 1 to 32 frames
> > (XEN) [    9.867403] common/grant_table.c:1909:d0v1 Expanding d2 grant table 
> > from 32 to 40 frames
> >      Fail: Expected error on an invalid resource type, got success
> >   *   Execution of "/etc/local.d/xen.start" failed.
> >   [ !! ]
> >   [ !! ]
> > Welcome to Alpine Linux 3.18
> > Kernel 6.1.19 on an x86_64 (/dev/hvc0)
> > ---
> > ---
> >   tools/tests/resource/test-resource.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/tools/tests/resource/test-resource.c 
> > b/tools/tests/resource/test-resource.c
> > index 1b10be16a6..3f5479cf78 100644
> > --- a/tools/tests/resource/test-resource.c
> > +++ b/tools/tests/resource/test-resource.c
> > @@ -123,6 +123,17 @@ static void test_gnttab(uint32_t domid, unsigned int 
> > nr_frames,
> >           fail("    Fail: Managed to map gnttab v2 status frames in v1 mode\n");
> >           xenforeignmemory_unmap_resource(fh, res);
> >       }
> > +
> > +    /*
> > +     * While at it, verify that an attempt to query the size of an unknown
> > +     * for Xen resource type fails.
> "If this check starts failing, you've find the right place to test your
> addition to the Acquire Resource infrastructure."
>
> Or something suitable.  There needs to be a hint of why 3 was picked
> here, and part of this sentence will show up in the context of the patch
> bumping 3 to 4, which also helps remind reviewers to ask if a change
> isn't somewhere in the submitted series.
>
> Anyway, LGTM now.
Thanks

> Personally, I'd merge this into the bugfix patch.  This improved test
> wants backporting along with the fix, and the end result is still only 3
> hunks.
>
> Then, I'd suggest posting the combined result for-4.20 (cc Oleksii).
> It's not a major bug, but it's also a very simple and low risk fix too.
>
> ~Andrew
>
> > +     */
> > +    rc = xenforeignmemory_resource_size(
> > +        fh, domid, 3, 0, &size);
> P.S.  Now it's simple, fold this back into 1 line.  It causes an extra
> line of the comment to be visible in context.
I agree with the comments.