From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH RFC v2 04/12] xen/mem_event: Abstract architecture specific sanity checks Date: Thu, 28 Aug 2014 10:40:35 +0200 Message-ID: References: <1409148400-14810-1-git-send-email-tklengyel@sec.in.tum.de> <1409148400-14810-5-git-send-email-tklengyel@sec.in.tum.de> <53FE133D020000780002E27B@mail.emea.novell.com> <53FEEA7D020000780002E74C@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4134491101836089800==" Return-path: In-Reply-To: <53FEEA7D020000780002E74C@mail.emea.novell.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: Jan Beulich Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============4134491101836089800== Content-Type: multipart/alternative; boundary=001a11343e1e89855d0501ac7d2f --001a11343e1e89855d0501ac7d2f Content-Type: text/plain; charset=ISO-8859-1 On Thu, Aug 28, 2014 at 8:38 AM, Jan Beulich wrote: > >>> On 27.08.14 at 23:54, wrote: > >> On Wed, Aug 27, 2014 at 5:19 PM, Jan Beulich > wrote: > >> > >>> >>> On 27.08.14 at 16:06, wrote: > >>> > --- a/xen/common/mem_event.c > >>> > +++ b/xen/common/mem_event.c > >>> > @@ -424,6 +424,19 @@ int __mem_event_claim_slot(struct domain *d, > >>> struct mem_event_domain *med, > >>> > return mem_event_grab_slot(med, (current->domain != d)); > >>> > } > >>> > > >>> > +static inline bool_t mem_event_sanity_check(struct domain *d) > >>> > +{ > >>> > + /* Only HAP is supported */ > >>> > + if ( !hap_enabled(d) ) > >>> > + return 0; > >>> > + > >>> > + /* Currently only EPT is supported */ > >>> > + if ( !cpu_has_vmx ) > >>> > + return 0; > >>> > + > >>> > + return 1; > >>> > +} > >>> > >>> So what does it buy us to have this in a separate function, but > >>> still in the same common file? > >>> > >> > >> This patch really just sets up the ground for ARM where these checks are > >> not required and will just return 1. > >> > > > > In the next series I'll actually relocate this function into architecture > > specific p2m.h and rename it p2m_mem_event_sanity_check. Same for the > > mem_access sanity check function. > > Sounds like suboptimal patch splitting then... > > Jan > Suboptimal in what sense? From a performance perspective it has no impact as it is static inline. I could add the ARM side here as well, but the compilation of this code is not turned on for ARM until the end of the series. Tamas --001a11343e1e89855d0501ac7d2f Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



On Thu, Aug 28, 2014 at 8:38 AM, Jan Beulich <= JBeulich@suse.com> wrote:
>= >> On 27.08.14 at 23:54, <tamas.lengyel@zentific.com> wrote:
>>=A0 On Wed, Aug 27, 2014 at 5:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>
>>> >>> On 27.08.14 at 16:06, <tklengyel@sec.in.tum.de> wrote:
>>> > --- a/xen/common/mem_event.c
>>> > +++ b/xen/common/mem_event.c
>>> > @@ -424,6 +424,19 @@ int __mem_event_claim_slot(struct do= main *d,
>>> struct mem_event_domain *med,
>>> >=A0 =A0 =A0 =A0 =A0 return mem_event_grab_slot(med, (curre= nt->domain !=3D d));
>>> >=A0 }
>>> >
>>> > +static inline bool_t mem_event_sanity_check(struct domai= n *d)
>>> > +{
>>> > +=A0 =A0 /* Only HAP is supported */
>>> > +=A0 =A0 if ( !hap_enabled(d) )
>>> > +=A0 =A0 =A0 =A0 return 0;
>>> > +
>>> > +=A0 =A0 /* Currently only EPT is supported */
>>> > +=A0 =A0 if ( !cpu_has_vmx )
>>> > +=A0 =A0 =A0 =A0 return 0;
>>> > +
>>> > +=A0 =A0 return 1;
>>> > +}
>>>
>>> So what does it buy us to have this in a separate function, bu= t
>>> still in the same common file?
>>>
>>
>> This patch really just sets up the ground for ARM where these chec= ks are
>> not required and will just return 1.
>>
>
> In the next series I'll actually relocate this function into archi= tecture
> specific p2m.h and rename it p2m_mem_event_sanity_check. Same for the<= br> > mem_access sanity check function.

Sounds like suboptimal patch splitting then...

Jan

Suboptimal in what se= nse? From a performance perspective it has no impact as it is static inline= . I could add the ARM side here as well, but the compilation of this code i= s not turned on for ARM until the end of the series.

Tamas

--001a11343e1e89855d0501ac7d2f-- --===============4134491101836089800== 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 --===============4134491101836089800==--