Re: [PATCH] xen/guest_access: Fix accessors for 32bit builds of Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2024 18:31, Andrew Cooper wrote:
> Gitlab CI reports an ARM32 randconfig failure as follows:
> 
>   In file included from common/livepatch.c:9:
>   common/livepatch.c: In function ‘livepatch_list’:
>   ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of 
> different size [-Werror=int-to-pointer-cast]
>     130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
>         |                         ^
>   common/livepatch.c:1283:18: note: in expansion of macro 
> ‘__copy_to_guest_offset’
>    1283 |             if ( __copy_to_guest_offset(list->name, name_offset,
>         |                  ^~~~~~~~~~~~~~~~~~~~~~
>   ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of 
> different size [-Werror=int-to-pointer-cast]
>     130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
>         |                         ^
>   common/livepatch.c:1287:17: note: in expansion of macro 
> ‘__copy_to_guest_offset’
>    1287 |                 __copy_to_guest_offset(list->metadata, 
> metadata_offset,
>         |                 ^~~~~~~~~~~~~~~~~~~~~~
> 
> This isn't specific to ARM32; it's LIVEPATCH on any 32bit build of Xen.
> 
> Both name_offset and metadata_offset are uint64_t, meaning that the
> expression:
> 
>   (d_ + (off) * sizeof(*(hnd).p)
> 
> gets promoted to uint64_t, and is too wide to cast back to a pointer in 32bit
> builds.  The expression needs casting through (unsigned long) before it can be
> cast to (void *).

I disagree. Instead I'd like to raise the question why these two local variables
are uint64_t in the first place. They accumulate buffer size, and hence ought to
have been size_t from the beginning. I'll make an alternative patch (first 
making
sure I test livepatch building not only for x86 and arm64).

> @@ -65,7 +65,7 @@
>      /* Check that the handle is not for a const type */ \
>      void *__maybe_unused _t = (hnd).p;                  \
>      (void)((hnd).p == _s);                              \
> -    raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
> +    raw_copy_to_guest(_p(d_ + (off) * sizeof(*_s)),     \

It's also from an abstract perspective that I disagree with using _p() like 
this.
We'd rather keep this as straightforward as possible, to keep down the risk of
hiding bugs by excess casting.

Jan