Re: [PATCH for-4.19] gnttab: fix compat query-size handling

[Jan Beulich <jbeulich@xxxxxxxx>]

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