From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v5 11/28] xsplice: Implement support for applying/reverting/replacing patches. Date: Mon, 11 Apr 2016 19:29:47 -0400 Message-ID: References: <1458849640-22588-1-git-send-email-konrad.wilk@oracle.com> <1458849640-22588-12-git-send-email-konrad.wilk@oracle.com> <56FE938002000078000E2104@prv-mh.provo.novell.com> <20160407030927.GC24604@char.us.oracle.com> <57068E3A02000078000E5FB4@prv-mh.provo.novell.com> <20160410023600.GA8445@localhost.localdomain> <20160410024511.GB8445@localhost.localdomain> <570BD3C702000078000E6399@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4173903946303192703==" Return-path: Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aplHC-0000cP-Ls for xen-devel@lists.xenproject.org; Mon, 11 Apr 2016 23:29:50 +0000 Received: by mail-io0-f195.google.com with SMTP id z133so575622iod.1 for ; Mon, 11 Apr 2016 16:29:48 -0700 (PDT) In-Reply-To: <570BD3C702000078000E6399@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Kevin Tian , Keir Fraser , Jun Nakajima , andrew.cooper3@citrix.com, mpohlack@amazon.de, ross.lagerwall@citrix.com, Julien Grall , Stefano Stabellini , Suravee Suthikulpanit , xen-devel@lists.xenproject.org, sasha.levin@oracle.com, Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org --===============4173903946303192703== Content-Type: multipart/alternative; boundary=001a113fd60899a5be05303deb59 --001a113fd60899a5be05303deb59 Content-Type: text/plain; charset=UTF-8 On Apr 11, 2016 11:41 AM, "Jan Beulich" wrote: > > >>> On 10.04.16 at 04:45, 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, 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 > --001a113fd60899a5be05303deb59 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


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 w= rote:
> >> On Thu, Apr 07, 2016 at 09:43:38AM -0600, Jan Beulich wrote:<= br> > >> > >>> On 07.04.16 at 05:09, <konrad.wilk@oracle.com> wrote:
> >> > >> > +=C2=A0 =C2=A0 uint8_t *old_ptr;
> >> > >> > +
> >> > >> > +=C2=A0 =C2=A0 BUILD_BUG_ON(PATCH_INSN_SIZ= E > sizeof(func->undo));
> >> > >> > +=C2=A0 =C2=A0 BUILD_BUG_ON(PATCH_INSN_SIZ= E !=3D (1 + sizeof val));
> >> > >> > +
> >> > >> > +=C2=A0 =C2=A0 old_ptr =3D (uint8_t *)func= ->old_addr;
> >> > >>
> >> > >> (Considering this cast, the "old_addr"= ; member should be
> >> > >> unsigned long (or void *), not uint64_t: The la= test on ARM32
> >> > >> such would otherwise cause problems.)
> >> > >
> >> > > I has to be uint8_t to make the single byte modific= ations. 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 x8= 6 code, but
> >> > my reference to ARM32 was only mean to say that there yo= u'll
> >> > need to do something similar, and casting uint64_t to wh= atever
> >> > kind of pointer type is not going to work without compil= er warning.)
> >>
> >> Way back .. when we spoke about the .xsplice.funcs structure<= br> > >> 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' structur= e
> > 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
>

--001a113fd60899a5be05303deb59-- --===============4173903946303192703== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============4173903946303192703==--