Re: [Xen-devel] [PATCH 04/18] PVH xen: add params to read_segment_register

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.13 at 03:43, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 06 Jun 2013 07:48:49 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 06.06.13 at 03:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > On Fri, 31 May 2013 11:00:12 +0100
>> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> > 
>> > save_segments() calls.
>> > 
>> > BTW, I can't use current in the macro because of call from
>> > save_segments(). 
>> > 
>> >> at all (i.e. things ought to work much like the if() portion of
>> >> show_registers() which you _do not_ modify).
>> > 
>> > Yeah, it was on hold because I've been investigating guest_cr[]
>> > sanity, and found that I was missing:
>> > 
>> >     v->arch.hvm_vcpu.guest_cr[4] = value;
>> > 
>> > So, my next version will add that and update show_registers() for
>> > PVH. I can scratch off another fixme from my list.
>> 
>> But you don't answer the underlying question that I raised: Is
>> accessing the struct cpu_user_regs selector register fields valid
>> at all? Again, the #VMEXIT handling code only stores garbage
>> into them (in debug builds, in non-debug builds it simply leaves
>> the fields unaltered).
> 
> Are you looking at the right version? The patch doesn't have much DEBUG
> code anymore. The selectors are updated every VMExit whether DEBUG or not:
> 
> static void read_vmcs_selectors(struct cpu_user_regs *regs)
> {
>     regs->cs = __vmread(GUEST_CS_SELECTOR);
>     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);
> }
> 
> 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));
> 
>     /* guest_kernel_mode() needs cs. read_segment_register needs others */
>     read_vmcs_selectors(regs);
> .....
> 
> 
> You are very thorough, so I'm sure it's not this obvious and I am just
> missing something, so please bear with me if that's the case. I'll
> continue staring at it... 

In fact I simply overlooked this, as I was looking at the assembly
entry point only (where those fields get written under !NDEBUG).
I'm sorry for that.

As a subsequent cleanup item, I think this assembly code should be
moved out to HVM-only C code, thus not being redundant with what
you do for PVH. In fact I was considering other things that can be
done in C code (reading/writing RIP, RSP, and RFLAGS) should be
removed from both respective entry.S files - it's not clear to me why
they were put there in the first place.

And anything we leave there should be optimized to hide latencies
between dependent instructions (the worst example here likely is
the CR2 read followed immediately by the intermediate register
getting written to memory, even though the control register read
could be done almost first thing in the handler).

One question though is why HVM code gets away without always
filling those fields, but PVH code doesn't. Could you say a word
on this?

Jan


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