Re: [PATCH 10/18 V2]: PVH xen: introduce vmx_pvh.c and pvh.c

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Apr 02, 2013 at 06:29:18PM -0700, Mukesh Rathor wrote:
> On Tue, 2 Apr 2013 10:10:12 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Mon, Apr 01, 2013 at 06:26:45PM -0700, Mukesh Rathor wrote:
> > > On Mon, 18 Mar 2013 12:32:06 -0400
> > > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > > 
> > > No, hide the X96EMUL_* from the caller of this function which deals
> > > with rc or non-zero.
> > 
> > In that case please make that part of the comment at the start. It
> > says: 0 success. But nothing about the failure path.
> 
> it's better to have it say some thing than nothing! failure paths are
> many and change over time, but when debugging its good to know right
> away if a function succeeded.

Sure. I am not saying that the comment is bad. I am just saying add
also the error conditions.

I don't think the error return values (whether they are positive or
negative) is going to change over time. But if they are, then the
comments can be updated at that time too.

> 
> > > >  - HYPERVISOR_set_segment_base
> > > >  - HYPERVISOR_set_gdt
> > > >  - HYPERVISOR_tmem
> > > >  .. and host of other.
> > > > 
> > > > This should be documented somewhere in docs?
> > > > Perhaps in docs/misc/pvh.txt and just outline which ones are not
> > > > to be used anymore?
> > > 
> > > I am keeping track of all doc stuff, lets document in the end when
> > > I enable PVH. We'll be changing stuff for a short while.
> > 
> > Why not make this 'doc stuff' part of the patches? That way when you
> > are done with one item you can have a patch to remove it out the TODO
> > list. Also this way other folks can look at it and if they have time
> > help you on some of the TODOs.
> 
> Lets keep things manageable. I have fixme's in the code, and also documentd
> in the cover what needs to be done. When we enable PVH, we'll know what
> got done and what didn't, and we can document it properly.

Kind off. We forgot about the 32-bit until Jan realized it. That should be
part of a TODO.

I am a big proponent of putting out a list of 'these are things that
are busted'. That way if somebody is trying to get familiar with this
they can work on off the bugs or items without having to stumble trying
to find the TODOs.

In other words - making it easier for other folks to help you with this.

> 
> > > 
> > > > Could you use 'cpuid' macro defined in processor.h?
> > > 
> > > since we know in this fucntion we are on intel, we can just do
> > > cpuid. the macro has somw quirks for cyrix cpus, and clears rcx.
> > > Besides this is user mode cpuid that will exactly be like this on a
> > > pure PV guest.
> > 
> > I am not following you. Is the reason you are doing this b/c the
> > macro clears rcx?
> 
> Correct.

Please state that as a comment in the code then. It helps as in a couple
of years (months?) somebody will notice this and say: Geey, why not
just use the #define cpuid.

This will give them enough information to make the proper decision
(which might be making the macro _not_ clear the rcx).

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