From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V10 PATCH 12/23] PVH xen: Support privileged op emulation for PVH Date: Mon, 12 Aug 2013 10:43:02 +0100 Message-ID: <5208AE26.5030604@eu.citrix.com> References: <1374631171-15224-1-git-send-email-mukesh.rathor@oracle.com> <1374631171-15224-13-git-send-email-mukesh.rathor@oracle.com> <20130807185941.56d3613d@mantra.us.oracle.com> <20130808183249.17340c18@mantra.us.oracle.com> <5204AE3702000078000EA8E0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5204AE3702000078000EA8E0@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: "keir.xen@gmail.com" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 09/08/13 07:54, Jan Beulich wrote: >>>> On 09.08.13 at 03:32, Mukesh Rathor wrote: >> On Thu, 8 Aug 2013 15:18:56 +0100 >> George Dunlap wrote: >>> The problem I have is that you still pass in *both* the value of >>> regs->$SEGMENT_REGISTER, *and* an enum of a segment register, and use >>> one in one case, and another in a different case. That's just a >>> really ugly interface. >>> >>> What I'd like to see is for read_descriptor_sel() to *just* take >>> which_sel (perhaps renamed sreg or something, since it's referring to >>> a segment register), and in the PV case, read the appropriate segment >>> register, then calling read_descriptor(). Then you don't have this >>> crazy thing where you set two variables (sel and which_cs) all over >>> the place. >> >> Hmm... lemme make sure I understand precisely, what you mean is >> something like: >> >> static int read_descriptor_sel(enum x86_segment which_sel, >> struct vcpu *v, >> const struct cpu_user_regs *regs, >> unsigned long *base, >> unsigned long *limit, >> unsigned int *ar, >> unsigned int vm86attr) >> >> { >> uint sel; >> if (!pvh) >> { >> sel = read_pv_segreg(which_sel) >> return read_descriptor(sel, v, regs, base, limit, ar, vm86attr); >> } >> } >> >> where read_pv_segreg() has one long case statment: >> case x86_seg_cs >> return read_segment_register(v, regs, cs); >> case x86_seg_cs >> return read_segment_register(v, regs, ds); >> ..... >> >> >> Then emulate_privileged_op() will not be setting data_sel, but >> only which_sel, except for one place: >> >> .... >> if ( lm_ovr == lm_seg_none || data_sel < 4 ) >> { >> switch ( lm_ovr ) >> { >> case lm_seg_none: >> ... >> >> That sounds like a good change to me. Jan, you OK with this? > It's worse performance wise, but better maintenance wise, so I > guess I don't really object (but also am not too happy with it). Is this a really hot path? It does mean going through a bit of extra code in the simple version. In theory one could do something with arrays or something to make that to avoid it. In any case, I think the interface in the patch is really ugly, but I'll leave it up to Keir and Jan what they want to do. -George