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 <jbeulich@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

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 <jbeulich@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

On 25.06.2024 12:43, Andrew Cooper wrote:
> 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 <jbeulich@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.

As said elsewhere, it's there just-in-case (and now consistent with
sibling gnttab-ops). We can certainly evaluate deleting all of those
just-in-case constructs. But we want to retain consistency.

> So - my v1 was correct, and your and Stefano's feedback on v1 was incorrect.

I'm sorry, but maybe more a misunderstanding. I notice I had "now" in my
reply to you when referring to my reply to Stefano, when I think I really
meant "not". And he never actually replied, afaics.

Jan

On Tue, Jun 25, 2024 at 09:30:07AM +0200, 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 <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.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.

If we have to backport the fix anyway, we might as well consider
taking it now.

Thanks, Roger.

On 25.06.2024 11:24, Roger Pau Monné wrote:
> On Tue, Jun 25, 2024 at 09:30:07AM +0200, 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 <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> 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.
> 
> If we have to backport the fix anyway, we might as well consider
> taking it now.

While I put a Fixes: tag there, this is the kind of change where I don't
think it needs backporting. The ABI of released versions is supposed to
be yet less in flux than the "stable" ABI in general.

Jan

On Tue, 2024-06-25 at 09:30 +0200, 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 <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko <Oleksii.kurochko@gmail.com>

~ Oleksii

> ---
> 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
>  

On 25.06.2024 09:30, 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 <jbeulich@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.

Actually it's get-version too, yet then for both only half of what's needed
that's missing (and luckily only the just-in-case part, not the actual layout
check). In any event I've queued a patch for 4.20.

Jan