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

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

[Mukesh Rathor]

PVH only needs gdtaddr and gdtsz, so a union is created. There is no
functional code change in this patch.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>


diff -r bf249cd5f2c1 -r 278d7a933d88 tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c	Tue Oct 30 18:12:11 2012 +0000
+++ b/tools/libxc/xc_domain_restore.c	Fri Jan 11 16:19:40 2013 -0800
@@ -1933,15 +1933,15 @@ int xc_domain_restore(xc_interface *xch,
             munmap(start_info, PAGE_SIZE);
         }
         /* Uncanonicalise each GDT frame number. */
-        if ( GET_FIELD(ctxt, gdt_ents) > 8192 )
+        if ( GET_FIELD(ctxt, u.pv.gdt_ents) > 8192 )
         {
             ERROR("GDT entry count out of range");
             goto out;
         }
 
-        for ( j = 0; (512*j) < GET_FIELD(ctxt, gdt_ents); j++ )
+        for ( j = 0; (512*j) < GET_FIELD(ctxt, u.pv.gdt_ents); j++ )
         {
-            pfn = GET_FIELD(ctxt, gdt_frames[j]);
+            pfn = GET_FIELD(ctxt, u.pv.gdt_frames[j]);
             if ( (pfn >= dinfo->p2m_size) ||
                  (pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB) )
             {
@@ -1949,7 +1949,7 @@ int xc_domain_restore(xc_interface *xch,
                       j, (unsigned long)pfn);
                 goto out;
             }
-            SET_FIELD(ctxt, gdt_frames[j], ctx->p2m[pfn]);
+            SET_FIELD(ctxt, u.pv.gdt_frames[j], ctx->p2m[pfn]);
         }
         /* Uncanonicalise the page table base pointer. */
         pfn = UNFOLD_CR3(GET_FIELD(ctxt, ctrlreg[3]));
diff -r bf249cd5f2c1 -r 278d7a933d88 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c	Tue Oct 30 18:12:11 2012 +0000
+++ b/tools/libxc/xc_domain_save.c	Fri Jan 11 16:19:40 2013 -0800
@@ -1889,15 +1889,15 @@ int xc_domain_save(xc_interface *xch, in
         }
 
         /* Canonicalise each GDT frame number. */
-        for ( j = 0; (512*j) < GET_FIELD(&ctxt, gdt_ents); j++ )
+        for ( j = 0; (512*j) < GET_FIELD(&ctxt, u.pv.gdt_ents); j++ )
         {
-            mfn = GET_FIELD(&ctxt, gdt_frames[j]);
+            mfn = GET_FIELD(&ctxt, u.pv.gdt_frames[j]);
             if ( !MFN_IS_IN_PSEUDOPHYS_MAP(mfn) )
             {
                 ERROR("GDT frame is not in range of pseudophys map");
                 goto out;
             }
-            SET_FIELD(&ctxt, gdt_frames[j], mfn_to_pfn(mfn));
+            SET_FIELD(&ctxt, u.pv.gdt_frames[j], mfn_to_pfn(mfn));
         }
 
         /* Canonicalise the page table base pointer. */
diff -r bf249cd5f2c1 -r 278d7a933d88 xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c	Tue Oct 30 18:12:11 2012 +0000
+++ b/xen/arch/x86/domain.c	Fri Jan 11 16:19:40 2013 -0800
@@ -811,8 +811,8 @@ int arch_set_info_guest(
         }
 
         for ( i = 0; i < ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames); ++i )
-            fail |= v->arch.pv_vcpu.gdt_frames[i] != c(gdt_frames[i]);
-        fail |= v->arch.pv_vcpu.gdt_ents != c(gdt_ents);
+            fail |= v->arch.pv_vcpu.gdt_frames[i] != c(u.pv.gdt_frames[i]);
+        fail |= v->arch.pv_vcpu.gdt_ents != c(u.pv.gdt_ents);
 
         fail |= v->arch.pv_vcpu.ldt_base != c(ldt_base);
         fail |= v->arch.pv_vcpu.ldt_ents != c(ldt_ents);
@@ -861,17 +861,17 @@ int arch_set_info_guest(
         d->vm_assist = c(vm_assist);
 
     if ( !compat )
-        rc = (int)set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
+        rc = (int)set_gdt(v, c.nat->u.pv.gdt_frames, c.nat->u.pv.gdt_ents);
     else
     {
         unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames)];
-        unsigned int n = (c.cmp->gdt_ents + 511) / 512;
+        unsigned int n = (c.cmp->u.pv.gdt_ents + 511) / 512;
 
         if ( n > ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames) )
             return -EINVAL;
         for ( i = 0; i < n; ++i )
-            gdt_frames[i] = c.cmp->gdt_frames[i];
-        rc = (int)set_gdt(v, gdt_frames, c.cmp->gdt_ents);
+            gdt_frames[i] = c.cmp->u.pv.gdt_frames[i];
+        rc = (int)set_gdt(v, gdt_frames, c.cmp->u.pv.gdt_ents);
     }
     if ( rc != 0 )
         return rc;
