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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Sep 5, 2014 at 12:48 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 05.09.14 at 12:36, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Fri, Sep 5, 2014 at 12:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
>> >>> On 05.09.14 at 11:45, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> > On Fri, Sep 5, 2014 at 11:14 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >
>> >> >>> On 05.09.14 at 10:58, <tklengyel@xxxxxxxxxxxxx> 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

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