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:20:56 -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> <574D5F6402000078000EFDFE@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2187931981254122494==" 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 1b7mPc-000691-K2 for xen-devel@lists.xenproject.org; Tue, 31 May 2016 16:21:00 +0000 Received: by mail-wm0-f66.google.com with SMTP id e3so34176955wme.2 for ; Tue, 31 May 2016 09:20:58 -0700 (PDT) Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com. [74.125.82.41]) by smtp.gmail.com with ESMTPSA id 75sm18080096wml.15.2016.05.31.09.20.56 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 31 May 2016 09:20:57 -0700 (PDT) Received: by mail-wm0-f41.google.com with SMTP id z87so115089916wmh.0 for ; Tue, 31 May 2016 09:20:56 -0700 (PDT) In-Reply-To: <574D5F6402000078000EFDFE@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 --===============2187931981254122494== Content-Type: multipart/alternative; boundary=001a1148ea96fd5bc3053425c1fe --001a1148ea96fd5bc3053425c1fe Content-Type: text/plain; charset=UTF-8 On May 31, 2016 01:54, "Jan Beulich" wrote: > > >>> On 30.05.16 at 22:37, wrote: > > On Mon, May 30, 2016 at 2:20 PM, Julien Grall wrote: > >> On 30/05/2016 20:47, Tamas K Lengyel wrote: > >>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich wrote: > >>>>> +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. > >> > >> > >> Your comment only says: > >> "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." it does not explain > >> the criteria of which registers got put here. > > > > Well, as we discussed it in the previous revision, there is no > > hard-set rule of what can and cannot be transmitted here. The only > > thing to keep in mind is to not grow this struct to be too large. The > > registers sent right now represent a "best guess" of what may be > > useful for performance-sensitive vm_event applications on ARM. It can > > be adjusted in the future if applications require other registers. > > Right now there are no applications at all in this space so we don't > > have any specifications to rely on for making this selection. I'm > > happy to make adjustments to the selection if others have a better > > idea what to send, the only registers I certainly find very useful are > > TTBR0/1, cpsr and pc at this time. > > Not being an ARM maintainer I'd say "Then include just those and no > (other) GPRs at all, or include all GPRs." Such a "best guess" approach > can really only end up being wrong from some future consumer. And > in that consideration, please also include the aspects that lead to all > x86 GPRs to get included here (not to speak of even various MSR > values). IOW the same criteria should be applied to all architectures. > I don't think there is such a thing as being wrong here. The user always has access to the full set of registers plus there is ample room here to add other registers in the future while we are smaller then the x86 struct. For an initial set I think it's perfectly fine to do a subset and add more as we go forward and learn about the usecases. So since there is no technical reason that doing a subset would be incorrect I don't see an issue here. As I said, I'm happy to explain it in the commit message and a comment in the code that the register set is expandable and adjustable. Tamas --001a1148ea96fd5bc3053425c1fe Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On May 31, 2016 01:54, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 30.05.16 at 22:37, <tamas@tklengyel.com> wrote:
> > On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
> >> On 30/05/2016 20:47, Tamas K Lengyel wrote:
> >>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> +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 = incomplete set
> >>>> of GPRs here is quite obvious: Is there a reason to n= ot make all of
> >>>> them available here? And if there is, could the crite= ria of which
> >>>> registers got put here please be documented in a comm= ent (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.
> >>
> >>
> >> Your comment only says:
> >> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for bot= h x86 and ARM
> >> so as to not fill the vm_event ring buffer too quickly."= it does not explain
> >> the criteria of which registers got put here.
> >
> > Well, as we discussed it in the previous revision, there is no > > hard-set rule of what can and cannot be transmitted here. The onl= y
> > thing to keep in mind is to not grow this struct to be too large.= The
> > registers sent right now represent a "best guess" of wh= at may be
> > useful for performance-sensitive vm_event applications on ARM. It= can
> > be adjusted in the future if applications require other registers= .
> > Right now there are no applications at all in this space so we do= n't
> > have any specifications to rely on for making this selection. I&#= 39;m
> > happy to make adjustments to the selection if others have a bette= r
> > idea what to send, the only registers I certainly find very usefu= l are
> > TTBR0/1, cpsr and pc at this time.
>
> Not being an ARM maintainer I'd say "Then include just those = and no
> (other) GPRs at all, or include all GPRs." Such a "best gues= s" approach
> can really only end up being wrong from some future consumer. And
> in that consideration, please also include the aspects that lead to al= l
> x86 GPRs to get included here (not to speak of even various MSR
> values). IOW the same criteria should be applied to all architectures.=
>

I don't think there is such a thing as being wrong here.= The user always has access to the full set of registers plus there is ampl= e room here to add other registers in the future while we are smaller then = the x86 struct. For an initial set I think it's perfectly fine to do a = subset and add more as we go forward and learn about the usecases. So since= there is no technical reason that doing a subset would be incorrect I don&= #39;t see an issue here. As I said, I'm happy to explain it in the comm= it message and a comment in the code that the register set is expandable an= d adjustable.

Tamas

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