[PATCH] pub-headers: reduce C99 dependencies

[PATCH] pub-headers: reduce C99 dependencies

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 6599 bytes --]

For consumers not using (fully) C99-aware compilers, limit the number
of places where tweaking of the headers would be necessary: Introduce
and use xen_mk_ullong(), allowing its helper macro to be overridden at
once.

For now don't touch public/io/, which also has a few offenders.

The need to include xen.h in hvm/e820.h demonstrates that it is a bad
idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
adjustment just because of this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder why all those ARM constants carry the ULL suffix despite only
two of them actually exceeding 32 significant bits.

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -16,7 +16,6 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <public/hvm/e820.h>
 #include <xen/domain_page.h>
 #include <asm/e820.h>
 #include <asm/iocap.h>
@@ -25,6 +24,7 @@
 #include <asm/mtrr.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/cacheattr.h>
+#include <public/hvm/e820.h>
 
 /* Get page attribute fields (PAn) from PAT MSR. */
 #define pat_cr_2_paf(pat_cr,n)  ((((uint64_t)pat_cr) >> ((n)<<3)) & 0xff)
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -391,38 +391,38 @@ typedef uint64_t xen_callback_t;
  */
 
 /* vGIC v2 mappings */
-#define GUEST_GICD_BASE   0x03001000ULL
-#define GUEST_GICD_SIZE   0x00001000ULL
-#define GUEST_GICC_BASE   0x03002000ULL
-#define GUEST_GICC_SIZE   0x00002000ULL
+#define GUEST_GICD_BASE   xen_mk_ullong(0x03001000)
+#define GUEST_GICD_SIZE   xen_mk_ullong(0x00001000)
+#define GUEST_GICC_BASE   xen_mk_ullong(0x03002000)
+#define GUEST_GICC_SIZE   xen_mk_ullong(0x00002000)
 
 /* vGIC v3 mappings */
-#define GUEST_GICV3_GICD_BASE      0x03001000ULL
-#define GUEST_GICV3_GICD_SIZE      0x00010000ULL
+#define GUEST_GICV3_GICD_BASE      xen_mk_ullong(0x03001000)
+#define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
 
-#define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
+#define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
 #define GUEST_GICV3_RDIST_REGIONS  1
 
-#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU127 */
-#define GUEST_GICV3_GICR0_SIZE     0x01000000ULL
+#define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
+#define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
 
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
  */
-#define GUEST_GNTTAB_BASE 0x38000000ULL
-#define GUEST_GNTTAB_SIZE 0x01000000ULL
+#define GUEST_GNTTAB_BASE xen_mk_ullong(0x38000000)
+#define GUEST_GNTTAB_SIZE xen_mk_ullong(0x01000000)
 
-#define GUEST_MAGIC_BASE  0x39000000ULL
-#define GUEST_MAGIC_SIZE  0x01000000ULL
+#define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
+#define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
 
 #define GUEST_RAM_BANKS   2
 
-#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of low RAM @ 1GB */
-#define GUEST_RAM0_SIZE   0xc0000000ULL
+#define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
+#define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
 
-#define GUEST_RAM1_BASE   0x0200000000ULL /* 1016GB of RAM @ 8GB */
-#define GUEST_RAM1_SIZE   0xfe00000000ULL
+#define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
+#define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
 
 #define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */
 /* Largest amount of actual RAM, not including holes */
--- a/xen/include/public/hvm/e820.h
+++ b/xen/include/public/hvm/e820.h
@@ -23,6 +23,8 @@
 #ifndef __XEN_PUBLIC_HVM_E820_H__
 #define __XEN_PUBLIC_HVM_E820_H__
 
+#include "../xen.h"
+
 /* E820 location in HVM virtual address space. */
 #define HVM_E820_PAGE        0x00090000
 #define HVM_E820_NR_OFFSET   0x000001E8
@@ -30,6 +32,7 @@
 
 #define HVM_BELOW_4G_RAM_END        0xF0000000
 #define HVM_BELOW_4G_MMIO_START     HVM_BELOW_4G_RAM_END
