* [PATCH 0/3] tools: use xen-tools/libs.h for common definitions
@ 2023-02-27 12:09 Juergen Gross
2023-02-27 12:09 ` [PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h Juergen Gross
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 12:09 UTC (permalink / raw
To: xen-devel
Cc: Juergen Gross, Wei Liu, Anthony PERARD, Jan Beulich,
Andrew Cooper, Roger Pau Monné, Julien Grall
There are some macros defined multiple times in tools. Use only
xen-tools/libs.h for defining those macros and drop the copies.
Juergen Gross (3):
tools: add container_of() macro to xen-tools/libs.h
tools: get rid of additional min() and max() definitions
tools: add offsetof() to xen-tools/libs.h
tools/firmware/hvmloader/util.h | 11 ++---------
tools/firmware/include/stddef.h | 4 ++--
tools/include/xen-tools/libs.h | 8 ++++++++
tools/libfsimage/Rules.mk | 2 ++
tools/libfsimage/xfs/fsys_xfs.c | 4 +---
tools/libs/vchan/init.c | 7 +------
tools/tests/vhpet/emul.h | 14 --------------
tools/tests/vpci/emul.h | 22 +---------------------
tools/tests/x86_emulator/x86-emulate.h | 5 -----
tools/xenstore/list.h | 6 ++----
10 files changed, 19 insertions(+), 64 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h
2023-02-27 12:09 [PATCH 0/3] tools: use xen-tools/libs.h for common definitions Juergen Gross
@ 2023-02-27 12:09 ` Juergen Gross
2023-02-27 12:09 ` [PATCH 2/3] tools: get rid of additional min() and max() definitions Juergen Gross
2023-02-27 12:09 ` [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h Juergen Gross
2 siblings, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 12:09 UTC (permalink / raw
To: xen-devel
Cc: Juergen Gross, Wei Liu, Anthony PERARD, Jan Beulich,
Andrew Cooper, Roger Pau Monné, Julien Grall
Instead of having 4 identical copies of the definition of a
container_of() macro in different tools header files, add that macro
to xen-tools/libs.h and use that instead.
Delete the other copies of that macro.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
There is a similar macro CONTAINER_OF() defined in
tools/include/xentoolcore_internal.h, which allows to not only use a
type for the 2nd parameter, but a variable, too.
I'd like to get rid of that macro as well, but there are lots of use
cases especially in libxl. Any thoughts regarding that macro?
I could either:
- don't touch it at all
- enhance container_of() like CONTAINER_OF() and replace all use cases
of CONTAINER_OF() with container_of()
- replace the few CONTAINER_OF() users outside libxl with container_of()
and define CONTAINER_OF() in e.g. libxl_internal.h
- replace all CONTAINER_OF() use cases with container_of(), including
the change from (.., var, ..) to (.., type, ...).
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/include/xen-tools/libs.h | 4 ++++
tools/tests/vhpet/emul.h | 3 ---
tools/tests/vpci/emul.h | 6 +-----
tools/tests/x86_emulator/x86-emulate.h | 5 -----
tools/xenstore/list.h | 6 ++----
5 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index bafc90e2f6..3672486daa 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -67,4 +67,8 @@
#define __must_check __attribute__((__warn_unused_result__))
#endif
+#define container_of(ptr, type, member) ({ \
+ typeof( ((type *)0)->member ) *__mptr = (ptr); \
+ (type *)( (char *)__mptr - offsetof(type,member) );})
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index b022cc0eab..7db47c5beb 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -125,9 +125,6 @@ enum
#define max_t(type, x, y) \
({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-#define container_of(ptr, type, member) ({ \
- typeof( ((type *)0)->member ) *__mptr = (ptr); \
- (type *)( (char *)__mptr - offsetof(type,member) ); })
struct domain;
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index f03e3a56d1..237edb4e95 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -27,11 +27,7 @@
#include <stdio.h>
#include <stdlib.h>
-#define container_of(ptr, type, member) ({ \
- typeof(((type *)0)->member) *mptr = (ptr); \
- \
- (type *)((char *)mptr - offsetof(type, member)); \
-})
+#include <xen-tools/libs.h>
#define smp_wmb()
#define prefetch(x) __builtin_prefetch(x)
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index 18ae40d017..0849c07dbc 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -56,11 +56,6 @@
#define cf_check /* No Control Flow Integriy checking */
-#define container_of(ptr, type, member) ({ \
- typeof(((type *)0)->member) *mptr__ = (ptr); \
- (type *)((char *)mptr__ - offsetof(type, member)); \
-})
-
#define AC_(n,t) (n##t)
#define _AC(n,t) AC_(n,t)
diff --git a/tools/xenstore/list.h b/tools/xenstore/list.h
index b17d13e0ec..1f469eafaf 100644
--- a/tools/xenstore/list.h
+++ b/tools/xenstore/list.h
@@ -3,6 +3,8 @@
/* Taken from Linux kernel code, but de-kernelized for userspace. */
#include <stddef.h>
+#include <xen-tools/libs.h>
+
#undef LIST_HEAD_INIT
#undef LIST_HEAD
#undef INIT_LIST_HEAD
@@ -15,10 +17,6 @@
#define LIST_POISON1 ((void *) 0x00100100)
#define LIST_POISON2 ((void *) 0x00200200)
-#define container_of(ptr, type, member) ({ \
- typeof( ((type *)0)->member ) *__mptr = (ptr); \
- (type *)( (char *)__mptr - offsetof(type,member) );})
-
/*
* Simple doubly linked list implementation.
*
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] tools: get rid of additional min() and max() definitions
2023-02-27 12:09 [PATCH 0/3] tools: use xen-tools/libs.h for common definitions Juergen Gross
2023-02-27 12:09 ` [PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h Juergen Gross
@ 2023-02-27 12:09 ` Juergen Gross
2023-02-27 12:09 ` [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h Juergen Gross
2 siblings, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 12:09 UTC (permalink / raw
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, Anthony PERARD
Defining min(), min_t(), max() and max_t() at other places than
xen-tools/libs.h isn't needed, as the definitions in said header can
be used instead.
Same applies to BUILD_BUG_ON() in hvmloader.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/firmware/hvmloader/util.h | 8 ++------
tools/libs/vchan/init.c | 3 +--
tools/tests/vhpet/emul.h | 11 +----------
tools/tests/vpci/emul.h | 16 ----------------
4 files changed, 4 insertions(+), 34 deletions(-)
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 8d95eab28a..69afcc6daa 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -9,6 +9,8 @@
#include <xen/hvm/hvm_info_table.h>
#include "e820.h"
+#include <xen-tools/libs.h>
+
/* Request un-prefixed values from errno.h. */
#define XEN_ERRNO(name, value) name = value,
enum {
@@ -41,12 +43,6 @@ void __assert_failed(const char *assertion, const char *file, int line)
void __bug(const char *file, int line) __attribute__((noreturn));
#define BUG() __bug(__FILE__, __LINE__)
#define BUG_ON(p) do { if (p) BUG(); } while (0)
-#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * !!(p)]))
-
-#define min_t(type,x,y) \
- ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
-#define max_t(type,x,y) \
- ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
#define MB(mb) (mb##ULL << 20)
#define GB(gb) (gb##ULL << 30)
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 9195bd3b98..ea2d7f2c54 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -45,6 +45,7 @@
#include <xen/sys/gntalloc.h>
#include <xen/sys/gntdev.h>
#include <libxenvchan.h>
+#include <xen-tools/libs.h>
#include "vchan.h"
@@ -72,8 +73,6 @@
#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
#endif
-#define max(a,b) ((a > b) ? a : b)
-
static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
{
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index 7db47c5beb..efd04ed313 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -114,16 +114,7 @@ enum
TASKLET_SOFTIRQ,
NR_COMMON_SOFTIRQS
};
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max at all, of course.
- */
-#define min_t(type, x, y) \
- ({ type __x = (x); type __y = (y); __x < __y ? __x : __y; })
-#define max_t(type, x, y) \
- ({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
+
#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
struct domain;
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 237edb4e95..bf3cef5586 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -106,22 +106,6 @@ typedef union {
#define BUG() assert(0)
#define ASSERT_UNREACHABLE() assert(0)
-#define min(x, y) ({ \
- const typeof(x) tx = (x); \
- const typeof(y) ty = (y); \
- \
- (void) (&tx == &ty); \
- tx < ty ? tx : ty; \
-})
-
-#define max(x, y) ({ \
- const typeof(x) tx = (x); \
- const typeof(y) ty = (y); \
- \
- (void) (&tx == &ty); \
- tx > ty ? tx : ty; \
-})
-
#endif
/*
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 12:09 [PATCH 0/3] tools: use xen-tools/libs.h for common definitions Juergen Gross
2023-02-27 12:09 ` [PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h Juergen Gross
2023-02-27 12:09 ` [PATCH 2/3] tools: get rid of additional min() and max() definitions Juergen Gross
@ 2023-02-27 12:09 ` Juergen Gross
2023-02-27 12:31 ` Jan Beulich
2 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 12:09 UTC (permalink / raw
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, Anthony PERARD
Instead of having multiple files defining offsetof(), add the
definition to tools/include/xen-tools/libs.h.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/firmware/hvmloader/util.h | 3 ---
tools/firmware/include/stddef.h | 4 ++--
tools/include/xen-tools/libs.h | 4 ++++
tools/libfsimage/Rules.mk | 2 ++
tools/libfsimage/xfs/fsys_xfs.c | 4 +---
tools/libs/vchan/init.c | 4 ----
tools/tests/vhpet/emul.h | 2 --
7 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 69afcc6daa..668ee74f3c 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
#define SEL_DATA32 0x0020
#define SEL_CODE64 0x0028
-#undef offsetof
-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
#undef NULL
#define NULL ((void*)0)
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
index c7f974608a..926ae12fa5 100644
--- a/tools/firmware/include/stddef.h
+++ b/tools/firmware/include/stddef.h
@@ -1,10 +1,10 @@
#ifndef _STDDEF_H_
#define _STDDEF_H_
+#include <xen-tools/libs.h>
+
typedef __SIZE_TYPE__ size_t;
#define NULL ((void*)0)
-#define offsetof(t, m) __builtin_offsetof(t, m)
-
#endif
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index 3672486daa..c222aa5bb0 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -71,4 +71,8 @@
typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type,member) );})
+#ifndef offsetof
+#define offsetof(a, b) __builtin_offsetof(a, b)
+#endif
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/libfsimage/Rules.mk b/tools/libfsimage/Rules.mk
index cf37d6cb0d..85674f2379 100644
--- a/tools/libfsimage/Rules.mk
+++ b/tools/libfsimage/Rules.mk
@@ -3,6 +3,8 @@ include $(XEN_ROOT)/tools/libfsimage/common.mk
FSLIB = fsimage.so
TARGETS += $(FSLIB)
+CFLAGS += $(CFLAGS_xeninclude)
+
.PHONY: all
all: $(TARGETS)
diff --git a/tools/libfsimage/xfs/fsys_xfs.c b/tools/libfsimage/xfs/fsys_xfs.c
index d735a88e55..4c0cde9777 100644
--- a/tools/libfsimage/xfs/fsys_xfs.c
+++ b/tools/libfsimage/xfs/fsys_xfs.c
@@ -19,6 +19,7 @@
#include <xenfsimage_grub.h>
#include "xfs.h"
+#include <xen-tools/libs.h>
#define MAX_LINK_COUNT 8
@@ -182,9 +183,6 @@ fsb2daddr (xfs_fsblock_t fsbno)
(xfs_agblock_t)(fsbno & mask32lo(xfs.agblklog)));
}
-#undef offsetof
-#define offsetof(t,m) ((size_t)&(((t *)0)->m))
-
static inline int
btroot_maxrecs (fsi_file_t *ffi)
{
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index ea2d7f2c54..9c0c5ca0c5 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -69,10 +69,6 @@
#define MAX_RING_SHIFT 20
#define MAX_RING_SIZE (1 << MAX_RING_SHIFT)
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
-#endif
-
static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
{
int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
index efd04ed313..2eeefa7098 100644
--- a/tools/tests/vhpet/emul.h
+++ b/tools/tests/vhpet/emul.h
@@ -115,8 +115,6 @@ enum
NR_COMMON_SOFTIRQS
};
-#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
-
struct domain;
struct vcpu
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 12:09 ` [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h Juergen Gross
@ 2023-02-27 12:31 ` Jan Beulich
2023-02-27 12:34 ` Juergen Gross
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2023-02-27 12:31 UTC (permalink / raw
To: Juergen Gross
Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Anthony PERARD,
xen-devel
On 27.02.2023 13:09, Juergen Gross wrote:
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -30,9 +30,6 @@ enum {
> #define SEL_DATA32 0x0020
> #define SEL_CODE64 0x0028
>
> -#undef offsetof
> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
> -
> #undef NULL
> #define NULL ((void*)0)
>
> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
> index c7f974608a..926ae12fa5 100644
> --- a/tools/firmware/include/stddef.h
> +++ b/tools/firmware/include/stddef.h
> @@ -1,10 +1,10 @@
> #ifndef _STDDEF_H_
> #define _STDDEF_H_
>
> +#include <xen-tools/libs.h>
I'm not convinced firmware/ files can validly include this header.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 12:31 ` Jan Beulich
@ 2023-02-27 12:34 ` Juergen Gross
2023-02-27 12:43 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 12:34 UTC (permalink / raw
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Anthony PERARD,
xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 970 bytes --]
On 27.02.23 13:31, Jan Beulich wrote:
> On 27.02.2023 13:09, Juergen Gross wrote:
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -30,9 +30,6 @@ enum {
>> #define SEL_DATA32 0x0020
>> #define SEL_CODE64 0x0028
>>
>> -#undef offsetof
>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>> -
>> #undef NULL
>> #define NULL ((void*)0)
>>
>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>> index c7f974608a..926ae12fa5 100644
>> --- a/tools/firmware/include/stddef.h
>> +++ b/tools/firmware/include/stddef.h
>> @@ -1,10 +1,10 @@
>> #ifndef _STDDEF_H_
>> #define _STDDEF_H_
>>
>> +#include <xen-tools/libs.h>
>
> I'm not convinced firmware/ files can validly include this header.
This file only contains macros which don't call any external functions.
Would you be fine with me adding a related comment to it?
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 12:34 ` Juergen Gross
@ 2023-02-27 12:43 ` Jan Beulich
2023-02-27 14:06 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2023-02-27 12:43 UTC (permalink / raw
To: Juergen Gross
Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Anthony PERARD,
xen-devel
On 27.02.2023 13:34, Juergen Gross wrote:
> On 27.02.23 13:31, Jan Beulich wrote:
>> On 27.02.2023 13:09, Juergen Gross wrote:
>>> --- a/tools/firmware/hvmloader/util.h
>>> +++ b/tools/firmware/hvmloader/util.h
>>> @@ -30,9 +30,6 @@ enum {
>>> #define SEL_DATA32 0x0020
>>> #define SEL_CODE64 0x0028
>>>
>>> -#undef offsetof
>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>> -
>>> #undef NULL
>>> #define NULL ((void*)0)
>>>
>>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>>> index c7f974608a..926ae12fa5 100644
>>> --- a/tools/firmware/include/stddef.h
>>> +++ b/tools/firmware/include/stddef.h
>>> @@ -1,10 +1,10 @@
>>> #ifndef _STDDEF_H_
>>> #define _STDDEF_H_
>>>
>>> +#include <xen-tools/libs.h>
>>
>> I'm not convinced firmware/ files can validly include this header.
>
> This file only contains macros which don't call any external functions.
>
> Would you be fine with me adding a related comment to it?
If it was to be usable like this, that would be a prereq, but personally
I don't view this as sufficient. The environment code runs in that lives
under firmware/ is simply different (hvmloader, for example, is 32-bit
no matter that the toolstack would normally be 64-bit), so to me it
feels like setting up a trap for ourselves. If Andrew or Roger are fully
convinced this is a good move, I won't be standing in the way ...
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 12:43 ` Jan Beulich
@ 2023-02-27 14:06 ` Andrew Cooper
2023-02-27 14:10 ` Juergen Gross
2023-02-27 14:12 ` Jan Beulich
0 siblings, 2 replies; 11+ messages in thread
From: Andrew Cooper @ 2023-02-27 14:06 UTC (permalink / raw
To: Jan Beulich, Juergen Gross
Cc: Roger Pau Monné, Wei Liu, Anthony PERARD, xen-devel
On 27/02/2023 12:43 pm, Jan Beulich wrote:
> On 27.02.2023 13:34, Juergen Gross wrote:
>> On 27.02.23 13:31, Jan Beulich wrote:
>>> On 27.02.2023 13:09, Juergen Gross wrote:
>>>> --- a/tools/firmware/hvmloader/util.h
>>>> +++ b/tools/firmware/hvmloader/util.h
>>>> @@ -30,9 +30,6 @@ enum {
>>>> #define SEL_DATA32 0x0020
>>>> #define SEL_CODE64 0x0028
>>>>
>>>> -#undef offsetof
>>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>>> -
>>>> #undef NULL
>>>> #define NULL ((void*)0)
>>>>
>>>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>>>> index c7f974608a..926ae12fa5 100644
>>>> --- a/tools/firmware/include/stddef.h
>>>> +++ b/tools/firmware/include/stddef.h
>>>> @@ -1,10 +1,10 @@
>>>> #ifndef _STDDEF_H_
>>>> #define _STDDEF_H_
>>>>
>>>> +#include <xen-tools/libs.h>
>>> I'm not convinced firmware/ files can validly include this header.
>> This file only contains macros which don't call any external functions.
>>
>> Would you be fine with me adding a related comment to it?
> If it was to be usable like this, that would be a prereq, but personally
> I don't view this as sufficient. The environment code runs in that lives
> under firmware/ is simply different (hvmloader, for example, is 32-bit
> no matter that the toolstack would normally be 64-bit), so to me it
> feels like setting up a trap for ourselves. If Andrew or Roger are fully
> convinced this is a good move, I won't be standing in the way ...
We still support 32bit builds of the toolstack on multiple
architectures, so I don't see bitness as an argument against.
I am slightly uneasy adding a header like this, but it really is only
common macros and I don't see how it could possibly change from that
given the existing uses. OTOH, removing things like the problematic
definition of offsetof() is an improvement.
I'd probably tentatively ack this patch, seeing as it is a minor
improvlement, but I think there's a middle option too. Rename libs.h to
macros.h or common-macros.h? IMO that would be something that's far
more obviously safe to include into firmware/, and something rather more
descriptive at all of its include sites.
Thoughts?
~Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 14:06 ` Andrew Cooper
@ 2023-02-27 14:10 ` Juergen Gross
2023-02-27 14:12 ` Jan Beulich
1 sibling, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2023-02-27 14:10 UTC (permalink / raw
To: Andrew Cooper, Jan Beulich
Cc: Roger Pau Monné, Wei Liu, Anthony PERARD, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2465 bytes --]
On 27.02.23 15:06, Andrew Cooper wrote:
> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>> On 27.02.2023 13:34, Juergen Gross wrote:
>>> On 27.02.23 13:31, Jan Beulich wrote:
>>>> On 27.02.2023 13:09, Juergen Gross wrote:
>>>>> --- a/tools/firmware/hvmloader/util.h
>>>>> +++ b/tools/firmware/hvmloader/util.h
>>>>> @@ -30,9 +30,6 @@ enum {
>>>>> #define SEL_DATA32 0x0020
>>>>> #define SEL_CODE64 0x0028
>>>>>
>>>>> -#undef offsetof
>>>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>>>> -
>>>>> #undef NULL
>>>>> #define NULL ((void*)0)
>>>>>
>>>>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>>>>> index c7f974608a..926ae12fa5 100644
>>>>> --- a/tools/firmware/include/stddef.h
>>>>> +++ b/tools/firmware/include/stddef.h
>>>>> @@ -1,10 +1,10 @@
>>>>> #ifndef _STDDEF_H_
>>>>> #define _STDDEF_H_
>>>>>
>>>>> +#include <xen-tools/libs.h>
>>>> I'm not convinced firmware/ files can validly include this header.
>>> This file only contains macros which don't call any external functions.
>>>
>>> Would you be fine with me adding a related comment to it?
>> If it was to be usable like this, that would be a prereq, but personally
>> I don't view this as sufficient. The environment code runs in that lives
>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>> no matter that the toolstack would normally be 64-bit), so to me it
>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>> convinced this is a good move, I won't be standing in the way ...
>
> We still support 32bit builds of the toolstack on multiple
> architectures, so I don't see bitness as an argument against.
>
> I am slightly uneasy adding a header like this, but it really is only
> common macros and I don't see how it could possibly change from that
> given the existing uses. OTOH, removing things like the problematic
> definition of offsetof() is an improvement.
>
> I'd probably tentatively ack this patch, seeing as it is a minor
> improvlement, but I think there's a middle option too. Rename libs.h to
> macros.h or common-macros.h? IMO that would be something that's far
> more obviously safe to include into firmware/, and something rather more
> descriptive at all of its include sites.
>
> Thoughts?
I'm fine with that.
My preference would be "common-macros.h".
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 14:06 ` Andrew Cooper
2023-02-27 14:10 ` Juergen Gross
@ 2023-02-27 14:12 ` Jan Beulich
2023-02-27 14:17 ` Andrew Cooper
1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2023-02-27 14:12 UTC (permalink / raw
To: Andrew Cooper, Juergen Gross
Cc: Roger Pau Monné, Wei Liu, Anthony PERARD, xen-devel
On 27.02.2023 15:06, Andrew Cooper wrote:
> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>> On 27.02.2023 13:34, Juergen Gross wrote:
>>> On 27.02.23 13:31, Jan Beulich wrote:
>>>> On 27.02.2023 13:09, Juergen Gross wrote:
>>>>> --- a/tools/firmware/hvmloader/util.h
>>>>> +++ b/tools/firmware/hvmloader/util.h
>>>>> @@ -30,9 +30,6 @@ enum {
>>>>> #define SEL_DATA32 0x0020
>>>>> #define SEL_CODE64 0x0028
>>>>>
>>>>> -#undef offsetof
>>>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>>>> -
>>>>> #undef NULL
>>>>> #define NULL ((void*)0)
>>>>>
>>>>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>>>>> index c7f974608a..926ae12fa5 100644
>>>>> --- a/tools/firmware/include/stddef.h
>>>>> +++ b/tools/firmware/include/stddef.h
>>>>> @@ -1,10 +1,10 @@
>>>>> #ifndef _STDDEF_H_
>>>>> #define _STDDEF_H_
>>>>>
>>>>> +#include <xen-tools/libs.h>
>>>> I'm not convinced firmware/ files can validly include this header.
>>> This file only contains macros which don't call any external functions.
>>>
>>> Would you be fine with me adding a related comment to it?
>> If it was to be usable like this, that would be a prereq, but personally
>> I don't view this as sufficient. The environment code runs in that lives
>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>> no matter that the toolstack would normally be 64-bit), so to me it
>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>> convinced this is a good move, I won't be standing in the way ...
>
> We still support 32bit builds of the toolstack on multiple
> architectures, so I don't see bitness as an argument against.
Bitness by itself wasn't the argument. Mixed bitness is what concerns
me.
> I am slightly uneasy adding a header like this, but it really is only
> common macros and I don't see how it could possibly change from that
> given the existing uses. OTOH, removing things like the problematic
> definition of offsetof() is an improvement.
>
> I'd probably tentatively ack this patch, seeing as it is a minor
> improvlement, but I think there's a middle option too. Rename libs.h to
> macros.h or common-macros.h? IMO that would be something that's far
> more obviously safe to include into firmware/, and something rather more
> descriptive at all of its include sites.
>
> Thoughts?
Maybe. One fundamental requirement would need to be made prominently
visible in such a header: It has to be entirely self-contained, i.e.
it may in particular not gain any dependencies on configure's output.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h
2023-02-27 14:12 ` Jan Beulich
@ 2023-02-27 14:17 ` Andrew Cooper
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2023-02-27 14:17 UTC (permalink / raw
To: Jan Beulich, Juergen Gross
Cc: Roger Pau Monné, Wei Liu, Anthony PERARD, xen-devel
On 27/02/2023 2:12 pm, Jan Beulich wrote:
> On 27.02.2023 15:06, Andrew Cooper wrote:
>> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>>> On 27.02.2023 13:34, Juergen Gross wrote:
>>>> On 27.02.23 13:31, Jan Beulich wrote:
>>>>> On 27.02.2023 13:09, Juergen Gross wrote:
>>>>>> --- a/tools/firmware/hvmloader/util.h
>>>>>> +++ b/tools/firmware/hvmloader/util.h
>>>>>> @@ -30,9 +30,6 @@ enum {
>>>>>> #define SEL_DATA32 0x0020
>>>>>> #define SEL_CODE64 0x0028
>>>>>>
>>>>>> -#undef offsetof
>>>>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>>>>> -
>>>>>> #undef NULL
>>>>>> #define NULL ((void*)0)
>>>>>>
>>>>>> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
>>>>>> index c7f974608a..926ae12fa5 100644
>>>>>> --- a/tools/firmware/include/stddef.h
>>>>>> +++ b/tools/firmware/include/stddef.h
>>>>>> @@ -1,10 +1,10 @@
>>>>>> #ifndef _STDDEF_H_
>>>>>> #define _STDDEF_H_
>>>>>>
>>>>>> +#include <xen-tools/libs.h>
>>>>> I'm not convinced firmware/ files can validly include this header.
>>>> This file only contains macros which don't call any external functions.
>>>>
>>>> Would you be fine with me adding a related comment to it?
>>> If it was to be usable like this, that would be a prereq, but personally
>>> I don't view this as sufficient. The environment code runs in that lives
>>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>>> no matter that the toolstack would normally be 64-bit), so to me it
>>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>>> convinced this is a good move, I won't be standing in the way ...
>> We still support 32bit builds of the toolstack on multiple
>> architectures, so I don't see bitness as an argument against.
> Bitness by itself wasn't the argument. Mixed bitness is what concerns
> me.
I still don't see how that would matter. The bitness would always be
consistent inside a single translation.
>
>> I am slightly uneasy adding a header like this, but it really is only
>> common macros and I don't see how it could possibly change from that
>> given the existing uses. OTOH, removing things like the problematic
>> definition of offsetof() is an improvement.
>>
>> I'd probably tentatively ack this patch, seeing as it is a minor
>> improvlement, but I think there's a middle option too. Rename libs.h to
>> macros.h or common-macros.h? IMO that would be something that's far
>> more obviously safe to include into firmware/, and something rather more
>> descriptive at all of its include sites.
>>
>> Thoughts?
> Maybe. One fundamental requirement would need to be made prominently
> visible in such a header: It has to be entirely self-contained, i.e.
> it may in particular not gain any dependencies on configure's output.
That's a reasonable restriction IMO. Again, I don't see how we'd ever
come to violate that in the future, given the current use here.
~Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-27 14:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-27 12:09 [PATCH 0/3] tools: use xen-tools/libs.h for common definitions Juergen Gross
2023-02-27 12:09 ` [PATCH 1/3] tools: add container_of() macro to xen-tools/libs.h Juergen Gross
2023-02-27 12:09 ` [PATCH 2/3] tools: get rid of additional min() and max() definitions Juergen Gross
2023-02-27 12:09 ` [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h Juergen Gross
2023-02-27 12:31 ` Jan Beulich
2023-02-27 12:34 ` Juergen Gross
2023-02-27 12:43 ` Jan Beulich
2023-02-27 14:06 ` Andrew Cooper
2023-02-27 14:10 ` Juergen Gross
2023-02-27 14:12 ` Jan Beulich
2023-02-27 14:17 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).