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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.06.13 at 03:35, 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:
>> > --- /dev/null
> ........
>> > +void vmx_pvh_vmexit_handler(struct cpu_user_regs *regs)
>> > +{
>> > +    unsigned long exit_qualification;
>> > +    unsigned int exit_reason = __vmread(VM_EXIT_REASON);
>> > +    int rc=0, ccpu = smp_processor_id();
>> > +    struct vcpu *v = current;
>> > +
>> > +    dbgp1("PVH:[%d]left VMCS exitreas:%d RIP:%lx RSP:%lx
>> > EFLAGS:%lx CR0:%lx\n",
>> > +          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/.
>> 
>> And if it is, I'd like to point out once again that this check should
>> be looking at SS.DPL, not CS.RPL.
> 
> Are you suggesting changing the macro to check for SS.DPL instead of
> CS.RPL it has always done for PV also? Note, PVH has checks in this
> patch to enforce long mode execution always, so CS.RPL should always
> be valid for PVH.

I'm saying that guest_kernel_mode() should be looking at the
VMCS for PVH (and, should it happen to be used in HVM code
paths, for HVM too) rather than struct cpu_user_regs. That
makes the saving of the CS selector pointless (in line with how
HVM behaves), and once you're going through
hvm_get_segment_register(), you can as well do this properly
(i.e. look at SS.DPL rather than CS.RPL). And no, repeatedly
comparing segment register handling with PV is bogus: In the PV
case we just don't have the luxury of accessible hidden register
portions, i.e. we need to get away with looking at selectors only.
Once you introduce this sort of hybrid model, you should avoid
_any_ unnecessary relaxations.

Jan