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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.13 at 16:45, Tim Deegan <tim@xxxxxxx> wrote:
> At 15:37 +0000 on 24 Jan (1359041870), Jan Beulich wrote:
>> >>> On 24.01.13 at 15:29, Tim Deegan <tim@xxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel