Re: [PATCH 18/18] PVH xen: introduce vmx_pvh.c

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

On Thu, 27 Jun 2013 08:41:34 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 27.06.13 at 05:30, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Tue, 25 Jun 2013 11:49:57 +0100 "Jan Beulich"
> > <JBeulich@suse.com> wrote:
> >> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > +static void read_vmcs_selectors(struct cpu_user_regs *regs)
> >> > +{
> >> > +    regs->ss = __vmread(GUEST_SS_SELECTOR);
> >> > +    regs->ds = __vmread(GUEST_DS_SELECTOR);
> >> > +    regs->es = __vmread(GUEST_ES_SELECTOR);
> >> > +    regs->gs = __vmread(GUEST_GS_SELECTOR);
> >> > +    regs->fs = __vmread(GUEST_FS_SELECTOR);
> >> > +}
> >> 
> >> By only conditionally reading the selector registers, how do you
> >> guarantee that read_segment_register() would always read
> >> valid values? I think that macro needs to not look at "regs->?s"
> >> at all...
> > 
> > read_segment_register() gets called for PVH only for 
> > EXIT_REASON_IO_INSTRUCTION
> > intercept. In this path, we call read all selectors before calling
> > emulate_privileged_op. If someone changes code, they'd have to make
> > sure of that. I can add more comments there, or go back to always
> > read all selectors
> > upon vmexit, but you already made me change that.
> 
> As per my earlier reply, I think this is wrong. Both from a
> conceptual POV and considering that new users of
> read_segment_register() may appear in the future. You
> ought to read the VMCS field in read_segment_register() if
> you want to keep avoiding the saving of the selector fields
> (which I strongly recommend).

I fail to see why saving of selector fields is any worse. New users
of read_segment_register would have to make sure that reading of selector
on demand happens always on current==PVH in the new path. To me that is no 
different than cheking to make sure the selectors are saved on the new call 
path. 

> >> > +static int vmxit_invalid_op(struct cpu_user_regs *regs)
> >> > +{
> >> > +    if ( guest_kernel_mode(current, regs)
> >> > || !emulate_forced_invalid_op(regs) )
> >> 
> >> Did you perhaps mean !guest_kernel_mode()?
> > 
> > No, pvh kernel has been changed to just do cpuid natively.
> > Hopefully, over time, looong time, emulate_forced_invalid_op can
> > just be removed.
> 
> While I don't disagree with a decision like this, the way you present
> this still makes me want to comment: What you do or don't do in
> Linux doesn't matter. What matters is a clear ABI description - what
> are the requirements to a PVH kernel implementation, and in
> particular what are the differences to a PV one? In the case here, a
> requirement would now be to _not_ use the PV form of CPUID (or
> more generally any operation that would result in #UD with the
> expectation that the hypervisor emulates the instruction).
> 
> I don't think I've seen such a formalized list of differences, which
> would make it somewhat difficult for someone else to convert their
> favorite OS to support PVH too.

Correct, I don't think we are at a point where we can create such a
list. It's been six months almost that the patches have been out, and 
right now I'd like to focus on getting them in.

> >> > +          ccpu, exit_reason, regs->rip, regs->rsp, regs->rflags,
> >> > +          __vmread(GUEST_CR0));
> >> > +
> >> > +    /* For guest_kernel_mode which is called from most places
> >> > below. */
> >> > +    regs->cs = __vmread(GUEST_CS_SELECTOR);
> >> 
> >> Which raises the question of whether your uses of
> >> guest_kernel_mode() are appropriate in the first place: Before this
> >> series there's no use at all under xen/arch/x86/hvm/.
> > 
> > HVM should do this for debug intercepts, otherwise it is wrongly 
> > intercepting user level debuggers like gdb.
> 
> And why would intercepting kernel debuggers like kdb or kgdb be
> correct?

The last I checked there were no kernel debuggers supported in domU, so
I wrote gdbsx. If that has changed, then there's some work to do for
gdbsx for both hvm and pvh.

> > HVM can also use this check for emulating forced invalid op for only
> > user levels. Since there's a cpuid intercept, and we are trying to
> > reduce pv-ops, this seems plausible.
> 
> HVM isn't supposed to be using PV CPUID. Ideally the same would
> be true for PVH (i.e. it may be better to make it look to user space
> like HVM, but I'm not sure if there aren't other collisions).

Correct, PVH does not use PV CPUID in PVH kernel. Initially, it was not
supported at all, but you convinced me to support from user level.

thanks
Mukesh