-#define HVM_BELOW_4G_MMIO_LENGTH    ((1ULL << 32) - HVM_BELOW_4G_MMIO_START)
+#define HVM_BELOW_4G_MMIO_LENGTH    ((xen_mk_ullong(1) << 32) - \
+                                     HVM_BELOW_4G_MMIO_START)
 
 #endif /* __XEN_PUBLIC_HVM_E820_H__ */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -486,7 +486,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_access_o
  * for sharing utilities sitting as "filters" in IO backends
  * (e.g. memshr + blktap(2)). The IO backend is only exposed 
  * to grant references, and this allows sharing of the grefs */
-#define XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG   (1ULL << 62)
+#define XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG   (xen_mk_ullong(1) << 62)
 
 #define XENMEM_SHARING_OP_FIELD_MAKE_GREF(field, val)  \
     (field) = (XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG | val)
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -88,7 +88,7 @@ struct vcpu_runstate_info {
      * When activated via VMASST_TYPE_runstate_update_flag, set during
      * updates in guest memory mapped copy of vcpu_runstate_info.
      */
-#define XEN_RUNSTATE_UPDATE          (1ULL << 63)
+#define XEN_RUNSTATE_UPDATE          (xen_mk_ullong(1) << 63)
     /*
      * Time spent in each RUNSTATE_* (ns). The sum of these times is
      * guaranteed not to drift from system time.
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -53,17 +53,22 @@ DEFINE_XEN_GUEST_HANDLE(uint64_t);
 DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
 DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
-/* Turn a plain number into a C unsigned (long) constant. */
+/* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U
 #define __xen_mk_ulong(x) x ## UL
+#ifndef __xen_mk_ullong
+# define __xen_mk_ullong(x) x ## ULL
+#endif
 #define xen_mk_uint(x)    __xen_mk_uint(x)
 #define xen_mk_ulong(x)   __xen_mk_ulong(x)
+#define xen_mk_ullong(x)  __xen_mk_ullong(x)
 
 #else
 
 /* In assembly code we cannot use C numeric constant suffixes. */
-#define xen_mk_uint(x)  x
-#define xen_mk_ulong(x) x
+#define xen_mk_uint(x)   x
+#define xen_mk_ulong(x)  x
+#define xen_mk_ullong(x) x
 
 #endif
 
--- a/xen/include/public/xenoprof.h
+++ b/xen/include/public/xenoprof.h
@@ -68,7 +68,7 @@ struct event_log {
 };
 
 /* PC value that indicates a special code */
-#define XENOPROF_ESCAPE_CODE (~0ULL)
+#define XENOPROF_ESCAPE_CODE (~xen_mk_ullong(0))
 /* Transient events for the xenoprof->oprofile cpu buf */
 #define XENOPROF_TRACE_BEGIN 1
 



[-- Attachment #2: public-no-ULL.patch --]
[-- Type: text/plain, Size: 6635 bytes --]

pub-headers: reduce C99 dependencies

For consumers not using (fully) C99-aware compilers, limit the number
of places where tweaking of the headers would be necessary: Introduce
and use xen_mk_ullong(), allowing its helper macro to be overridden at
once.

For now don't touch public/io/, which also has a few offenders.

The need to include xen.h in hvm/e820.h demonstrates that it is a bad
idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
adjustment just because of this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder why all those ARM constants carry the ULL suffix despite only
two of them actually exceeding 32 significant bits.

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -16,7 +16,6 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <public/hvm/e820.h>
 #include <xen/domain_page.h>
 #include <asm/e820.h>
 #include <asm/iocap.h>
@@ -25,6 +24,7 @@
 #include <asm/mtrr.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/cacheattr.h>
+#include <public/hvm/e820.h>
 
 /* Get page attribute fields (PAn) from PAT MSR. */
 #define pat_cr_2_paf(pat_cr,n)  ((((uint64_t)pat_cr) >> ((n)<<3)) & 0xff)
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -391,38 +391,38 @@ typedef uint64_t xen_callback_t;
  */
 
 /* vGIC v2 mappings */
-#define GUEST_GICD_BASE   0x03001000ULL
-#define GUEST_GICD_SIZE   0x00001000ULL
-#define GUEST_GICC_BASE   0x03002000ULL
-#define GUEST_GICC_SIZE   0x00002000ULL
+#define GUEST_GICD_BASE   xen_mk_ullong(0x03001000)
+#define GUEST_GICD_SIZE   xen_mk_ullong(0x00001000)
+#define GUEST_GICC_BASE   xen_mk_ullong(0x03002000)
+#define GUEST_GICC_SIZE   xen_mk_ullong(0x00002000)
 
 /* vGIC v3 mappings */
-#define GUEST_GICV3_GICD_BASE      0x03001000ULL
-#define GUEST_GICV3_GICD_SIZE      0x00010000ULL
+#define GUEST_GICV3_GICD_BASE      xen_mk_ullong(0x03001000)
+#define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
 
-#define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
+#define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
 #define GUEST_GICV3_RDIST_REGIONS  1
 
-#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU127 */
-#define GUEST_GICV3_GICR0_SIZE     0x01000000ULL
+#define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
+#define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
 
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
  */
-#define GUEST_GNTTAB_BASE 0x38000000ULL
-#define GUEST_GNTTAB_SIZE 0x01000000ULL
+#define GUEST_GNTTAB_BASE xen_mk_ullong(0x38000000)
+#define GUEST_GNTTAB_SIZE xen_mk_ullong(0x01000000)
 
-#define GUEST_MAGIC_BASE  0x39000000ULL
-#define GUEST_MAGIC_SIZE  0x01000000ULL
+#define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
+#define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
 
 #define GUEST_RAM_BANKS   2
 
-#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of low RAM @ 1GB */
-#define GUEST_RAM0_SIZE   0xc0000000ULL
+#define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
+#define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
 
-#define GUEST_RAM1_BASE   0x0200000000ULL /* 1016GB of RAM @ 8GB */
-#define GUEST_RAM1_SIZE   0xfe00000000ULL
+#define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
+#define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
 
 #define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */
 /* Largest amount of actual RAM, not including holes */
--- a/xen/include/public/hvm/e820.h
+++ b/xen/include/public/hvm/e820.h
@@ -23,6 +23,8 @@
 #ifndef __XEN_PUBLIC_HVM_E820_H__
 #define __XEN_PUBLIC_HVM_E820_H__
 
+#include "../xen.h"
+
 /* E820 location in HVM virtual address space. */
 #define HVM_E820_PAGE        0x00090000
 #define HVM_E820_NR_OFFSET   0x000001E8
@@ -30,6 +32,7 @@
 
 #define HVM_BELOW_4G_RAM_END        0xF0000000
 #define HVM_BELOW_4G_MMIO_START     HVM_BELOW_4G_RAM_END
-#define HVM_BELOW_4G_MMIO_LENGTH    ((1ULL << 32) - HVM_BELOW_4G_MMIO_START)
+#define HVM_BELOW_4G_MMIO_LENGTH    ((xen_mk_ullong(1) << 32) - \
+                                     HVM_BELOW_4G_MMIO_START)
 
 #endif /* __XEN_PUBLIC_HVM_E820_H__ */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -486,7 +486,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_access_o
  * for sharing utilities sitting as "filters" in IO backends
  * (e.g. memshr + blktap(2)). The IO backend is only exposed 
  * to grant references, and this allows sharing of the grefs */
-#define XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG   (1ULL << 62)
+#define XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG   (xen_mk_ullong(1) << 62)
 
 #define XENMEM_SHARING_OP_FIELD_MAKE_GREF(field, val)  \
     (field) = (XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG | val)
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -88,7 +88,7 @@ struct vcpu_runstate_info {
      * When activated via VMASST_TYPE_runstate_update_flag, set during
      * updates in guest memory mapped copy of vcpu_runstate_info.
      */
-#define XEN_RUNSTATE_UPDATE          (1ULL << 63)
+#define XEN_RUNSTATE_UPDATE          (xen_mk_ullong(1) << 63)
     /*
      * Time spent in each RUNSTATE_* (ns). The sum of these times is
      * guaranteed not to drift from system time.
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -53,17 +53,22 @@ DEFINE_XEN_GUEST_HANDLE(uint64_t);
 DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
 DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
-/* Turn a plain number into a C unsigned (long) constant. */
+/* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U
 #define __xen_mk_ulong(x) x ## UL
+#ifndef __xen_mk_ullong
+# define __xen_mk_ullong(x) x ## ULL
+#endif
 #define xen_mk_uint(x)    __xen_mk_uint(x)
 #define xen_mk_ulong(x)   __xen_mk_ulong(x)
+#define xen_mk_ullong(x)  __xen_mk_ullong(x)
 
 #else
 
 /* In assembly code we cannot use C numeric constant suffixes. */
-#define xen_mk_uint(x)  x
-#define xen_mk_ulong(x) x
+#define xen_mk_uint(x)   x
+#define xen_mk_ulong(x)  x
+#define xen_mk_ullong(x) x
 
 #endif
 
--- a/xen/include/public/xenoprof.h
+++ b/xen/include/public/xenoprof.h
@@ -68,7 +68,7 @@ struct event_log {
 };
 
 /* PC value that indicates a special code */
-#define XENOPROF_ESCAPE_CODE (~0ULL)
+#define XENOPROF_ESCAPE_CODE (~xen_mk_ullong(0))
 /* Transient events for the xenoprof->oprofile cpu buf */
 #define XENOPROF_TRACE_BEGIN 1
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[George Dunlap]

On 28/09/16 13:00, Jan Beulich wrote:
> For consumers not using (fully) C99-aware compilers, limit the number
> of places where tweaking of the headers would be necessary: Introduce
> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> once.

I'm just curious: Did you actually run across someone who had an issue
with this? And for non-C99-aware compilers, what would people typically
have to put instead to get a 64-bit ULL there?

 -George


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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Jan Beulich]

>>> On 28.09.16 at 15:31, <george.dunlap@citrix.com> wrote:
> On 28/09/16 13:00, Jan Beulich wrote:
>> For consumers not using (fully) C99-aware compilers, limit the number
>> of places where tweaking of the headers would be necessary: Introduce
>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>> once.
> 
> I'm just curious: Did you actually run across someone who had an issue
> with this?

No. I just noticed in the context of
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02913.html
that the problem is more widespread than just that patch.

> And for non-C99-aware compilers, what would people typically
> have to put instead to get a 64-bit ULL there?

Windows compilers typically support a UI64 suffix. Other
compilers might use yet something else.

Jan


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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Wei Liu]

