From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [RFC PATCH 1/8]: PVH: Basic and preparatory changes Date: Fri, 17 Aug 2012 12:24:28 -0700 Message-ID: <20120817122428.7704f87f@mantra.us.oracle.com> References: <20120815175724.3405043a@mantra.us.oracle.com> <1345192554.30865.93.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1345192554.30865.93.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "Xen-devel@lists.xensource.com" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On Fri, 17 Aug 2012 09:35:54 +0100 Ian Campbell wrote: > On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote: > > > LDT (linear address, # ents) */ > > - unsigned long gdt_frames[16], gdt_ents; /* GDT (machine > > frames, # ents) */ > > + unsigned long gdt_frames[16], gdt_ents; /* GDT (machine > > frames, # ents).* > > + * PV in HVM: it's GDTR > > addr/sz */ > > I'm not sure I understand this comment. What is "GDTR addr/sz" do you > mean that gdtframes/gdt_ents has a different semantics here? > > Might be worthy of a union? Or finding some other way to expand this > struct. In case of PVH, the field is used to send down GDTR address and size. perhaps better to just leave the comment out. > > > > -void __init xen_arch_setup(void) > > +/* Normal PV domain not running in HVM container */ > > It's a bit of a shame to overload the "HVM" term this way, to mean > both the traditional "providing a full PC like environment" and "PV > using hardware virtualisation facilities". > > Perhaps: > /* Normal PV domain without PVH extensions */ Ok, HVM==Hardware Virtual Machine seems more appropriate here, but I can remove the word HVM and go with 'PVH extensions'. > > +static __init void inline xen_non_pvh_arch_setup(void) > > + xen_panic_handler_init(); > > + > > + if (!xen_pvh_domain()) > > + xen_non_pvh_arch_setup(); > > The negative in the fn name here strikes me as a bit weird. Can't this > just be xen_pv_arch_setup? > > Or even just have: > /* Everything else is specific to PV without hardware support > */ if (xen_pvh_domain()) > return; OK. > > > > #ifdef CONFIG_ACPI > > if (!(xen_start_info->flags & SIF_INITDOMAIN)) { > > diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c > > index f58dca7..cdf269d 100644 > > --- a/arch/x86/xen/smp.c > > +++ b/arch/x86/xen/smp.c > > @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu, struct > > task_struct *idle) gdt = get_cpu_gdt_table(cpu); > > - BUG_ON((unsigned long)gdt & ~PAGE_MASK); > > + ctxt->ldt_ents = 0; > > Something odd is going on with the indentation here (and below I've > just noticed). I suspect lots of the changes aren't really changing > anything other than whitespace? Konrad wanted just doing the indentation without code change first where it made sense so that further patch makes it easy to see if statements added.