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 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/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -67,6 +67,9 @@
>  #define NR_CPUS 256
>  #endif
>
> +#define CONFIG_MEM_SHARING 1
> +#define CONFIG_MEM_PAGING 1
> +
>  /* Maximum we can support with current vLAPIC ID mapping. */
>  #define MAX_HVM_VCPUS 128

The addition should go alongside the other CONFIG_* ones instead
of at an arbitrary place.

Ack.

 

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.

Jan

 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.

 

Tamas

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