[PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header

Frediano Ziglio posted 4 patches 1 month, 3 weeks ago
[PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Frediano Ziglio 1 month, 3 weeks ago
This allows to include other headers and avoid duplicated declarations.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/include/boot/public/xen.h | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 xen/arch/x86/include/boot/public/xen.h

diff --git a/xen/arch/x86/include/boot/public/xen.h b/xen/arch/x86/include/boot/public/xen.h
new file mode 100644
index 0000000000..399b86b5e5
--- /dev/null
+++ b/xen/arch/x86/include/boot/public/xen.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* This header allows the inclusion of public xen.h */
+
+#ifndef BOOT__PUBLIC__XEN_H
+#define BOOT__PUBLIC__XEN_H
+
+#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
+#error Unexpected defines
+#endif
+
+#include <xen/types.h>
+
+#ifdef __i386__
+
+# define __XEN_TOOLS__ 1
+# undef __XEN__
+# include <public/arch-x86/xen.h>
+# define __XEN__ 1
+# undef __XEN_TOOLS__
+
+#else
+
+# include <public/arch-x86/xen.h>
+
+#endif
+
+#endif /* BOOT__PUBLIC__XEN_H */
-- 
2.34.1
Re: [PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Jan Beulich 1 month ago
On 22.11.2024 10:33, Frediano Ziglio wrote:
> This allows to include other headers and avoid duplicated declarations.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Again it's left unclear what the purpose / goal is.

> --- /dev/null
> +++ b/xen/arch/x86/include/boot/public/xen.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* This header allows the inclusion of public xen.h */
> +
> +#ifndef BOOT__PUBLIC__XEN_H
> +#define BOOT__PUBLIC__XEN_H
> +
> +#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
> +#error Unexpected defines
> +#endif

What is this to guard against? We're in the Xen tree, building Xen.

> +#include <xen/types.h>
> +
> +#ifdef __i386__
> +
> +# define __XEN_TOOLS__ 1
> +# undef __XEN__
> +# include <public/arch-x86/xen.h>
> +# define __XEN__ 1
> +# undef __XEN_TOOLS__

Why would __XEN__ need un-defining and __XEN_TOOLS__ (seemingly wrongly)
need defining? (As an aside, I don't think the expansion of either macro
really matters. IOW I don#t see the need for the two 1-s.)

Jan
Re: [PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Frediano Ziglio 1 month ago
On Tue, Dec 10, 2024 at 10:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.11.2024 10:33, Frediano Ziglio wrote:
> > This allows to include other headers and avoid duplicated declarations.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>
> Again it's left unclear what the purpose / goal is.
>

Reduce duplication avoiding duplicate declarations. The alternative
would be to duplicate them, which was proposed already and refused as
duplication was not good.

> > --- /dev/null
> > +++ b/xen/arch/x86/include/boot/public/xen.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +/* This header allows the inclusion of public xen.h */
> > +
> > +#ifndef BOOT__PUBLIC__XEN_H
> > +#define BOOT__PUBLIC__XEN_H
> > +
> > +#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
> > +#error Unexpected defines
> > +#endif
>
> What is this to guard against? We're in the Xen tree, building Xen.
>

In include/public/arch-x86/xen.h file there are these declarations:

#if defined(__i386__)
# ifdef __XEN__
__DeFiNe__ __DECL_REG_LO8(which) uint32_t e ## which ## x
__DeFiNe__ __DECL_REG_LO16(name) union { uint32_t e ## name; }
# endif
#include "xen-x86_32.h"
# ifdef __XEN__
__UnDeF__ __DECL_REG_LO8
__UnDeF__ __DECL_REG_LO16
__DeFiNe__ __DECL_REG_LO8(which) e ## which ## x
__DeFiNe__ __DECL_REG_LO16(name) e ## name
# endif
#elif defined(__x86_64__)
#include "xen-x86_64.h"
#endif

This header allows us to include that part without compiler errors due
to __DeFiNe__ and __UnDeF__ not being C code.

> > +#include <xen/types.h>
> > +
> > +#ifdef __i386__
> > +
> > +# define __XEN_TOOLS__ 1
> > +# undef __XEN__
> > +# include <public/arch-x86/xen.h>
> > +# define __XEN__ 1
> > +# undef __XEN_TOOLS__
>
> Why would __XEN__ need un-defining and __XEN_TOOLS__ (seemingly wrongly)
> need defining? (As an aside, I don't think the expansion of either macro
> really matters. IOW I don#t see the need for the two 1-s.)
>
> Jan

The "1"-s are the default definitions, simply I define them
temporarily and then restore them.

Frediano
Re: [PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Jan Beulich 1 month ago
On 10.12.2024 15:35, Frediano Ziglio wrote:
> On Tue, Dec 10, 2024 at 10:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 22.11.2024 10:33, Frediano Ziglio wrote:
>>> This allows to include other headers and avoid duplicated declarations.
>>>
>>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>>
>> Again it's left unclear what the purpose / goal is.
>>
> 
> Reduce duplication avoiding duplicate declarations. The alternative
> would be to duplicate them, which was proposed already and refused as
> duplication was not good.

Which declarations specifically?

>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/boot/public/xen.h
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +/* This header allows the inclusion of public xen.h */
>>> +
>>> +#ifndef BOOT__PUBLIC__XEN_H
>>> +#define BOOT__PUBLIC__XEN_H
>>> +
>>> +#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
>>> +#error Unexpected defines
>>> +#endif
>>
>> What is this to guard against? We're in the Xen tree, building Xen.
>>
> 
> In include/public/arch-x86/xen.h file there are these declarations:
> 
> #if defined(__i386__)
> # ifdef __XEN__
> __DeFiNe__ __DECL_REG_LO8(which) uint32_t e ## which ## x
> __DeFiNe__ __DECL_REG_LO16(name) union { uint32_t e ## name; }
> # endif
> #include "xen-x86_32.h"
> # ifdef __XEN__
> __UnDeF__ __DECL_REG_LO8
> __UnDeF__ __DECL_REG_LO16
> __DeFiNe__ __DECL_REG_LO8(which) e ## which ## x
> __DeFiNe__ __DECL_REG_LO16(name) e ## name
> # endif
> #elif defined(__x86_64__)
> #include "xen-x86_64.h"
> #endif
> 
> This header allows us to include that part without compiler errors due
> to __DeFiNe__ and __UnDeF__ not being C code.

And why exactly can't 32-bit code simply include the compat variant of
the public header, which is being generated by processing those non-C
constructs?

Jan

Re: [PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Frediano Ziglio 1 month ago
On Tue, Dec 10, 2024 at 2:44 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.12.2024 15:35, Frediano Ziglio wrote:
> > On Tue, Dec 10, 2024 at 10:32 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 22.11.2024 10:33, Frediano Ziglio wrote:
> >>> This allows to include other headers and avoid duplicated declarations.
> >>>
> >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> >>
> >> Again it's left unclear what the purpose / goal is.
> >>
> >
> > Reduce duplication avoiding duplicate declarations. The alternative
> > would be to duplicate them, which was proposed already and refused as
> > duplication was not good.
>
> Which declarations specifically?
>
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/include/boot/public/xen.h
> >>> @@ -0,0 +1,28 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>> +
> >>> +/* This header allows the inclusion of public xen.h */
> >>> +
> >>> +#ifndef BOOT__PUBLIC__XEN_H
> >>> +#define BOOT__PUBLIC__XEN_H
> >>> +
> >>> +#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
> >>> +#error Unexpected defines
> >>> +#endif
> >>
> >> What is this to guard against? We're in the Xen tree, building Xen.
> >>
> >
> > In include/public/arch-x86/xen.h file there are these declarations:
> >
> > #if defined(__i386__)
> > # ifdef __XEN__
> > __DeFiNe__ __DECL_REG_LO8(which) uint32_t e ## which ## x
> > __DeFiNe__ __DECL_REG_LO16(name) union { uint32_t e ## name; }
> > # endif
> > #include "xen-x86_32.h"
> > # ifdef __XEN__
> > __UnDeF__ __DECL_REG_LO8
> > __UnDeF__ __DECL_REG_LO16
> > __DeFiNe__ __DECL_REG_LO8(which) e ## which ## x
> > __DeFiNe__ __DECL_REG_LO16(name) e ## name
> > # endif
> > #elif defined(__x86_64__)
> > #include "xen-x86_64.h"
> > #endif
> >
> > This header allows us to include that part without compiler errors due
> > to __DeFiNe__ and __UnDeF__ not being C code.
>
> And why exactly can't 32-bit code simply include the compat variant of
> the public header, which is being generated by processing those non-C
> constructs?
>

I suppose I could solve that specific issue in that way. Where are
they generated?

> Jan

Frediano
Re: [PATCH v2 2/4] x86/boot: Use header to allows inclusion of public xen.h header
Posted by Jan Beulich 1 month ago
On 10.12.2024 15:48, Frediano Ziglio wrote:
> On Tue, Dec 10, 2024 at 2:44 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 10.12.2024 15:35, Frediano Ziglio wrote:
>>> On Tue, Dec 10, 2024 at 10:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 22.11.2024 10:33, Frediano Ziglio wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/include/boot/public/xen.h
>>>>> @@ -0,0 +1,28 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +
>>>>> +/* This header allows the inclusion of public xen.h */
>>>>> +
>>>>> +#ifndef BOOT__PUBLIC__XEN_H
>>>>> +#define BOOT__PUBLIC__XEN_H
>>>>> +
>>>>> +#if !defined(__XEN__) || defined(__XEN_TOOLS__) || __XEN__ != 1
>>>>> +#error Unexpected defines
>>>>> +#endif
>>>>
>>>> What is this to guard against? We're in the Xen tree, building Xen.
>>>>
>>>
>>> In include/public/arch-x86/xen.h file there are these declarations:
>>>
>>> #if defined(__i386__)
>>> # ifdef __XEN__
>>> __DeFiNe__ __DECL_REG_LO8(which) uint32_t e ## which ## x
>>> __DeFiNe__ __DECL_REG_LO16(name) union { uint32_t e ## name; }
>>> # endif
>>> #include "xen-x86_32.h"
>>> # ifdef __XEN__
>>> __UnDeF__ __DECL_REG_LO8
>>> __UnDeF__ __DECL_REG_LO16
>>> __DeFiNe__ __DECL_REG_LO8(which) e ## which ## x
>>> __DeFiNe__ __DECL_REG_LO16(name) e ## name
>>> # endif
>>> #elif defined(__x86_64__)
>>> #include "xen-x86_64.h"
>>> #endif
>>>
>>> This header allows us to include that part without compiler errors due
>>> to __DeFiNe__ and __UnDeF__ not being C code.
>>
>> And why exactly can't 32-bit code simply include the compat variant of
>> the public header, which is being generated by processing those non-C
>> constructs?
> 
> I suppose I could solve that specific issue in that way. Where are
> they generated?

include/Makefile generates them into include/compat/.

Jan