On Wed, Sep 28, 2016 at 06:00:31AM -0600, Jan Beulich wrote:
> For consumers not using (fully) C99-aware compilers, limit the number
> of places where tweaking of the headers would be necessary: Introduce
> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> once.
> 
> For now don't touch public/io/, which also has a few offenders.
> 
> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> adjustment just because of this.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder why all those ARM constants carry the ULL suffix despite only
> two of them actually exceeding 32 significant bits.
> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -16,7 +16,6 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <public/hvm/e820.h>
>  #include <xen/domain_page.h>
>  #include <asm/e820.h>
>  #include <asm/iocap.h>
> @@ -25,6 +24,7 @@
>  #include <asm/mtrr.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/cacheattr.h>
> +#include <public/hvm/e820.h>
>  

Unrelated change here.

Wei.

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Jan Beulich]

>>> On 28.09.16 at 16:04, <wei.liu2@citrix.com> wrote:
> On Wed, Sep 28, 2016 at 06:00:31AM -0600, Jan Beulich wrote:
>> For consumers not using (fully) C99-aware compilers, limit the number
>> of places where tweaking of the headers would be necessary: Introduce
>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>> once.
>> 
>> For now don't touch public/io/, which also has a few offenders.
>> 
>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
>> adjustment just because of this.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder why all those ARM constants carry the ULL suffix despite only
>> two of them actually exceeding 32 significant bits.
>> 
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -16,7 +16,6 @@
>>   * this program; If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> -#include <public/hvm/e820.h>
>>  #include <xen/domain_page.h>
>>  #include <asm/e820.h>
>>  #include <asm/iocap.h>
>> @@ -25,6 +24,7 @@
>>  #include <asm/mtrr.h>
>>  #include <asm/hvm/support.h>
>>  #include <asm/hvm/cacheattr.h>
>> +#include <public/hvm/e820.h>
>>  
> 
> Unrelated change here.

Definitely not - see the commit message.

Jan


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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Wei Liu]

On Wed, Sep 28, 2016 at 08:14:51AM -0600, Jan Beulich wrote:
> >>> On 28.09.16 at 16:04, <wei.liu2@citrix.com> wrote:
> > On Wed, Sep 28, 2016 at 06:00:31AM -0600, Jan Beulich wrote:
> >> For consumers not using (fully) C99-aware compilers, limit the number
> >> of places where tweaking of the headers would be necessary: Introduce
> >> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> >> once.
> >> 
> >> For now don't touch public/io/, which also has a few offenders.
> >> 
> >> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> >> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> >> adjustment just because of this.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I wonder why all those ARM constants carry the ULL suffix despite only
> >> two of them actually exceeding 32 significant bits.
> >> 
> >> --- a/xen/arch/x86/hvm/mtrr.c
> >> +++ b/xen/arch/x86/hvm/mtrr.c
> >> @@ -16,7 +16,6 @@
> >>   * this program; If not, see <http://www.gnu.org/licenses/>.
> >>   */
> >>  
> >> -#include <public/hvm/e820.h>
> >>  #include <xen/domain_page.h>
> >>  #include <asm/e820.h>
> >>  #include <asm/iocap.h>
> >> @@ -25,6 +24,7 @@
> >>  #include <asm/mtrr.h>
> >>  #include <asm/hvm/support.h>
> >>  #include <asm/hvm/cacheattr.h>
> >> +#include <public/hvm/e820.h>
> >>  
> > 
> > Unrelated change here.
> 
> Definitely not - see the commit message.
> 

