Re: [Xen-devel] [PATCH v3 1/2] tools: use '__i386__' and '__x86_64__' to replace PSR macros

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 04, 2017 at 07:01:43PM +0800, Yi Sun wrote:
> The libxl interfaces and related functions are not necessary to be included by
> 'LIBXL_HAVE_PSR_CMT' and 'LIBXL_HAVE_PSR_CAT'. So replace them to common x86
> macros. Furthermore, only compile 'xl_psr.c' under x86.
> 
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Suggested-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

With some comments, that I would let the maintainers decide what's
best to do.

To make sure this doesn't break ARM I've pushed it to a git branch on
my repo:

https://travis-ci.org/royger/xen/builds/271736602

> ---
>     - This patch is not tested to be compiled under ARM.
> v3:
>     - replace PSR macros to '__i386__' and '__x86_64__'.
>       (suggested by Wei Liu)
>     - only compile 'xl_psr.c' under x86.
>       (suggested by Roger Pau Monné)
> v2:
>     - remove the macros in xl.h.
>       (suggested by Roger Pau Monné)
> ---
>  tools/libxl/libxl.h    | 6 +-----
>  tools/xl/Makefile      | 5 +++--
>  tools/xl/xl.h          | 4 +---
>  tools/xl/xl_cmdtable.c | 5 +----
>  tools/xl/xl_psr.c      | 5 -----
>  5 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 229e289..6c087dd 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -2166,7 +2166,7 @@ bool libxl_ms_vm_genid_is_zero(const libxl_ms_vm_genid 
> *id);
>  void libxl_ms_vm_genid_copy(libxl_ctx *ctx, libxl_ms_vm_genid *dst,
>                              const libxl_ms_vm_genid *src);
>  
> -#ifdef LIBXL_HAVE_PSR_CMT
> +#if defined(__i386__) || defined(__x86_64__)

I not sure you actually need to guard the prototypes, the build is
going to fail at link time anyway.

>  int libxl_psr_cmt_attach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
> @@ -2179,9 +2179,7 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
>                                        uint32_t domid,
>                                        uint32_t socketid,
>                                        uint32_t *l3_cache_occupancy);
> -#endif
>  
> -#ifdef LIBXL_HAVE_PSR_MBM
>  int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
>  int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
>                               uint32_t domid,
> @@ -2189,9 +2187,7 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
>                               uint64_t scope,
>                               uint64_t *sample_r,
>                               uint64_t *tsc_r);
> -#endif
>  
> -#ifdef LIBXL_HAVE_PSR_CAT
>  /*
>   * Function to set a domain's cbm. It operates on a single or multiple
>   * target(s) defined in 'target_map'. The definition of 'target_map' is
> diff --git a/tools/xl/Makefile b/tools/xl/Makefile
> index c868899..ff85d48 100644
> --- a/tools/xl/Makefile
> +++ b/tools/xl/Makefile
> @@ -15,11 +15,12 @@ LDFLAGS += $(PTHREAD_LDFLAGS)
>  CFLAGS_XL += $(CFLAGS_libxenlight)
>  CFLAGS_XL += -Wshadow
>  
> -XL_OBJS = xl.o xl_cmdtable.o xl_sxp.o xl_utils.o
> +XL_OBJS-$(CONFIG_X86) = xl_psr.o
> +XL_OBJS = xl.o xl_cmdtable.o xl_sxp.o xl_utils.o $(XL_OBJS-y)
>  XL_OBJS += xl_tmem.o xl_parse.o xl_cpupool.o xl_flask.o
>  XL_OBJS += xl_vtpm.o xl_block.o xl_nic.o xl_usb.o
>  XL_OBJS += xl_sched.o xl_pci.o xl_vcpu.o xl_cdrom.o xl_mem.o
> -XL_OBJS += xl_psr.o xl_info.o xl_console.o xl_misc.o
> +XL_OBJS += xl_info.o xl_console.o xl_misc.o
>  XL_OBJS += xl_vmcontrol.o xl_saverestore.o xl_migrate.o

I would rather add:

XL_OBJS += $(XL_OBJS-y)

Here rather than at the end of the first line.

>  
>  $(XL_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index aa95b77..8d7b957 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -197,13 +197,11 @@ int main_loadpolicy(int argc, char **argv);
>  int main_remus(int argc, char **argv);
>  #endif
>  int main_devd(int argc, char **argv);
> -#ifdef LIBXL_HAVE_PSR_CMT
> +#if defined(__i386__) || defined(__x86_64__)
>  int main_psr_hwinfo(int argc, char **argv);
>  int main_psr_cmt_attach(int argc, char **argv);
>  int main_psr_cmt_detach(int argc, char **argv);
>  int main_psr_cmt_show(int argc, char **argv);
> -#endif
> -#ifdef LIBXL_HAVE_PSR_CAT
>  int main_psr_cat_cbm_set(int argc, char **argv);
>  int main_psr_cat_show(int argc, char **argv);
>  #endif

Same here, it's going to fail to build at link time anyway AFAICT.

Thanks, Roger.

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