diff -r bf249cd5f2c1 -r 278d7a933d88 xen/arch/x86/domctl.c
--- a/xen/arch/x86/domctl.c	Tue Oct 30 18:12:11 2012 +0000
+++ b/xen/arch/x86/domctl.c	Fri Jan 11 16:19:40 2013 -0800
@@ -1651,12 +1651,12 @@ void arch_get_info_guest(struct vcpu *v,
         c(ldt_base = v->arch.pv_vcpu.ldt_base);
         c(ldt_ents = v->arch.pv_vcpu.ldt_ents);
         for ( i = 0; i < ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames); ++i )
-            c(gdt_frames[i] = v->arch.pv_vcpu.gdt_frames[i]);
-        BUILD_BUG_ON(ARRAY_SIZE(c.nat->gdt_frames) !=
-                     ARRAY_SIZE(c.cmp->gdt_frames));
-        for ( ; i < ARRAY_SIZE(c.nat->gdt_frames); ++i )
-            c(gdt_frames[i] = 0);
-        c(gdt_ents = v->arch.pv_vcpu.gdt_ents);
+            c(u.pv.gdt_frames[i] = v->arch.pv_vcpu.gdt_frames[i]);
+        BUILD_BUG_ON(ARRAY_SIZE(c.nat->u.pv.gdt_frames) !=
+                     ARRAY_SIZE(c.cmp->u.pv.gdt_frames));
+        for ( ; i < ARRAY_SIZE(c.nat->u.pv.gdt_frames); ++i )
+            c(u.pv.gdt_frames[i] = 0);
+        c(u.pv.gdt_ents = v->arch.pv_vcpu.gdt_ents);
         c(kernel_ss = v->arch.pv_vcpu.kernel_ss);
         c(kernel_sp = v->arch.pv_vcpu.kernel_sp);
         for ( i = 0; i < ARRAY_SIZE(v->arch.pv_vcpu.ctrlreg); ++i )
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;
+        } pvh;
+    } u;
     unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1)   */
     /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
     unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)  */

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

[Jan Beulich]

>>> On 12.01.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> --- 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;
> +        } pvh;
> +    } u;

This, being a public header, needs a __XEN_INTERFACE_VERSION__
guard so that consumers updating the header (without updating
their interface version) would still build.

Jan

>      unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1)   */
>      /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
>      unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)  
> */
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

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

[Mukesh Rathor]

On Mon, 14 Jan 2013 11:18:55 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 12.01.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > --- 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;
> > +        } pvh;
> > +    } u;
> 
> This, being a public header, needs a __XEN_INTERFACE_VERSION__
> guard so that consumers updating the header (without updating
> their interface version) would still build.

Done. Thanks.

+#if __XEN_INTERFACE_VERSION__ < 0x00040300
+    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
+#else
     union {
         struct {
             /* GDT (machine frames, # ents) */
@@ -167,6 +170,7 @@ struct vcpu_guest_context {
             unsigned long gdtaddr, gdtsz;
         } pvh;
     } u;
+#endif

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

[Tim Deegan]

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. 

Cheers,

Tim.

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

[Jan Beulich]

>>> 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?

Jan

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

[Tim Deegan]

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.

Tim.

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

[Jan Beulich]

>>> 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.

Jan

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

[Tim Deegan]

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.

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

[Mukesh Rathor]

On Thu, 24 Jan 2013 14:29:32 +0000
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. 

Initially, I had it called gdt, but during code review of linux
patch (exact same change there), it was suggested to change it. Also,
I really dont' like field names likes "addr" or "sz". I really don't
wanna grep or cscope for them.

Thanks
Mukesh

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

[Tim Deegan]

At 18:04 -0800 on 24 Jan (1359050661), Mukesh Rathor wrote:
> Initially, I had it called gdt, but during code review of linux
> patch (exact same change there), it was suggested to change it.

Huh.  Fair enough, I guess.

> Also, I really dont' like field names likes "addr" or "sz". I really
> don't wanna grep or cscope for them.

I'm fairly strongly in the other camp -- I almost never find myself
grepping for leaf-node names, but having unions called 'u' and repeating
type or scope information in names both annoy the crap out of me. :)

Anyway, it seems like Jan agrees with you on the naming, so I'll let it
slide.  Can I suggest you call the new fields gdt_addr and gdt_sz, to
match the underscores in the existing fields?

Tim.

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

[Jan Beulich]

>>> On 25.01.13 at 10:38, Tim Deegan <tim@xen.org> wrote:
> At 18:04 -0800 on 24 Jan (1359050661), Mukesh Rathor wrote:
>> Also, I really dont' like field names likes "addr" or "sz". I really
>> don't wanna grep or cscope for them.
> 
> I'm fairly strongly in the other camp -- I almost never find myself
> grepping for leaf-node names, but having unions called 'u' and repeating
> type or scope information in names both annoy the crap out of me. :)
> 
> Anyway, it seems like Jan agrees with you on the naming, so I'll let it
> slide.  Can I suggest you call the new fields gdt_addr and gdt_sz, to
> match the underscores in the existing fields?

Actually I don't really. I prefer short field names (namely
without them repeating containing structure's names), and
consider the ones with extra prefixes or suffixes relics from
the K&R days.

As to "u" for unions, I think that's the closest we can get to
short of not naming them at all (which C89 doesn' allow). I
would even be tempted to introduce an abstraction to make
them unnamed conditionally (i.e. when a capable compiler is
being detected).

Jan