xen: Fix warning by adding KERNBASE to vm_paddr_t before casting

[dim]

Summary

Clang 13 produces the following warning for hammer_time_xen():

sys/x86/xen/pv.c:183:19: error: the pointer incremented by -2147483648 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements) [-Werror,-Warray-bounds]
                    (vm_paddr_t)start_info->modlist_paddr + KERNBASE;
                                ^                           ~~~~~~~~
sys/xen/interface/arch-x86/hvm/start_info.h:131:5: note: array 'modlist_paddr' declared here
    uint64_t modlist_paddr;     /* Physical address of an array of           */
    ^

This is because the expression first casts start_info->modlist_paddr
to struct hvm_modlist_entry * (via vmpaddr_t), and *then* adds
KERNBASE, which is then interpreted as KERNBASE * sizeof(struct
hvm_modlist_entry).

Instead, parenthesize the addition to get the intended result, and cast
it to struct hvm_modlist_entry * afterwards.

[royger]

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

[dim]

In D31711#715616, @royger wrote:

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

Hm, won't that give problems on i386? I'll see what a i386 build does.

[royger]

In D31711#715619, @dim wrote:

In D31711#715616, @royger wrote:

I think you can also remove the cast to vm_paddr_t while there, the field is declared as uint64_t.

Hm, won't that give problems on i386? I'll see what a i386 build does.

That code is only built for amd64, I don't have plans to use it in i386.

[royger]

On second thought it might have been easier to just cast to void * and forget about the parentheses.

[ehem_freebsd_m5p.com]

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

[royger]

In D31711#716440, @ehem_freebsd_m5p.com wrote:

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

It's a GNU C extension which I thought was fine to be used, as we already use other GNU C extensions I think.

[dim]

In D31711#716449, @royger wrote:

In D31711#716440, @ehem_freebsd_m5p.com wrote:

No, then you would be doing arithmetic on a void pointer, which isn't officially legal (size of void being undefined). Better to do the math on integers and cast to the correct type.

It's a GNU C extension which I thought was fine to be used, as we already use other GNU C extensions I think.

In practice it should be fine as gcc compatible compilers just assume char-sized arithmetic on void pointers, but you'll definitely get warnings about it, which you then must suppress. :)

So it's probably better safe than sorry, either cast to a byte pointer type like char* or uint8_t*, then do the arithmetic, or go via intptr_t's.