From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 04/18] PVH xen: add params to read_segment_register Date: Wed, 5 Jun 2013 18:25:46 -0700 Message-ID: <20130605182546.7c1ca5c5@mantra.us.oracle.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A890CC02000078000D9F56@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Fri, 31 May 2013 11:00:12 +0100 "Jan Beulich" wrote: > >>> On 25.05.13 at 03:25, Mukesh Rathor > >>> wrote: > > @@ -240,10 +240,10 @@ void do_double_fault(struct cpu_user_regs > > *regs) crs[2] = read_cr2(); > > crs[3] = read_cr3(); > > crs[4] = read_cr4(); > > - regs->ds = read_segment_register(ds); > > - regs->es = read_segment_register(es); > > - regs->fs = read_segment_register(fs); > > - regs->gs = read_segment_register(gs); > > + regs->ds = read_segment_register(current, regs, ds); > > + regs->es = read_segment_register(current, regs, es); > > + regs->fs = read_segment_register(current, regs, fs); > > + regs->gs = read_segment_register(current, regs, gs); > > In patch 9 you start using the first parameter of > read_segment_register() in ways not compatible with the use of > current here - the double fault handler (and in general all host side > exception handling code, i.e. the change to show_registers() is > questionable too) wants to use the real register value, not what's > in regs->. Even more, with the VMEXIT code storing at best > a known bad value into these fields, is it really valid to use them > at all (i.e. things ought to work much like the if() portion of > show_registers() which you _do not_ modify). Right, in case of double fault we'd need the real values. The only thing comes to mind: #define read_segment_register(vcpu, regs, name) \ ({ u16 __sel; \ struct cpu_user_regs *_regs = (regs); \ \ if ( guest_mode(regs) && is_pvh_vcpu(vcpu) ) <========== __sel = _regs->name; \ else \ asm volatile ( "movw %%" #name ",%0" : "=r" (__sel) ); \ __sel; \ }) but let me verify this would work for all possible contect_switch -> 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. BTW, In the process I realized in the cr4 update intercept I am missing: if ( value & HVM_CR4_GUEST_RESERVED_BITS(v) ) { HVM_DBG_LOG(DBG_LEVEL_1, "Guest attempts to set reserved bit in CR4: %lx", value); goto gpf; } if ( !(value & X86_CR4_PAE) && hvm_long_mode_enabled(v) ) { HVM_DBG_LOG(DBG_LEVEL_1, "Guest cleared CR4.PAE while " "EFER.LMA is set"); goto gpf; } I can't recall now whether I somehow concluded I didn't need to worry about it for PVH since I was only thinking 64bit, or just missed it. I guess I should have the check even if I expect the guest to always be in LME, right? thanks mukesh