Re: [Xen-devel] Is: ARM maintainers advice ..Was:Re: [PATCH v5 11/28] xsplice: Implement support for applying/reverting/replacing patches.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sun, 10 Apr 2016, Konrad Rzeszutek Wilk wrote:
> On Sat, Apr 09, 2016 at 10:36:02PM -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@xxxxxxxxxx> 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. 
>
> CC-ing Stefano and Julien here to advise. I ended up 
> exposing the ABI part in sysctl.h (and design document as):
> 
> 
> #define XSPLICE_PAYLOAD_VERSION 1
> /*
>  * .xsplice.funcs structure layout defined in the `Payload format`
>  * section in the xSplice design document.
>  *
>  * The size should be exactly 64 bytes.
>  */
> struct xsplice_patch_func {
>     const char *name;       /* Name of function to be patched. */
>     uint64_t new_addr;
>     uint64_t old_addr;      /* Can be zero and name will be looked up. */
>     uint32_t new_size;
>     uint32_t old_size;
>     uint8_t version;        /* MUST be XSPLICE_PAYLOAD_VERSION. */
>     uint8_t pad[31];        /* MUST be zero filled. */
> };
> typedef struct xsplice_patch_func xsplice_patch_func_t;
> 
> Which looks nice.
> 
> When the ELF file is loaded we load it as this structure:
> 
> [x86]
> #ifndef CONFIG_ARM
> struct xsplice_patch_func_internal {
>     const char *name;
>     void *new_addr;
>     void *old_addr;
>     uint32_t new_size;
>     uint32_t old_size;
>     uint8_t version;
>     union {
>         uint8_t undo[8];
>         uint8_t pad[31];
>     } u;
> };
> #else
> [ARM]
> struct xsplice_patch_func_internal {
>     const char *name;
>     uint32_t _pad0;
>     void *new_addr;
>     uint32_t _pad1;
>     void *old_addr;
>     uint32_t _pad2;
>     uint32_t new_size;
>     uint32_t old_size;
>     uint8_t version;
>     union {
>         uint8_t pad[31];
>     } u;
> };
> #endif
> 
> That allows the size and offsets to be the same. I checked under ARM32
> builds:
> 
> 
> struct xsplice_patch_func_internal {
>     const char  *              name;                 /*     0     4 */
>     uint32_t                   _pad0;                /*     4     4 */
>     void *                     new_addr;             /*     8     4 */
>     uint32_t                   _pad1;                /*    12     4 */
>     void *                     old_addr;             /*    16     4 */
>     uint32_t                   _pad2;                /*    20     4 */
>     uint32_t                   new_size;             /*    24     4 */
>     uint32_t                   old_size;             /*    28     4 */
>     uint8_t                    version;              /*    32     1 */
>     union {
>         uint8_t            pad[31];              /*          31 */
>     } u;                                             /*    33    31 */
>     /* --- cacheline 1 boundary (64 bytes) --- */
> 
>     /* size: 64, cachelines: 1, members: 10 */
> };
> 
> So it all looks correct. (and I can cast the old_addr to uint8_t).
> Naturally we can expand the pad[] to hold whatever is needed
> when implementing xSplice under ARM
> 
> However I would appreciate advise from ARM folks if I made some
> wrong assumptions..

It looks good. I take that ARM64 will be like x86. In that case, instead
of #ifdef'ing x86 or ARM, I would #ifdef BITS_PER_LONG or POINTER_ALIGN.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel