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