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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Tue, 25 Jun 2013 11:49:57 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/vmx/vmx_pvh.c
> 
> 
> Should probably also say why.
> 
> > +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.

> > +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.

> > +          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.
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.

thanks,
-Mukesh

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