From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v8 06/19] xen: Relocate mem_event_op domctl and access_op memop into common. Date: Tue, 23 Sep 2014 16:28:52 +0200 Message-ID: References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-7-git-send-email-tklengyel@sec.in.tum.de> <542192860200007800037BB5@mail.emea.novell.com> <54217D0F.90606@bitdefender.com> <54219AD70200007800037C02@mail.emea.novell.com> <5421825E.1040607@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1310760602490925191==" Return-path: In-Reply-To: <5421825E.1040607@bitdefender.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Razvan Cojocaru Cc: Ian Campbell , Tim Deegan , Julien Grall , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============1310760602490925191== Content-Type: multipart/alternative; boundary=001a113a96caf9cbab0503bc62dd --001a113a96caf9cbab0503bc62dd Content-Type: text/plain; charset=ISO-8859-1 On Tue, Sep 23, 2014 at 4:23 PM, Razvan Cojocaru wrote: > On 09/23/2014 05:13 PM, Tamas K Lengyel wrote: > > > > > > On Tue, Sep 23, 2014 at 4:07 PM, Jan Beulich > > wrote: > > > > >>> On 23.09.14 at 16:00, rcojocaru@bitdefender.com>> wrote: > > > On 09/23/2014 04:32 PM, Jan Beulich wrote: > > >>>>> On 23.09.14 at 15:14, tklengyel@sec.in.tum.de>> wrote: > > >>> --- a/xen/common/mem_event.c > > >>> +++ b/xen/common/mem_event.c > > >>> @@ -623,12 +623,9 @@ int mem_event_domctl(struct domain *d, > > >>> xen_domctl_mem_event_op_t *mec, > > >>> HVM_PARAM_ACCESS_RING_PFN, > > >>> mem_access_notification); > > >>> > > >>> - if ( mec->op != > XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE && > > >>> - rc == 0 && > hvm_funcs.enable_msr_exit_interception ) > > >>> - { > > >>> - d->arch.hvm_domain.introspection_enabled = 1; > > >>> - hvm_funcs.enable_msr_exit_interception(d); > > >>> - } > > >>> + if ( !rc && mec->op != > XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE ) > > >>> + p2m_enable_msr_exit_interception(d); > > >> > > >> The name is clearly not suitable for an abstraction - there's > certainly > > >> not going to be MSRs on each and every CPU architecture. Maybe > > >> consult with Razvan on an agreeable more suitable name. > > > > > > P2m_set_up_introspection() perhaps? With the MSR HVM code where > > > applicable, nothing (or something else) where not? Would this be > too > > > generic? > > > > I'd be fine with that name provided the != above gets converted > > to a == XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION. > > > > Jan > > > > > > My problem with this name is that introspection is really way too > > generic of a term. You can certainly do all sorts of introspection > > without having this feature or using this feature.. Ultimately its just > > a name so if this becomes Xen's terminology to mean this particular > > feature I'm fine with it but that's going to be confusing when other > > people talk about 'introspection'. > > "Introspection" in general, yes, is a bit generic. However, the > "MEM_EVENT_OP_ACCESS" part of > XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION, and the "p2m_" part > of "p2m_set_up_introspection()" would, I think, narrow it down a bit more. > > But it is, of course, ultimately up to you (and the Xen maintainers). It > was merely a suggestion. > > > Regards, > Razvan Cojocaru > Thanks, I guess we can keep that name for the function with a comment saying this is used to enable additional arch-specific introspection options, such as MSR interception on x86. Maybe there will be more in the future which could be put in here beside what it is used for at the moment. Tamas --001a113a96caf9cbab0503bc62dd Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Tue, Sep 23, 2014 at 4:23 PM, Razvan Cojocaru <= rcojocaru@bi= tdefender.com> wrote:
On 09/23/2014 05:13 PM, Tamas K Lengyel wrote:
>
>
> On Tue, Sep 23, 2014 at 4:07 PM, Jan Beulich <JBeulich@suse.com
> <mailto:JBeulich@suse.c= om>> wrote:
>
>=A0 =A0 =A0>>> On 23.09.14 at 16:00, <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wr= ote:
>=A0 =A0 =A0> On 09/23/2014 04:32 PM, Jan Beulich wrote:
>=A0 =A0 =A0>>>>> On 23.09.= 14 at 15:14, <tklengyel@sec.i= n.tum.de <mailto:tklengye= l@sec.in.tum.de>> wrote:
>=A0 =A0 =A0>>> --- a/xen/common/mem_event.c
>=A0 =A0 =A0>>> +++ b/xen/common/mem_event.c
>=A0 =A0 =A0>>> @@ -623,12 +623,9 @@ int mem_event_domctl(struc= t domain *d,
>=A0 =A0 =A0>>> xen_domctl_mem_event_op_t *mec,
>=A0 =A0 =A0>>>=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 HVM_PARAM_ACCESS_RING_PFN,
>=A0 =A0 =A0>>>=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 mem_access_notification);
>=A0 =A0 =A0>>>
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 if ( mec->op !=3D X= EN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE &&
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rc =3D=3D 0= && hvm_funcs.enable_msr_exit_interception )
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 {
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 d->arch.hvm= _domain.introspection_enabled =3D 1;
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hvm_funcs.enab= le_msr_exit_interception(d);
>=A0 =A0 =A0>>> -=A0 =A0 =A0 =A0 =A0 =A0 }
>=A0 =A0 =A0>>> +=A0 =A0 =A0 =A0 =A0 =A0 if ( !rc && me= c->op !=3D XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE )
>=A0 =A0 =A0>>> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p2m_enable_msr= _exit_interception(d);
>=A0 =A0 =A0>>
>=A0 =A0 =A0>> The name is clearly not suitable for an abstraction= - there's certainly
>=A0 =A0 =A0>> not going to be MSRs on each and every CPU architec= ture. Maybe
>=A0 =A0 =A0>> consult with Razvan on an agreeable more suitable n= ame.
>=A0 =A0 =A0>
>=A0 =A0 =A0> P2m_set_up_introspection() perhaps? With the MSR HVM co= de where
>=A0 =A0 =A0> applicable, nothing (or something else) where not? Woul= d this be too
>=A0 =A0 =A0> generic?
>
>=A0 =A0 =A0I'd be fine with that name provided the !=3D above gets = converted
>=A0 =A0 =A0to a =3D=3D XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECT= ION.
>
>=A0 =A0 =A0Jan
>
>
> My problem with this name is that introspection is really way too
> generic of a term. You can certainly do all sorts of introspection
> without having this feature or using this feature.. Ultimately its jus= t
> a name so if this becomes Xen's terminology to mean this particula= r
> feature I'm fine with it but that's going to be confusing when= other
> people talk about 'introspection'.

"Introspection" in general, yes, is a bit generic. Ho= wever, the
"MEM_EVENT_OP_ACCESS" part of
XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION, and the "p2m_&quo= t; part
of "p2m_set_up_introspection()" would, I think, narrow it down a = bit more.

But it is, of course, ultimately up to you (and the Xen maintainers). It was merely a suggestion.


Regards,
Razvan Cojocaru

Thanks, I guess we = can keep that name for the function with a comment saying this is used to e= nable additional arch-specific introspection options, such as MSR intercept= ion on x86. Maybe there will be more in the future which could be put in he= re beside what it is used for at the moment.

Tamas

--001a113a96caf9cbab0503bc62dd-- --===============1310760602490925191== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1310760602490925191==--