[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 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@xxxxxxxxxx> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> > > 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |