From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests Date: Fri, 24 Jul 2015 13:36:41 -0400 Message-ID: <20150724173641.GA9565@l.oracle.com> References: <55B11CBE.8050704@citrix.com> <55B1208C.9090207@citrix.com> <55B211D70200007800094FCF@prv-mh.provo.novell.com> <55B20C9D.5070708@citrix.com> <55B233890200007800095124@prv-mh.provo.novell.com> <55B22B5A.8020304@citrix.com> <55B24F4A02000078000952CC@prv-mh.provo.novell.com> <55B25941.9@citrix.com> <55B27AAB02000078000954A0@prv-mh.provo.novell.com> <55B26DB1.4020302@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZIgtv-000382-Vh for xen-devel@lists.xenproject.org; Fri, 24 Jul 2015 17:36:52 +0000 Content-Disposition: inline In-Reply-To: <55B26DB1.4020302@citrix.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: Roger Pau =?iso-8859-1?Q?Monn=E9?= Cc: Ian Campbell , Andrew Cooper , David Vrabel , Jan Beulich , xen-devel@lists.xenproject.org, Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On Fri, Jul 24, 2015 at 06:54:09PM +0200, Roger Pau Monn=E9 wrote: > El 24/07/15 a les 17.49, Jan Beulich ha escrit: > >>>> On 24.07.15 at 17:26, wrote: > >> El 24/07/15 a les 14.44, Jan Beulich ha escrit: > >>>>>> On 24.07.15 at 14:11, wrote: > >>>> Or your idea was to put all the bitness specific registers inside of > >>>> another separate structure and then unionize them? AFAICT the 16 and > >>>> 32bit structures are going to be empty. > >>> > >>> How that? 64-bit mode e.g. doesn't need full descriptor data for > >>> many of the segment registers. > >> > >> Please bear with me, but AFAIK cs, ds, es and ss still need to point t= o = > >> a GDT entry with a 0 base and a limit of 2^64. > > = > > No, in 64-bit mode bases are implicitly zero (with the exception of > > fs and gs) and limits are implicitly 2^64-1 (which you can't even > > express in a descriptor table entry, and with the exception of AMD's > > Long Mode Segment Limit extension). > > = > > But looking at the code below I see that you're thinking of selector > > values only, which would imply that you expect the hypervisor > > to actually read descriptors from the descriptor tables. That's > > doable, but cumbersome. > = > See the comment in the bottom of the email. > = > >> What about the following layout: > >> > >> #if defined(__GNUC__) && !defined(__STRICT_ANSI__) > >> /* Anonymous union includes 16-, 32- and 64-bit names (e.g., bp/ebp/rb= p). */ > >> # define __DECL_REG(n64, n32, n16) union { \ > >> uint64_t n64; \ > >> uint32_t n32; \ > >> uint16_t n16; \ > >> } > >> #else > >> /* Non-gcc sources must always use the proper 64-bit name (e.g., rbp).= */ > >> #define __DECL_REG(n64, n32, n16) uint64_t n64 > >> #endif > > = > > Since you keep repeating this, I finally have to comment on it: This > > is a suitable model for where we use it currently. It's imo not > > suitable at all here - not the least because even non-gcc users > > should be allowed to use just a 32-bit field when they mean one. > > But the problem is also that for 16- and 32-bit mode a 64-bit > > register is meaningless: Equally well you could expose r8-r15 to > > them. > = > Sorry, I didn't get that feeling by reading your previous comments. So > you would prefer to pack everything inside of the cpu_x86_ > structures? > = > >> #define __DECL_GP_REG(n) __DECL_REG(r##n, e##n, n) > >> > >> struct cpu_x86_16 { > >> /* Control registers. */ > >> uint32_t cr[8]; > >> /* Debug registers. */ > >> uint32_t dr[8]; > >> /* GDT descriptor address. */ > >> uint16_t gdtr; > > = > > Even in 16-bit mode this is a 32-bit base address. And you're > > omitting the limit here an below (which is a required part namely > > when you expect the hypervisor to access this table). > = > Well, I had in mind that this would be the memory address where the GDTR > base and limit is stored, just like the one you would use with the lgdt > instruction, but please read below. > = > >> }; > >> > >> struct cpu_x86_32 { > >> /* Control registers. */ > >> uint32_t cr[8]; > >> /* Debug registers. */ > >> uint32_t dr[8]; > >> /* GDT descriptor address. */ > >> uint32_t gdtr; > >> }; > >> > >> struct cpu_x86_64 { > >> /* Additional amd64 general purpose registers. */ > >> uint64_t r8, r9, r10, r11, r12, r13, r14, r15; > >> /* Control registers. */ > >> uint64_t cr[9]; > > = > > [16] > = > Why 16? Intel SDM section 2.5 only mentions cr8 as newly added in amd64. > TBH, I'm not sure it makes much sense to be able to set cr8 from this > interface, it can always be set by the guest OS and should not be > required for booting. > = > >> /* Debug registers. */ > >> uint64_t dr[8]; > >> /* Extended Feature Enable Register. */ > >> uint64_t efer; > >> /* GDT descriptor address. */ > >> uint64_t gdtr; > >> }; > >> > >> struct cpu_hvm_regs { > >> /* General purpose registers. */ > >> __DECL_GP_REG(ax); > >> __DECL_GP_REG(bx); > >> __DECL_GP_REG(cx); > >> __DECL_GP_REG(dx); > >> > >> /* Index registers. */ > >> __DECL_GP_REG(di); > >> __DECL_GP_REG(si); > >> > >> /* Pointer registers. */ > >> __DECL_GP_REG(bp); > >> __DECL_GP_REG(sp); > > = > > Now didn't you agree to use the _natural_ order (i.e. according to > > the numbers used to encode registers e.g. in instructions)? > = > OK, sorry, I didn't understand it that way. I though you only wanted > them properly sorted (so a, b, c...) and labelled. > = > >> struct vcpu_hvm_context { > >> /* 16bit fields of the strucutre will be used. */ > >> #define _VCPUHVM_MODE_16B 0 > >> #define VCPUHVM_MODE_16B (1<<_VCPUHVM_MODE_16B) > >> /* 32bit fields of the structure will be used. */ > >> #define _VCPUHVM_MODE_32B 1 > >> #define VCPUHVM_MODE_32B (1<<_VCPUHVM_MODE_32B) > >> /* 64bit fields of the structure will be used. */ > >> #define _VCPUHVM_MODE_64B 2 > >> #define VCPUHVM_MODE_64B (1<<_VCPUHVM_MODE_64B) > > = > > As soon as we have three values here, a boolean flag approach > > doesn't make sense anymore. > = > Yes, taking into account that only _one_ of them can be used at the same > time. > = > I have the feeling that we are over engineering this interface. IMHO we > should only allow the user to set the control registers, efer (on amd64) > and the GP registers. Segment selectors would be set by Xen to point to > a flat segment suitable for the mode the guest has selected. I think > that should be enough to get the guest OS into it's entry point, and > then it can do whatever it wants. If you are doing that why not use the old interface? > = > If that's not suitable, then my second option would be to allow the > guest to set the base, limit and AR bytes of the selectors, but not load > a GDT. They should be able to do any of those operations as a normal HVM guest I would think? > = > Roger.