Ah, sorry about the noise.

> Jan
> 

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Julien Grall]

Hi Jan,

On 28/09/2016 05:00, Jan Beulich wrote:
> For consumers not using (fully) C99-aware compilers, limit the number
> of places where tweaking of the headers would be necessary: Introduce
> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> once.
>
> For now don't touch public/io/, which also has a few offenders.
>
> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> adjustment just because of this.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder why all those ARM constants carry the ULL suffix despite only
> two of them actually exceeding 32 significant bits.

I am not the author of the code, but I think it was to declare all the 
constants of a given set uniformed.

For instance all the GUEST_* constants are used to define the layout of 
the guest. This may be shuffle in this future (this is not part of ABI) 
and if we suffix with ULL only 64-bits constant, it would be a call to 
forget when moving from a 32-bit to 64-bit value.

Cheers,

-- 
Julien Grall

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Jan Beulich]

>>> On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
> Hi Jan,
> 
> On 28/09/2016 05:00, Jan Beulich wrote:
>> For consumers not using (fully) C99-aware compilers, limit the number
>> of places where tweaking of the headers would be necessary: Introduce
>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>> once.
>>
>> For now don't touch public/io/, which also has a few offenders.
>>
>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
>> adjustment just because of this.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder why all those ARM constants carry the ULL suffix despite only
>> two of them actually exceeding 32 significant bits.
> 
> I am not the author of the code, but I think it was to declare all the 
> constants of a given set uniformed.
> 
> For instance all the GUEST_* constants are used to define the layout of 
> the guest. This may be shuffle in this future (this is not part of ABI) 

Oh, they're in a Xen/tools only section (the #endif could really be
annotated to help spot this) - in that case I could as well leave them
alone. Any preference?

> and if we suffix with ULL only 64-bits constant, it would be a call to 
> forget when moving from a 32-bit to 64-bit value.

Well, I don't view this as a good reason, but you're the maintainer
of that code ...

Jan


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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Ian Jackson]

Julien Grall writes ("Re: [PATCH] pub-headers: reduce C99 dependencies"):
> I am not the author of the code, but I think it was to declare all the 
> constants of a given set uniformed.
> 
> For instance all the GUEST_* constants are used to define the layout of 
> the guest. This may be shuffle in this future (this is not part of ABI) 
> and if we suffix with ULL only 64-bits constant, it would be a call to 
> forget when moving from a 32-bit to 64-bit value.

Also, it would mean that changes to these constants would lead to
changes to their types which would be undesirable.

Ian.

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Julien Grall]

Hi Jan,

