Re: [PATCH v4 01/16] xen: Relocate mem_access and mem_event into common.

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2333 bytes --]

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 @@
> >> >> >  #include <asm/msr.h>
> >> >> >  #include <asm/setup.h>
> >> >> >  #include <asm/numa.h>
> >> >> > -#include <asm/mem_event.h>
> >> >> > +#include <xen/mem_event.h>
> >> >> >  #include <asm/mem_sharing.h>
> >> >> > -#include <asm/mem_access.h>
> >> >> > +#include <xen/mem_access.h>
> >> >> >  #include <public/memory.h>
> >> >>
> >> >> 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

[-- Attachment #1.2: Type: text/html, Size: 3777 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel