[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
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@xxxxxxxxxx> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |