From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 04/18] PVH xen: add params to read_segment_register Date: Fri, 07 Jun 2013 07:29:40 +0100 Message-ID: <51B199F402000078000DC1CF@nat28.tlf.novell.com> References: <1369445137-19755-1-git-send-email-mukesh.rathor@oracle.com> <1369445137-19755-5-git-send-email-mukesh.rathor@oracle.com> <51A890CC02000078000D9F56@nat28.tlf.novell.com> <20130605182546.7c1ca5c5@mantra.us.oracle.com> <51B04CF102000078000DBC3D@nat28.tlf.novell.com> <20130606184328.0d56b3ae@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130606184328.0d56b3ae@mantra.us.oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 07.06.13 at 03:43, Mukesh Rathor wrote: > On Thu, 06 Jun 2013 07:48:49 +0100 > "Jan Beulich" wrote: > >> >>> On 06.06.13 at 03:25, Mukesh Rathor >> >>> wrote: >> > On Fri, 31 May 2013 11:00:12 +0100 >> > "Jan Beulich" 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