From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests Date: Fri, 24 Jul 2015 09:49:31 -0600 Message-ID: <55B27AAB02000078000954A0@prv-mh.provo.novell.com> References: <1435923310-9019-1-git-send-email-roger.pau@citrix.com> <1435923310-9019-8-git-send-email-roger.pau@citrix.com> <55A3E0F102000078000903B5@mail.emea.novell.com> <55B0C100.6000308@citrix.com> <55B0EC520200007800094842@prv-mh.provo.novell.com> <1437651718.19412.92.camel@citrix.com> <55B103DD.4030901@citrix.com> <55B125440200007800094BD2@prv-mh.provo.novell.com> <55B10CDB.5010708@citrix.com> <1437667259.24746.12.camel@citrix.com> <55B11316.2000201@citrix.com> <55B130420200007800094CB8@prv-mh.provo.novell.com> <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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZIfE7-00030m-Fp for xen-devel@lists.xenproject.org; Fri, 24 Jul 2015 15:49:35 +0000 In-Reply-To: <55B25941.9@citrix.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: =?UTF-8?Q?Roger=20Pau=20Monn=C3=A9?= Cc: Ian Campbell , Andrew Cooper , David Vrabel , xen-devel@lists.xenproject.org, Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org >>> 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 to > 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. > What about the following layout: > > #if defined(__GNUC__) && !defined(__STRICT_ANSI__) > /* Anonymous union includes 16-, 32- and 64-bit names (e.g., bp/ebp/rbp). */ > # 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. > #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). > }; > > 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] > /* 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)? > 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. Jan