| Message ID | 20230227120957.10037-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: use xen-tools/libs.h for common definitions | expand |
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
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
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
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
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
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
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
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
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(-)