From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v4 01/16] xen: Relocate mem_access and mem_event into common. Date: Fri, 5 Sep 2014 12:52:56 +0200 Message-ID: References: <1409907524-12509-1-git-send-email-tklengyel@sec.in.tum.de> <1409907524-12509-2-git-send-email-tklengyel@sec.in.tum.de> <54099B08020000780003136E@mail.emea.novell.com> <5409A76902000078000315E4@mail.emea.novell.com> <5409B13B0200007800031649@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6418579612823190601==" Return-path: In-Reply-To: <5409B13B0200007800031649@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: Ian Campbell , Tim Deegan , Julien Grall , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============6418579612823190601== Content-Type: multipart/alternative; boundary=001a11c361228df93705024f4573 --001a11c361228df93705024f4573 Content-Type: text/plain; charset=ISO-8859-1 On Fri, Sep 5, 2014 at 12:48 PM, Jan Beulich wrote: > >>> On 05.09.14 at 12:36, wrote: > > On Fri, Sep 5, 2014 at 12:07 PM, Jan Beulich wrote: > > > >> >>> On 05.09.14 at 11:45, wrote: > >> > On Fri, Sep 5, 2014 at 11:14 AM, Jan Beulich > wrote: > >> > > >> >> >>> On 05.09.14 at 10:58, wrote: > >> >> > --- a/xen/arch/x86/x86_64/mm.c > >> >> > +++ b/xen/arch/x86/x86_64/mm.c > >> >> > @@ -35,9 +35,9 @@ > >> >> > #include > >> >> > #include > >> >> > #include > >> >> > -#include > >> >> > +#include > >> >> > #include > >> >> > -#include > >> >> > +#include > >> >> > #include > >> >> > >> >> This is not the only place, but a specifically bad example: I'm > pretty > >> >> sure I asked you before to not make a mess by mixing asm/ and > >> >> xen/ included - move the now xen/ ones to the other ones already > >> >> coming from that directory. And do so consistently throughout the > >> >> patch. > >> >> > >> >> > >> > Sure. Is this a style-thing or does it have some other effect on the > >> code? > >> > >> A style thing that is not supposed to have an effect (we should > >> strive for headers to include their dependencies rather than relying > >> on their users doing so up front). > >> > >> >> Looking at this again I wonder though > >> >> whether we really need these - the use sites could easily check > >> >> whether p2m_is_{paging,shared} is defined instead. > >> > > >> > I would still need to wrap them in CONFIG_X86 blocks as > common/memory.c > >> > does, so ultimately I'm not sure there is much difference. > >> > >> I don't understand why - ARM doesn't define p2m_is_paging() > >> or p2m_is_shared(). > > > > Hm, I guess I could do that, just define them to return 0 and hope the > > compiler just drops the always-dead if blocks. > > No, that's not what I was asking you to do, as that may require you > to defined further stubs used inside the conditionals. Instead I asked > that you replace "#ifdef CONFIG_..." with "#ifdef p2m_is_...". > > Jan > > Oh, OK. Isn't that a bit hack-ish looking though? Tamas --001a11c361228df93705024f4573 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



On Fri, Sep 5, 2014 at 12:48 PM, Jan Beulich <= JBeulich@suse.com> wrote:
>>> On 05.09.14 at 12:36, <tamas.lengyel@zentific.com> wrote:
> On Fri, Sep 5, 2014 at 12:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
>
>> >>> On 05.09.14 at 11:45, <tamas.lengyel@zentific.com> wrote:
>> > On Fri, Sep 5, 2014 at 11:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >
>> >> >>> On 05.09.14 at 10:58, <tklengyel@sec.in.tum.de> wrote:
>> >> > --- a/xen/arch/x86/x86_64/mm.c
>> >> > +++ b/xen/arch/x86/x86_64/mm.c
>> >> > @@ -35,9 +35,9 @@
>> >> >=A0 #include <asm/msr.h>
>> >> >=A0 #include <asm/setup.h>
>> >> >=A0 #include <asm/numa.h>
>> >> > -#include <asm/mem_event.h>
>> >> > +#include <xen/mem_event.h>
>> >> >=A0 #include <asm/mem_sharing.h>
>> >> > -#include <asm/mem_access.h>
>> >> > +#include <xen/mem_access.h>
>> >> >=A0 #include <public/memory.h>
>> >>
>> >> This is not the only place, but a specifically bad exampl= e: I'm pretty
>> >> sure I asked you before to not make a mess by mixing asm/= and
>> >> xen/ included - move the now xen/ ones to the other ones = already
>> >> coming from that directory. And do so consistently throug= hout the
>> >> patch.
>> >>
>> >>
>> > Sure. Is this a style-thing or does it have some other effect= on the
>> code?
>>
>> A style thing that is not supposed to have an effect (we should >> strive for headers to include their dependencies rather than relyi= ng
>> on their users doing so up front).
>>
>> >> Looking at this again I wonder though
>> >> whether we really need these - the use sites could easily= check
>> >> whether p2m_is_{paging,shared} is defined instead.
>> >
>> >=A0 I would still need to wrap them in CONFIG_X86 blocks as co= mmon/memory.c
>> > does, so ultimately I'm not sure there is much difference= .
>>
>> I don't understand why - ARM doesn't define p2m_is_paging(= )
>> or p2m_is_shared().
>
> Hm, I guess I could do that, just define = them to return 0 and hope the
> compiler just drops the always-dead if blocks.

No, that's not what I was asking you to do, as that may require = you
to defined further stubs used inside the conditionals. Instead I asked
that you replace "#ifdef CONFIG_..." with "#ifdef p2m_is_...= ".

Jan


Oh, OK. Isn't that a= bit hack-ish looking though?

Tamas

--001a11c361228df93705024f4573-- --===============6418579612823190601== 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 --===============6418579612823190601==--