From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v4 5/8] arm/vm_event: get/set registers Date: Tue, 31 May 2016 10:28:53 -0600 Message-ID: References: <1464561430-13465-1-git-send-email-tamas@tklengyel.com> <1464561430-13465-5-git-send-email-tamas@tklengyel.com> <574C453202000078000EFA44@prv-mh.provo.novell.com> <574D5DF002000078000EFDF0@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9053897859247762302==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b7mXI-0006qk-UG for xen-devel@lists.xenproject.org; Tue, 31 May 2016 16:28:57 +0000 Received: by mail-wm0-f68.google.com with SMTP id a136so34433414wme.0 for ; Tue, 31 May 2016 09:28:55 -0700 (PDT) Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com. [74.125.82.54]) by smtp.gmail.com with ESMTPSA id r129sm651268wmr.20.2016.05.31.09.28.53 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 31 May 2016 09:28:54 -0700 (PDT) Received: by mail-wm0-f54.google.com with SMTP id z87so115382858wmh.0 for ; Tue, 31 May 2016 09:28:53 -0700 (PDT) In-Reply-To: <574D5DF002000078000EFDF0@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: Xen-devel , Julien Grall , Stefano Stabellini , Razvan Cojocaru List-Id: xen-devel@lists.xenproject.org --===============9053897859247762302== Content-Type: multipart/alternative; boundary=001a1130cc6e6976f1053425de79 --001a1130cc6e6976f1053425de79 Content-Type: text/plain; charset=UTF-8 On May 31, 2016 01:48, "Jan Beulich" wrote: > > >>> On 30.05.16 at 21:47, wrote: > > On Mon, May 30, 2016 at 5:50 AM, Jan Beulich wrote: > >>>>> On 30.05.16 at 00:37, wrote: > >>> +struct vm_event_regs_arm32 { > >>> + uint32_t r0_usr; > >>> + uint32_t r1_usr; > >>> + uint32_t r2_usr; > >>> + uint32_t r3_usr; > >>> + uint32_t r4_usr; > >>> + uint32_t r5_usr; > >>> + uint32_t r6_usr; > >>> + uint32_t r7_usr; > >>> + uint32_t r8_usr; > >>> + uint32_t r9_usr; > >>> + uint32_t r10_usr; > >>> + uint32_t r12_usr; > >>> + uint32_t lr_usr; > >>> + uint32_t fp; > >>> + uint32_t pc; > >>> + uint32_t sp_usr; > >>> + uint32_t sp_svc; > >>> + uint32_t spsr_svc; > >>> +}; > >> > >> It would seem more natural for the "ordinary" registers to be > >> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc. > > > > Not sure I follow. > > For one it is quite natural for someone looking at a sequence of > register values to assume / expect them to be in natural order. > And then, having them in natural (numeric) order allows e.g. > extracting the register identifying bits from an instruction to use > them as an array index into (part of) this structure. > > (For some background: I've been bitten a number of times by > people sorting x86 registers alphabetically instead or naturally, > i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX). I see, however I believe that would be a very careless use of this struct from the user as the register sizes are not even necessarily match the architecture. For example we only define the 64bit x86 registers, so trying to access it as an array of 32bit registers wouldn't work at all. Plus we are not doing a full set of registers, and I rather not imply that every element in the "natural sequence" is present. It may be, it may be not. > > >>> +struct vm_event_regs_arm64 { > >>> + uint64_t x0; > >>> + uint64_t x1; > >>> + uint64_t x2; > >>> + uint64_t x3; > >>> + uint64_t x4; > >>> + uint64_t x5; > >>> + uint64_t x6; > >>> + uint64_t x7; > >>> + uint64_t x8; > >>> + uint64_t x9; > >>> + uint64_t x10; > >>> + uint64_t x16; > >>> + uint64_t lr; > >>> + uint64_t fp; > >>> + uint64_t pc; > >>> + uint64_t sp_el0; > >>> + uint64_t sp_el1; > >>> + uint32_t spsr_el1; > >>> + uint32_t _pad; > >>> +}; > >> > >> My ARM knowledge is certainly quite limited, but the incomplete set > >> of GPRs here is quite obvious: Is there a reason to not make all of > >> them available here? And if there is, could the criteria of which > >> registers got put here please be documented in a comment (so that > >> the next person noticing the incomplete set won't ask again)? > > > > There already is a comment in place that explains why we are not > > sending the full set of registers here. > > I've just gone through the entire patch again, and I didn't find any. > Are you perhaps referring to "Using custom vCPU structs (i.e. not > hvm_hw_cpu) for both x86 and ARM so as to not fill the vm_event > ring buffer too quickly"? If so, that's irrelevant here: It explains why > e.g. floating point registers don't get sent, but it doesn't explain in > any way why some GPRs are more important than others. > > >> I'm also puzzled by fp and lr - I'm not aware of registers of those > >> names (and gas also doesn't accept them afaict). > > > > Not sure why but Xen names x29 fp and x30 lr. See > > include/asm-arm/arm64/processor.h. > > See Julien's reply. > > Jan > --001a1130cc6e6976f1053425de79 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
> > On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> >>> +struct vm_event_regs_arm32 {
> >>> +=C2=A0 =C2=A0 uint32_t r0_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r1_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r2_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r3_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r4_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r5_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r6_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r7_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r8_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r9_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r10_usr;
> >>> +=C2=A0 =C2=A0 uint32_t r12_usr;
> >>> +=C2=A0 =C2=A0 uint32_t lr_usr;
> >>> +=C2=A0 =C2=A0 uint32_t fp;
> >>> +=C2=A0 =C2=A0 uint32_t pc;
> >>> +=C2=A0 =C2=A0 uint32_t sp_usr;
> >>> +=C2=A0 =C2=A0 uint32_t sp_svc;
> >>> +=C2=A0 =C2=A0 uint32_t spsr_svc;
> >>> +};
> >>
> >> It would seem more natural for the "ordinary" regis= ters to be
> >> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.=
> >
> > Not sure I follow.
>
> For one it is quite natural for someone looking at a sequence of
> register values to assume / expect them to be in natural order.
> And then, having them in natural (numeric) order allows e.g.
> extracting the register identifying bits from an instruction to use > them as an array index into (part of) this structure.
>
> (For some background: I've been bitten a number of times by
> people sorting x86 registers alphabetically instead or naturally,
> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).

I see, however I believe that would be a very careless use o= f this struct from the user as the register sizes are not even necessarily = match the architecture. For example we only define the 64bit x86 registers,= so trying to access it as an array of 32bit registers wouldn't work at= all. Plus we are not doing a full set of registers, and I rather not imply= that every element in the "natural sequence" is present. It may = be, it may be not.

>
> >>> +struct vm_event_regs_arm64 {
> >>> +=C2=A0 =C2=A0 uint64_t x0;
> >>> +=C2=A0 =C2=A0 uint64_t x1;
> >>> +=C2=A0 =C2=A0 uint64_t x2;
> >>> +=C2=A0 =C2=A0 uint64_t x3;
> >>> +=C2=A0 =C2=A0 uint64_t x4;
> >>> +=C2=A0 =C2=A0 uint64_t x5;
> >>> +=C2=A0 =C2=A0 uint64_t x6;
> >>> +=C2=A0 =C2=A0 uint64_t x7;
> >>> +=C2=A0 =C2=A0 uint64_t x8;
> >>> +=C2=A0 =C2=A0 uint64_t x9;
> >>> +=C2=A0 =C2=A0 uint64_t x10;
> >>> +=C2=A0 =C2=A0 uint64_t x16;
> >>> +=C2=A0 =C2=A0 uint64_t lr;
> >>> +=C2=A0 =C2=A0 uint64_t fp;
> >>> +=C2=A0 =C2=A0 uint64_t pc;
> >>> +=C2=A0 =C2=A0 uint64_t sp_el0;
> >>> +=C2=A0 =C2=A0 uint64_t sp_el1;
> >>> +=C2=A0 =C2=A0 uint32_t spsr_el1;
> >>> +=C2=A0 =C2=A0 uint32_t _pad;
> >>> +};
> >>
> >> My ARM knowledge is certainly quite limited, but the incomple= te set
> >> of GPRs here is quite obvious: Is there a reason to not make = all of
> >> them available here? And if there is, could the criteria of w= hich
> >> registers got put here please be documented in a comment (so = that
> >> the next person noticing the incomplete set won't ask aga= in)?
> >
> > There already is a comment in place that explains why we are not<= br> > > sending the full set of registers here.
>
> I've just gone through the entire patch again, and I didn't fi= nd any.
> Are you perhaps referring to "Using custom vCPU structs (i.e. not=
> hvm_hw_cpu) for both x86 and ARM so as to not fill the vm_event
> ring buffer too quickly"? If so, that's irrelevant here: It e= xplains why
> e.g. floating point registers don't get sent, but it doesn't e= xplain in
> any way why some GPRs are more important than others.
>
> >> I'm also puzzled by fp and lr - I'm not aware of regi= sters of those
> >> names (and gas also doesn't accept them afaict).
> >
> > Not sure why but Xen names x29 fp and x30 lr. See
> > include/asm-arm/arm64/processor.h.
>
> See Julien's reply.
>
> Jan
>

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