In order to support FRED, we're going to have to remove the {ds..gs} fields
from struct cpu_user_regs, meaning that it is going to have to become a
different type to the structure embedded in vcpu_guest_context_u.

struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
using this name), so renaming the public struct to be guest_user_regs in Xen's
view only.

Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
structure.  This removes the need for current.h to include public/xen.h, and
highlights a case where the emulator was picking up cpu_user_regs
transitively.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Jan: Is this what you intended?

cpu_user_regs_t and the guest handle don't seem to be used anywhere.  I'm
tempted to exclude them from Xen builds.
---
 xen/arch/x86/include/asm/cpu-user-regs.h | 69 ++++++++++++++++++++++++
 xen/arch/x86/include/asm/current.h       |  3 +-
 xen/arch/x86/x86_emulate/private.h       |  2 +
 xen/include/public/arch-x86/xen-x86_32.h |  8 +++
 xen/include/public/arch-x86/xen-x86_64.h |  8 +++
 xen/include/public/arch-x86/xen.h        |  6 +++
 6 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/x86/include/asm/cpu-user-regs.h

diff --git a/xen/arch/x86/include/asm/cpu-user-regs.h b/xen/arch/x86/include/asm/cpu-user-regs.h
new file mode 100644
index 000000000000..845b41a22ef2
--- /dev/null
+++ b/xen/arch/x86/include/asm/cpu-user-regs.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef X86_CPU_USER_REGS_H
+#define X86_CPU_USER_REGS_H
+
+#define DECL_REG_LOHI(which) union { \
+    uint64_t r ## which ## x; \
+    uint32_t e ## which ## x; \
+    uint16_t which ## x; \
+    struct { \
+        uint8_t which ## l; \
+        uint8_t which ## h; \
+    }; \
+}
+#define DECL_REG_LO8(name) union { \
+    uint64_t r ## name; \
+    uint32_t e ## name; \
+    uint16_t name; \
+    uint8_t name ## l; \
+}
+#define DECL_REG_LO16(name) union { \
+    uint64_t r ## name; \
+    uint32_t e ## name; \
+    uint16_t name; \
+}
+#define DECL_REG_HI(num) union { \
+    uint64_t r ## num; \
+    uint32_t r ## num ## d; \
+    uint16_t r ## num ## w; \
+    uint8_t r ## num ## b; \
+}
+
+struct cpu_user_regs
+{
+    DECL_REG_HI(15);
+    DECL_REG_HI(14);
+    DECL_REG_HI(13);
+    DECL_REG_HI(12);
+    DECL_REG_LO8(bp);
+    DECL_REG_LOHI(b);
+    DECL_REG_HI(11);
+    DECL_REG_HI(10);
+    DECL_REG_HI(9);
+    DECL_REG_HI(8);
+    DECL_REG_LOHI(a);
+    DECL_REG_LOHI(c);
+    DECL_REG_LOHI(d);
+    DECL_REG_LO8(si);
+    DECL_REG_LO8(di);
+    uint32_t error_code;
+    uint32_t entry_vector;
+    DECL_REG_LO16(ip);
+    uint16_t cs, _pad0[1];
+    uint8_t  saved_upcall_mask;
+    uint8_t  _pad1[3];
+    DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
+    DECL_REG_LO8(sp);
+    uint16_t ss, _pad2[3];
+    uint16_t es, _pad3[3];
+    uint16_t ds, _pad4[3];
+    uint16_t fs, _pad5[3];
+    uint16_t gs, _pad6[3];
+};
+
+#undef DECL_REG_HI
+#undef DECL_REG_LO16
+#undef DECL_REG_LO8
+#undef DECL_REG_LOHI
+
+#endif /* X86_CPU_USER_REGS_H */
diff --git a/xen/arch/x86/include/asm/current.h b/xen/arch/x86/include/asm/current.h
index bcec328c9875..243d17ef79fd 100644
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -9,7 +9,8 @@
 
 #include <xen/percpu.h>
 #include <xen/page-size.h>
