On 25/06/2024 8:30 am, Jan Beulich wrote:
> The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar
> constructs, should have caught my attention. Turns out it was needed for
> the build to succeed merely because the corresponding #ifndef had a
> typo. That typo in turn broke compat mode guests, by having query-size
> requests of theirs wire into the domain_crash() at the bottom of the
> switch().
>
> Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size
> check")
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> ---
> Looks like set-version is similarly missing in the set of structures
> checked, but I'm pretty sure that we will now want to defer taking care
> of that until after 4.20 was branched.
>
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -33,7 +33,6 @@ CHECK_gnttab_unmap_and_replace;
> #define xen_gnttab_query_size gnttab_query_size
> CHECK_gnttab_query_size;
> #undef xen_gnttab_query_size
> -DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_compat_t);
>
> DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t);
> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_compat_t);
> @@ -111,7 +110,7 @@ int compat_grant_table_op(
> CASE(copy);
> #endif
>
> -#ifndef CHECK_query_size
> +#ifndef CHECK_gnttab_query_size
> CASE(query_size);
> #endif
>
/sigh - I almost rejected your and Stefano's feedback on v1 on the basis
that it didn't compile, but then I adjusted it to look like the
surrounding logic. Much fool me.
But, this change *cannot* be correct. The result is:
$ git grep -C3 CHECK_gnttab_query_size
compat/grant_table.c-31-#undef xen_gnttab_unmap_and_replace
compat/grant_table.c-32-
compat/grant_table.c-33-#define xen_gnttab_query_size gnttab_query_size
compat/grant_table.c:34:CHECK_gnttab_query_size;
compat/grant_table.c-35-#undef xen_gnttab_query_size
compat/grant_table.c-36-
compat/grant_table.c-37-DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t);
--
compat/grant_table.c-110- CASE(copy);
compat/grant_table.c-111-#endif
compat/grant_table.c-112-
compat/grant_table.c:113:#ifndef CHECK_gnttab_query_size
compat/grant_table.c-114- CASE(query_size);
compat/grant_table.c-115-#endif
compat/grant_table.c-116-
and the second is dead code because CHECK_gnttab_query_size is defined.
It shouldn't be there.
So - my v1 was correct, and your and Stefano's feedback on v1 was incorrect.
The problem is, of course, that absolutely none of this is written down,
and none of the logic one needs to read to figure out it is even checked
into the tree. It's entirely automatic and not even legible in its
intermediate form.
We are going to start with writing down a very simple set of
instructions for how the compat infrastructure works and how it should
be used. If it's too complicated I will delete bits until it becomes
manageable, because this is completely insane.
~Andrew
