Re: [RFC PATCH 1/8]: PVH: Basic and preparatory changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 17 Aug 2012 09:35:54 +0100
Ian Campbell <Ian.Campbell@citrix.com> 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.