On 28/09/2016 23:04, Jan Beulich wrote:
>>>> On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
>> Hi Jan,
>>
>> On 28/09/2016 05:00, Jan Beulich wrote:
>>> For consumers not using (fully) C99-aware compilers, limit the number
>>> of places where tweaking of the headers would be necessary: Introduce
>>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>>> once.
>>>
>>> For now don't touch public/io/, which also has a few offenders.
>>>
>>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
>>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
>>> adjustment just because of this.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I wonder why all those ARM constants carry the ULL suffix despite only
>>> two of them actually exceeding 32 significant bits.
>>
>> I am not the author of the code, but I think it was to declare all the
>> constants of a given set uniformed.
>>
>> For instance all the GUEST_* constants are used to define the layout of
>> the guest. This may be shuffle in this future (this is not part of ABI)
>
> Oh, they're in a Xen/tools only section (the #endif could really be
> annotated to help spot this) - in that case I could as well leave them
> alone. Any preference?

Correct. I can send a patch to annotate the #endif.

Regards,

-- 
Julien Grall

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Julien Grall]

Hi Jan,

On 28/09/2016 05:00, Jan Beulich wrote:
> For consumers not using (fully) C99-aware compilers, limit the number
> of places where tweaking of the headers would be necessary: Introduce
> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> once.
>
> For now don't touch public/io/, which also has a few offenders.
>
> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> adjustment just because of this.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the ARM parts:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Julien Grall]

On 29/09/2016 12:11, Julien Grall wrote:
> Hi Jan,
>
> On 28/09/2016 23:04, Jan Beulich wrote:
>>>>> On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
>>> Hi Jan,
>>>
>>> On 28/09/2016 05:00, Jan Beulich wrote:
>>>> For consumers not using (fully) C99-aware compilers, limit the number
>>>> of places where tweaking of the headers would be necessary: Introduce
>>>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>>>> once.
>>>>
>>>> For now don't touch public/io/, which also has a few offenders.
>>>>
>>>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
>>>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
>>>> adjustment just because of this.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I wonder why all those ARM constants carry the ULL suffix despite only
>>>> two of them actually exceeding 32 significant bits.
>>>
>>> I am not the author of the code, but I think it was to declare all the
>>> constants of a given set uniformed.
>>>
>>> For instance all the GUEST_* constants are used to define the layout of
>>> the guest. This may be shuffle in this future (this is not part of ABI)
>>
>> Oh, they're in a Xen/tools only section (the #endif could really be
>> annotated to help spot this) - in that case I could as well leave them
>> alone. Any preference?
>
> Correct. I can send a patch to annotate the #endif.

It looks like that I did not reply to your question. Do we expect the 
toolstack to always C99 standard? If not, then we should keep the 
xen_mk_ullong.

I am fine either way.

Cheers,

-- 
Julien Grall

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Wei Liu]

On Thu, Sep 29, 2016 at 12:22:37PM -0700, Julien Grall wrote:
> 
> 
> On 29/09/2016 12:11, Julien Grall wrote:
> >Hi Jan,
> >
> >On 28/09/2016 23:04, Jan Beulich wrote:
> >>>>>On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
> >>>Hi Jan,
> >>>
> >>>On 28/09/2016 05:00, Jan Beulich wrote:
> >>>>For consumers not using (fully) C99-aware compilers, limit the number
> >>>>of places where tweaking of the headers would be necessary: Introduce
> >>>>and use xen_mk_ullong(), allowing its helper macro to be overridden at
> >>>>once.
> >>>>
> >>>>For now don't touch public/io/, which also has a few offenders.
> >>>>
> >>>>The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> >>>>idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> >>>>adjustment just because of this.
> >>>>
> >>>>Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>---
> >>>>I wonder why all those ARM constants carry the ULL suffix despite only
> >>>>two of them actually exceeding 32 significant bits.
> >>>
> >>>I am not the author of the code, but I think it was to declare all the
> >>>constants of a given set uniformed.
> >>>
> >>>For instance all the GUEST_* constants are used to define the layout of
> >>>the guest. This may be shuffle in this future (this is not part of ABI)
> >>
> >>Oh, they're in a Xen/tools only section (the #endif could really be
> >>annotated to help spot this) - in that case I could as well leave them
> >>alone. Any preference?
> >
> >Correct. I can send a patch to annotate the #endif.
> 
> It looks like that I did not reply to your question. Do we expect the
> toolstack to always C99 standard? If not, then we should keep the
> xen_mk_ullong.
> 
> I am fine either way.
> 

We have -std=gnu99 in top-level Config.mk -- both xen and tools will be
built with that.

Wei.

> Cheers,
> 
> -- 
> Julien Grall

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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Jan Beulich]

>>> On 29.09.16 at 21:22, <julien.grall@arm.com> wrote:

> 
> On 29/09/2016 12:11, Julien Grall wrote:
>> Hi Jan,
>>
>> On 28/09/2016 23:04, Jan Beulich wrote:
>>>>>> On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
>>>> Hi Jan,
>>>>
>>>> On 28/09/2016 05:00, Jan Beulich wrote:
>>>>> For consumers not using (fully) C99-aware compilers, limit the number
>>>>> of places where tweaking of the headers would be necessary: Introduce
>>>>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
>>>>> once.
>>>>>
>>>>> For now don't touch public/io/, which also has a few offenders.
>>>>>
>>>>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
>>>>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
>>>>> adjustment just because of this.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> I wonder why all those ARM constants carry the ULL suffix despite only
>>>>> two of them actually exceeding 32 significant bits.
>>>>
>>>> I am not the author of the code, but I think it was to declare all the
>>>> constants of a given set uniformed.
>>>>
>>>> For instance all the GUEST_* constants are used to define the layout of
>>>> the guest. This may be shuffle in this future (this is not part of ABI)
>>>
>>> Oh, they're in a Xen/tools only section (the #endif could really be
>>> annotated to help spot this) - in that case I could as well leave them
>>> alone. Any preference?
>>
>> Correct. I can send a patch to annotate the #endif.
> 
> It looks like that I did not reply to your question. Do we expect the 
> toolstack to always C99 standard? If not, then we should keep the 
> xen_mk_ullong.

We're even expecting GNUisms to be supported, like
__attribute__((aligned(...))). But since you said you're fine either
way, I guess I'll keep the patch as is.

Jan


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

Re: [PATCH] pub-headers: reduce C99 dependencies

[Wei Liu]

On Fri, Sep 30, 2016 at 03:05:18AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 21:22, <julien.grall@arm.com> wrote:
> 
> > 
> > On 29/09/2016 12:11, Julien Grall wrote:
> >> Hi Jan,
> >>
> >> On 28/09/2016 23:04, Jan Beulich wrote:
> >>>>>> On 28.09.16 at 21:42, <julien.grall@arm.com> wrote:
> >>>> Hi Jan,
> >>>>
> >>>> On 28/09/2016 05:00, Jan Beulich wrote:
> >>>>> For consumers not using (fully) C99-aware compilers, limit the number
> >>>>> of places where tweaking of the headers would be necessary: Introduce
> >>>>> and use xen_mk_ullong(), allowing its helper macro to be overridden at
> >>>>> once.
> >>>>>
> >>>>> For now don't touch public/io/, which also has a few offenders.
> >>>>>
> >>>>> The need to include xen.h in hvm/e820.h demonstrates that it is a bad
> >>>>> idea to include public headers first thing - arch/x86/hvm/mtrr.c needs
> >>>>> adjustment just because of this.
> >>>>>
> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>> ---
> >>>>> I wonder why all those ARM constants carry the ULL suffix despite only
> >>>>> two of them actually exceeding 32 significant bits.
> >>>>
> >>>> I am not the author of the code, but I think it was to declare all the
> >>>> constants of a given set uniformed.
> >>>>
> >>>> For instance all the GUEST_* constants are used to define the layout of
> >>>> the guest. This may be shuffle in this future (this is not part of ABI)
> >>>
> >>> Oh, they're in a Xen/tools only section (the #endif could really be
> >>> annotated to help spot this) - in that case I could as well leave them
> >>> alone. Any preference?
> >>
> >> Correct. I can send a patch to annotate the #endif.
> > 
> > It looks like that I did not reply to your question. Do we expect the 
> > toolstack to always C99 standard? If not, then we should keep the 
> > xen_mk_ullong.
> 
> We're even expecting GNUisms to be supported, like
> __attribute__((aligned(...))). But since you said you're fine either
> way, I guess I'll keep the patch as is.
> 

Acked-by: Wei Liu <wei.liu2@citrix.com>

I will commit this patch soon.

Wei.

> Jan
> 

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