On Apr 11, 2016 11:41 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 10.04.16 at 04:45, <konrad@kernel.org> wrote:
> > On Sat, Apr 09, 2016 at 10:36:00PM -0400, Konrad Rzeszutek Wilk wrote:
> >> On Thu, Apr 07, 2016 at 09:43:38AM -0600, Jan Beulich wrote:
> >> > >>> On 07.04.16 at 05:09, <konrad.wilk@oracle.com> wrote:
> >> > >> > +    uint8_t *old_ptr;
> >> > >> > +
> >> > >> > +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->undo));
> >> > >> > +    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof val));
> >> > >> > +
> >> > >> > +    old_ptr = (uint8_t *)func->old_addr;
> >> > >>
> >> > >> (Considering this cast, the "old_addr" member should be
> >> > >> unsigned long (or void *), not uint64_t: The latest on ARM32
> >> > >> such would otherwise cause problems.)
> >> > >
> >> > > I has to be uint8_t to make the single byte modifications. Keep
> >> > > also in mind that this file is only for x86.
> >> >
> >> > old_addr can't reasonably be uint8_t, so I can only assume you're
> >> > mixing up things here. (And yes, I do realize this is x86 code, but
> >> > my reference to ARM32 was only mean to say that there you'll
> >> > need to do something similar, and casting uint64_t to whatever
> >> > kind of pointer type is not going to work without compiler warning.)
> >>
> >> Way back .. when we spoke about the .xsplice.funcs structure
> >> you recommended to make the types be either uintXX specific
> >> or Elf types. I choose Elf types but then we realized that
> >> ARM32 hypervisor would be involved which of course would have
> >> a different size of the structure. So I went with uintXXX
> >> to save a bit of headache (specifically those BUILD_BUG_ON
> >> checks).
> >>
> >> I can't see making the old_addr be unsigned long or void *,
> >> which means going back to Elf types. And for ARM32 I will
> >> have to deal with a different structure size.
> >
> > Oh gosh, that is going to be problem with our headers as
> > I would be now exposing the 'xsplice_patch_func' structure
> > in a public header which would depend on Elf_X types.
>
> So how about uintptr_t? Not exactly the thing we do in public
> headers already, but at least a possibility. Or else maybe
> xen_ulong_t, albeit on ARM32 that again won't cast well to
> pointer types.

I ended up (see v7 patches) making it uint64_t in public and in the private
void*.

>
> Jan
>