From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests Date: Fri, 24 Jul 2015 14:11:06 +0200 Message-ID: <55B22B5A.8020304@citrix.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZIbpI-0004K4-VP for xen-devel@lists.xenproject.org; Fri, 24 Jul 2015 12:11:45 +0000 In-Reply-To: <55B233890200007800095124@prv-mh.provo.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: Ian Campbell , Andrew Cooper , David Vrabel , xen-devel@lists.xenproject.org, Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org El 24/07/15 a les 12.46, Jan Beulich ha escrit: >>>> On 24.07.15 at 11:59, wrote: >> Just to recap and to make sure I got all the points: >> >> - vCPU initialization from the toolstack (BSP initialization) is going >> to be done using XEN_DOMCTL_sethvmcontext. >> - XEN_DOMCTL_{get/set}vcpucontext is going to return EOPNOTSUPP for >> HVM guests. >> - A new vcpu_guest_contatext (vcpu_hvm_context?) is going to be >> introduced together with a a new VCPUOP_initialize hypercall >> (VCPUOP_hvm_initialize?). > > Since the current one is called VCPUOP_initialise, why not just > VCPUOP_initialize? Having "hvm" in the name when it's meant to > also be used for PVH may end up being confusing. Right. >> The following is a layout proposal for the new vcpu_context: >> >> struct vcpu_hvm_context { >> /* 32bit fields of the structure will be used. */ >> #define _VCPUHVM_MODE_32B 0 >> #define VCPUHVM_MODE_32B (1<<_VCPUHVM_MODE_32B) >> /* 64bit fields of the structure will be used. */ >> #define _VCPUHVM_MODE_64B 1 >> #define VCPUHVM_MODE_64B (1<<_VCPUHVM_MODE_64B) > > 16-bit mode? I wasn't even considering that we would like to start the vCPUs in 16bit mode, but given that's how APs are started on bare metal I guess it makes sense to have it. > >> #define _VCPUHVM_online 2 >> #define VCPUHVM_online (1<<_VCPUHVM_online ) > > What is this needed for? It clears the VPF_down bit of the vcpu pause_flags, but we can also do this using the VCPUOP_up hypercall so I'm just going to remove it. >> uint32_t flags; /* VCPUHVM_* flags. */ >> struct cpu_hvm_regs user_regs; /* CPU registers. */ >> }; >> >> #if defined(__GNUC__) && !defined(__STRICT_ANSI__) >> /* Anonymous union includes both 32- and 64-bit names (e.g., ebp/rbp). */ >> # define __DECL_REG(n64, n32) union { \ >> uint64_t n64; \ >> uint32_t n32; \ >> } >> #else >> /* Non-gcc sources must always use the proper 64-bit name (e.g., rbp). */ >> #define __DECL_REG(n64, n32) uint64_t n64 >> #endif >> >> #define __DECL_GP_REG(n) __DECL_REG(r##n, e##n) >> >> struct cpu_hvm_regs { >> /* General purpose registers. */ >> __DECL_GP_REG(bx); >> __DECL_GP_REG(cx); >> __DECL_GP_REG(dx); >> __DECL_GP_REG(si); >> __DECL_GP_REG(di); >> __DECL_GP_REG(bp); >> __DECL_GP_REG(ax); >> __DECL_GP_REG(ip); >> __DECL_GP_REG(sp); > > Please put GPRs in their natural order - no need to mimic the order > in any existing structure. Ack. >> __DECL_GP_REG(flags); > > r8-r11, selector and descriptor registers, pseudo descriptor registers. > Or else for all of them their default state would need to be spelled out. r8-r15 right? > >> /* Control registers. */ >> uint64_t cr[8]; >> /* Valid on amd64 only. */ > > Fields valid/useful in one mode only should probably be put in > union-ized sub-structures. Do you mean something like: union { uint64_t efer; uint32_t __invalid32; uint16_t __invalid16; } It seems kind of pointless IMHO, the reason to have the union is to be able to access the registers using the native nomenclature, but if a register doesn't exist in a specific bitness I don't see the point of adding such "invalid" names. 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. Roger.