Mukesh Rathor
2013-Jan-12 01:25 UTC
[RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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) */
Jan Beulich
2013-Jan-14 11:18 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
>>> 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
Mukesh Rathor
2013-Jan-15 00:45 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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
Tim Deegan
2013-Jan-24 14:29 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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.
Jan Beulich
2013-Jan-24 15:37 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
>>> 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
Tim Deegan
2013-Jan-24 15:45 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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.
Jan Beulich
2013-Jan-24 15:54 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
>>> 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
Tim Deegan
2013-Jan-24 16:18 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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.
Mukesh Rathor
2013-Jan-25 02:04 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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
Tim Deegan
2013-Jan-25 09:38 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
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.
Jan Beulich
2013-Jan-25 10:23 UTC
Re: [RFC PATCH 1/15]: PVH xen: turn gdb_frames/gdt_ents into union
>>> 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