Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union

[Tim Deegan <tim@xen.org>]

At 15:54 +0000 on 24 Jan (1359042878), Jan Beulich wrote:
> >>> On 24.01.13 at 16:45, Tim Deegan <tim@xen.org> wrote:
> > At 15:37 +0000 on 24 Jan (1359041870), Jan Beulich wrote:
> >> >>> On 24.01.13 at 15:29, Tim Deegan <tim@xen.org> wrote:
> >> > At 17:25 -0800 on 11 Jan (1357925122), Mukesh Rathor wrote:
> >> >> diff -r bf249cd5f2c1 -r 278d7a933d88 xen/include/public/arch-x86/xen.h
> >> >> --- a/xen/include/public/arch-x86/xen.h	Tue Oct 30 18:12:11 2012 +0000
> >> >> +++ b/xen/include/public/arch-x86/xen.h	Fri Jan 11 16:19:40 2013 -0800
> >> >> @@ -157,7 +157,16 @@ struct vcpu_guest_context {
> >> >>      struct cpu_user_regs user_regs;         /* User-level CPU registers     
> > 
> >> > */
> >> >>      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) 
> > 
> >> > */
> >> >> +    union {
> >> >> +        struct {
> >> >> +            /* GDT (machine frames, # ents) */
> >> >> +            unsigned long gdt_frames[16], gdt_ents;
> >> >> +        } pv;
> >> >> +        struct {
> >> >> +            /* PVH: GDTR addr and size */   
> >> >> +            unsigned long gdtaddr, gdtsz;
> >> > 
> >> > The GDT size is always 16 bits; I'd be inclined to make the addr
> >> > explicitly 64 bits too, to help out 32-bit toolstacks. 
> >> > 
> >> >> +        } pvh;
> >> >> +    } u;
> >> > 
> >> > Also, would you consider renaming this as:
> >> > 
> >> >     union {
> >> >         struct {
> >> >             /* GDT (machine frames, # ents) */
> >> >             unsigned long frames[16], ents;
> >> >         } pv;
> >> >         struct {
> >> >             /* PVH: GDTR addr and size */   
> >> >             unsigned long addr, sz;
> >> >         } pvh;
> >> >     } gdt;
> >> > 
> >> > ?  Then the calling code looks a little nicer. 
> >> 
> >> Did you overlook that it is a public header that gets modified
> >> here?
> > 
> > No, but you already pointed that out so I didn't feel the need to.  I'm
> > just suggesting that if we're going to change the naming here, we can
> > change it to something nicer.
> 
> But we shouldn't change names or types here arbitrarily. It's
> one thing if something truly needs fixing, but another if it's merely
> cosmetic.

My comment about types was about a field that is added _in this patch_! 
Likewise the renaming -- this patch already requires callers to 
s/gdt_frames/u.pv.gdt_frames/g.  Making it so users have to do 
s/gdt_frames/gdt.pv.frames/g doesn't make that any worse. 

Tim.