-#include <public/xen.h>
+
+#include <asm/cpu-user-regs.h>
 
 /*
  * Xen's cpu stacks are 8 pages (8-page aligned), arranged as:
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index ef4745f56e27..dde4d3e3ccef 100644
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -10,6 +10,8 @@
 
 # include <xen/bug.h>
 # include <xen/kernel.h>
+
+# include <asm/cpu-user-regs.h>
 # include <asm/endbr.h>
 # include <asm/msr-index.h>
 # include <asm/x86-vendors.h>
diff --git a/xen/include/public/arch-x86/xen-x86_32.h b/xen/include/public/arch-x86/xen-x86_32.h
index 9e3bf06b121e..cd21438ab12b 100644
--- a/xen/include/public/arch-x86/xen-x86_32.h
+++ b/xen/include/public/arch-x86/xen-x86_32.h
@@ -114,6 +114,10 @@
 #define __DECL_REG_LO16(name) uint32_t e ## name
 #endif
 
+#ifdef __XEN__
+#define cpu_user_regs guest_user_regs
+#endif
+
 struct cpu_user_regs {
     __DECL_REG_LO8(b);
     __DECL_REG_LO8(c);
@@ -139,6 +143,10 @@ struct cpu_user_regs {
 typedef struct cpu_user_regs cpu_user_regs_t;
 DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
 
+#ifdef __XEN__
+#undef cpu_user_regs
+#endif
+
 #undef __DECL_REG_LO8
 #undef __DECL_REG_LO16
 
diff --git a/xen/include/public/arch-x86/xen-x86_64.h b/xen/include/public/arch-x86/xen-x86_64.h
index 43f6e3d22001..4388e20eaf49 100644
--- a/xen/include/public/arch-x86/xen-x86_64.h
+++ b/xen/include/public/arch-x86/xen-x86_64.h
@@ -159,6 +159,10 @@ struct iret_context {
 #define __DECL_REG_HI(num)    uint64_t r ## num
 #endif
 
+#ifdef __XEN__
+#define cpu_user_regs guest_user_regs
+#endif
+
 struct cpu_user_regs {
     __DECL_REG_HI(15);
     __DECL_REG_HI(14);
@@ -192,6 +196,10 @@ struct cpu_user_regs {
 typedef struct cpu_user_regs cpu_user_regs_t;
 DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
 
+#ifdef __XEN__
+#undef cpu_user_regs
+#endif
+
 #undef __DECL_REG
 #undef __DECL_REG_LOHI
 #undef __DECL_REG_LO8
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index fc2487986642..3b0fd05432f4 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -173,7 +173,13 @@ struct vcpu_guest_context {
 #define _VGCF_online                   5
 #define VGCF_online                    (1<<_VGCF_online)
     unsigned long flags;                    /* VGCF_* flags                 */
+
+#ifdef __XEN__
+    struct guest_user_regs user_regs;       /* User-level CPU registers     */
+#else
     struct cpu_user_regs user_regs;         /* User-level CPU registers     */
+#endif
+
     struct trap_info trap_ctxt[256];        /* Virtual IDT                  */
     unsigned long ldt_base, ldt_ents;       /* LDT (linear address, # ents) */
     unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
-- 
2.39.5

On 11.03.2025 22:10, Andrew Cooper wrote:
> In order to support FRED, we're going to have to remove the {ds..gs} fields
> from struct cpu_user_regs, meaning that it is going to have to become a
> different type to the structure embedded in vcpu_guest_context_u.
> 
> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
> using this name), so renaming the public struct to be guest_user_regs in Xen's
> view only.
> 
> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
> structure.  This removes the need for current.h to include public/xen.h, and
> highlights a case where the emulator was picking up cpu_user_regs
> transitively.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Albeit, besides a few remarks, a suggestion below.

> Jan: Is this what you intended?

Yes.

> cpu_user_regs_t and the guest handle don't seem to be used anywhere.  I'm
> tempted to exclude them from Xen builds.

I concur. We can always re-expose them should they be needed somewhere.

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef X86_CPU_USER_REGS_H
> +#define X86_CPU_USER_REGS_H
> +
> +#define DECL_REG_LOHI(which) union { \
> +    uint64_t r ## which ## x; \
> +    uint32_t e ## which ## x; \
> +    uint16_t which ## x; \
> +    struct { \
> +        uint8_t which ## l; \
> +        uint8_t which ## h; \
> +    }; \
> +}
> +#define DECL_REG_LO8(name) union { \
> +    uint64_t r ## name; \
> +    uint32_t e ## name; \
> +    uint16_t name; \
> +    uint8_t name ## l; \
> +}
> +#define DECL_REG_LO16(name) union { \
> +    uint64_t r ## name; \
> +    uint32_t e ## name; \
> +    uint16_t name; \
> +}
> +#define DECL_REG_HI(num) union { \
> +    uint64_t r ## num; \
> +    uint32_t r ## num ## d; \
> +    uint16_t r ## num ## w; \
> +    uint8_t r ## num ## b; \
> +}

Can we try to avoid repeating these here? The #undef-s in the public header are
to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
otherwise use identifiers of these names, can't we simply #ifdef-out those
#undef-s, and then not re-introduce the same (less the two underscores) here?
Granted we then need to include the public header here, but I think that's a
fair price to pay to avoid the redundancy.

> +struct cpu_user_regs
> +{
> +    DECL_REG_HI(15);
> +    DECL_REG_HI(14);
> +    DECL_REG_HI(13);
> +    DECL_REG_HI(12);
> +    DECL_REG_LO8(bp);
> +    DECL_REG_LOHI(b);
> +    DECL_REG_HI(11);
> +    DECL_REG_HI(10);
> +    DECL_REG_HI(9);
> +    DECL_REG_HI(8);
> +    DECL_REG_LOHI(a);
> +    DECL_REG_LOHI(c);
> +    DECL_REG_LOHI(d);
> +    DECL_REG_LO8(si);
> +    DECL_REG_LO8(di);
> +    uint32_t error_code;
> +    uint32_t entry_vector;
> +    DECL_REG_LO16(ip);
> +    uint16_t cs, _pad0[1];
> +    uint8_t  saved_upcall_mask;
> +    uint8_t  _pad1[3];
> +    DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
> +    DECL_REG_LO8(sp);
> +    uint16_t ss, _pad2[3];
> +    uint16_t es, _pad3[3];
> +    uint16_t ds, _pad4[3];
> +    uint16_t fs, _pad5[3];
> +    uint16_t gs, _pad6[3];

I had to peek ahead at the last patch to figure why you introduce these 4 fields
(plus their padding) here, just to remove them again. Personally I think it would
be neater if both were folded; nevertheless I'd like to leave this entirely to
you.

Jan

On 17/03/2025 12:15 pm, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>> from struct cpu_user_regs, meaning that it is going to have to become a
>> different type to the structure embedded in vcpu_guest_context_u.
>>
>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
>> using this name), so renaming the public struct to be guest_user_regs in Xen's
>> view only.
>>
>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
>> structure.  This removes the need for current.h to include public/xen.h, and
>> highlights a case where the emulator was picking up cpu_user_regs
>> transitively.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>> cpu_user_regs_t and the guest handle don't seem to be used anywhere.  I'm
>> tempted to exclude them from Xen builds.
> I concur. We can always re-expose them should they be needed somewhere.

It's actually a little ugly to do.

#ifdef __XEN__
#undef cpu_user_regs
#else
typedef struct cpu_user_regs cpu_user_regs_t;
DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
#endif

and I don't particularly like it, given the complexity of #ifdef-ary
around it.  Thoughts?

>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
>> @@ -0,0 +1,69 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef X86_CPU_USER_REGS_H
>> +#define X86_CPU_USER_REGS_H
>> +
>> +#define DECL_REG_LOHI(which) union { \
>> +    uint64_t r ## which ## x; \
>> +    uint32_t e ## which ## x; \
>> +    uint16_t which ## x; \
>> +    struct { \
>> +        uint8_t which ## l; \
>> +        uint8_t which ## h; \
>> +    }; \
>> +}
>> +#define DECL_REG_LO8(name) union { \
>> +    uint64_t r ## name; \
>> +    uint32_t e ## name; \
>> +    uint16_t name; \
>> +    uint8_t name ## l; \
>> +}
>> +#define DECL_REG_LO16(name) union { \
>> +    uint64_t r ## name; \
>> +    uint32_t e ## name; \
>> +    uint16_t name; \
>> +}
>> +#define DECL_REG_HI(num) union { \
>> +    uint64_t r ## num; \
>> +    uint32_t r ## num ## d; \
>> +    uint16_t r ## num ## w; \
>> +    uint8_t r ## num ## b; \
>> +}
> Can we try to avoid repeating these here? The #undef-s in the public header are
> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
> otherwise use identifiers of these names, can't we simply #ifdef-out those
> #undef-s, and then not re-introduce the same (less the two underscores) here?
> Granted we then need to include the public header here, but I think that's a
> fair price to pay to avoid the redundancy.

Breaking the connection between asm/current.h and public/xen.h is very
important IMO.  Right now, the public interface/types/defines are in
every TU, and they absolutely shouldn't be.

Sadly, the compiler isn't happy when including public/xen.h after
asm/current.h, hence the dropping of the underscores.

I did have half a mind to expand them fully.  I find them unintuitive,
but I also didn't think I'd successfully argue that change in.

I'm not terribly fussed how we do this, but I really do want to reduce
the header tangle.

>
>> +struct cpu_user_regs
>> +{
>> +    DECL_REG_HI(15);
>> +    DECL_REG_HI(14);
>> +    DECL_REG_HI(13);
>> +    DECL_REG_HI(12);
>> +    DECL_REG_LO8(bp);
>> +    DECL_REG_LOHI(b);
>> +    DECL_REG_HI(11);
>> +    DECL_REG_HI(10);
>> +    DECL_REG_HI(9);
>> +    DECL_REG_HI(8);
>> +    DECL_REG_LOHI(a);
>> +    DECL_REG_LOHI(c);
>> +    DECL_REG_LOHI(d);
>> +    DECL_REG_LO8(si);
>> +    DECL_REG_LO8(di);
>> +    uint32_t error_code;
>> +    uint32_t entry_vector;
>> +    DECL_REG_LO16(ip);
>> +    uint16_t cs, _pad0[1];
>> +    uint8_t  saved_upcall_mask;
>> +    uint8_t  _pad1[3];
>> +    DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
>> +    DECL_REG_LO8(sp);
>> +    uint16_t ss, _pad2[3];
>> +    uint16_t es, _pad3[3];
>> +    uint16_t ds, _pad4[3];
>> +    uint16_t fs, _pad5[3];
>> +    uint16_t gs, _pad6[3];
> I had to peek ahead at the last patch to figure why you introduce these 4 fields
> (plus their padding) here, just to remove them again. Personally I think it would
> be neater if both were folded; nevertheless I'd like to leave this entirely to
> you.

While both patches are reasonably small, I think it's important for
bisection to keep them separate.  They're both complex in separate ways.

~Andrew

On 21.03.2025 16:11, Andrew Cooper wrote:
> On 17/03/2025 12:15 pm, Jan Beulich wrote:
>> On 11.03.2025 22:10, Andrew Cooper wrote:
>>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>>> from struct cpu_user_regs, meaning that it is going to have to become a
>>> different type to the structure embedded in vcpu_guest_context_u.
>>>
>>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
>>> using this name), so renaming the public struct to be guest_user_regs in Xen's
>>> view only.
>>>
>>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
>>> structure.  This removes the need for current.h to include public/xen.h, and
>>> highlights a case where the emulator was picking up cpu_user_regs
>>> transitively.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
>>> cpu_user_regs_t and the guest handle don't seem to be used anywhere.  I'm
>>> tempted to exclude them from Xen builds.
>> I concur. We can always re-expose them should they be needed somewhere.
> 
> It's actually a little ugly to do.
> 
> #ifdef __XEN__
> #undef cpu_user_regs
> #else
> typedef struct cpu_user_regs cpu_user_regs_t;
> DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
> #endif
> 
> and I don't particularly like it, given the complexity of #ifdef-ary
> around it.  Thoughts?

It's not really pretty, but I'd be okay with this.

>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
>>> @@ -0,0 +1,69 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +#ifndef X86_CPU_USER_REGS_H
>>> +#define X86_CPU_USER_REGS_H
>>> +
>>> +#define DECL_REG_LOHI(which) union { \
>>> +    uint64_t r ## which ## x; \
>>> +    uint32_t e ## which ## x; \
>>> +    uint16_t which ## x; \
>>> +    struct { \
>>> +        uint8_t which ## l; \
>>> +        uint8_t which ## h; \
>>> +    }; \
>>> +}
>>> +#define DECL_REG_LO8(name) union { \
>>> +    uint64_t r ## name; \
>>> +    uint32_t e ## name; \
>>> +    uint16_t name; \
>>> +    uint8_t name ## l; \
>>> +}
>>> +#define DECL_REG_LO16(name) union { \
>>> +    uint64_t r ## name; \
>>> +    uint32_t e ## name; \
>>> +    uint16_t name; \
>>> +}
>>> +#define DECL_REG_HI(num) union { \
>>> +    uint64_t r ## num; \
>>> +    uint32_t r ## num ## d; \
>>> +    uint16_t r ## num ## w; \
>>> +    uint8_t r ## num ## b; \
>>> +}
>> Can we try to avoid repeating these here? The #undef-s in the public header are
>> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
>> otherwise use identifiers of these names, can't we simply #ifdef-out those
>> #undef-s, and then not re-introduce the same (less the two underscores) here?
>> Granted we then need to include the public header here, but I think that's a
>> fair price to pay to avoid the redundancy.
> 
> Breaking the connection between asm/current.h and public/xen.h is very
> important IMO.  Right now, the public interface/types/defines are in
> every TU, and they absolutely shouldn't be.

Hmm, that's a good point. Nevertheless I wonder if we still couldn't avoid the
unhelpful redundancy. E.g. by introducing a separate, small public header with
just these. Which we'd then pull in here as well.

> Sadly, the compiler isn't happy when including public/xen.h after
> asm/current.h, hence the dropping of the underscores.

Even if the ones here are #undef-ed after use?

> I did have half a mind to expand them fully.  I find them unintuitive,
> but I also didn't think I'd successfully argue that change in.

Roger - do you have an opinion here? I like these wrappers, yet then I also
understand this is code that's pretty unlikely to ever change again. Hence
fully expanding them is an option.

Jan

On Mon, Mar 24, 2025 at 10:47:29AM +0100, Jan Beulich wrote:
> On 21.03.2025 16:11, Andrew Cooper wrote:
> > On 17/03/2025 12:15 pm, Jan Beulich wrote:
> >> On 11.03.2025 22:10, Andrew Cooper wrote:
> >>> In order to support FRED, we're going to have to remove the {ds..gs} fields
> >>> from struct cpu_user_regs, meaning that it is going to have to become a
> >>> different type to the structure embedded in vcpu_guest_context_u.
> >>>
> >>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
> >>> using this name), so renaming the public struct to be guest_user_regs in Xen's
> >>> view only.
> >>>
> >>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
> >>> structure.  This removes the need for current.h to include public/xen.h, and
> >>> highlights a case where the emulator was picking up cpu_user_regs
> >>> transitively.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Thanks.
> > 
> >>> cpu_user_regs_t and the guest handle don't seem to be used anywhere.  I'm
> >>> tempted to exclude them from Xen builds.
> >> I concur. We can always re-expose them should they be needed somewhere.
> > 
> > It's actually a little ugly to do.
> > 
> > #ifdef __XEN__
> > #undef cpu_user_regs
> > #else
> > typedef struct cpu_user_regs cpu_user_regs_t;
> > DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
> > #endif
> > 
> > and I don't particularly like it, given the complexity of #ifdef-ary
> > around it.  Thoughts?
> 
> It's not really pretty, but I'd be okay with this.
> 
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
> >>> @@ -0,0 +1,69 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +#ifndef X86_CPU_USER_REGS_H
> >>> +#define X86_CPU_USER_REGS_H
> >>> +
> >>> +#define DECL_REG_LOHI(which) union { \
> >>> +    uint64_t r ## which ## x; \
> >>> +    uint32_t e ## which ## x; \
> >>> +    uint16_t which ## x; \
> >>> +    struct { \
> >>> +        uint8_t which ## l; \
> >>> +        uint8_t which ## h; \
> >>> +    }; \
> >>> +}
> >>> +#define DECL_REG_LO8(name) union { \
> >>> +    uint64_t r ## name; \
> >>> +    uint32_t e ## name; \
> >>> +    uint16_t name; \
> >>> +    uint8_t name ## l; \
> >>> +}
> >>> +#define DECL_REG_LO16(name) union { \
> >>> +    uint64_t r ## name; \
> >>> +    uint32_t e ## name; \
> >>> +    uint16_t name; \
> >>> +}
> >>> +#define DECL_REG_HI(num) union { \
> >>> +    uint64_t r ## num; \
> >>> +    uint32_t r ## num ## d; \
> >>> +    uint16_t r ## num ## w; \
> >>> +    uint8_t r ## num ## b; \
> >>> +}
> >> Can we try to avoid repeating these here? The #undef-s in the public header are
> >> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
> >> otherwise use identifiers of these names, can't we simply #ifdef-out those
> >> #undef-s, and then not re-introduce the same (less the two underscores) here?
> >> Granted we then need to include the public header here, but I think that's a
> >> fair price to pay to avoid the redundancy.
> > 
> > Breaking the connection between asm/current.h and public/xen.h is very
> > important IMO.  Right now, the public interface/types/defines are in
> > every TU, and they absolutely shouldn't be.
> 
> Hmm, that's a good point. Nevertheless I wonder if we still couldn't avoid the
> unhelpful redundancy. E.g. by introducing a separate, small public header with
> just these. Which we'd then pull in here as well.
> 
> > Sadly, the compiler isn't happy when including public/xen.h after
> > asm/current.h, hence the dropping of the underscores.
> 
> Even if the ones here are #undef-ed after use?
> 
> > I did have half a mind to expand them fully.  I find them unintuitive,
> > but I also didn't think I'd successfully argue that change in.
> 
> Roger - do you have an opinion here? I like these wrappers, yet then I also
> understand this is code that's pretty unlikely to ever change again. Hence
> fully expanding them is an option.

Hm, I don't have a strong opinion TBH, as I haven't done much work
that required touching those.  I think the proposal is fine, we can
always fully expand later if needed.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.