From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Subject: Re: [PATCH v6 24/29] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs Date: Tue, 29 Sep 2015 18:49:45 +0200 Message-ID: <560AC129.1040309@citrix.com> References: <1441368548-43465-1-git-send-email-roger.pau@citrix.com> <1441368548-43465-25-git-send-email-roger.pau@citrix.com> <5600420C02000078000A4234@prv-mh.provo.novell.com> <5609664D.8060604@citrix.com> <560A555402000078000A6625@prv-mh.provo.novell.com> <560A6124.2090404@citrix.com> <560A7EFA02000078000A6846@prv-mh.provo.novell.com> <560A6714.7060303@citrix.com> <560A851202000078000A68AA@prv-mh.provo.novell.com> <560A69DB.9060505@citrix.com> <560A888D02000078000A68DD@prv-mh.provo.novell.com> <560A99C0.5050202@citrix.com> <560ACA7302000078000A6C93@prv-mh.provo.novell.com> <560AB5C6.6080404@citrix.com> <560AD65F02000078000A6CF6@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 1Zgy6B-0003ND-QA for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 16:49:52 +0000 In-Reply-To: <560AD65F02000078000A6CF6@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 , George Dunlap , Andrew Cooper , Tim Deegan , Stefano Stabellini , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org El 29/09/15 a les 18.20, Jan Beulich ha escrit: >>>> On 29.09.15 at 18:01, wrote: >> El 29/09/15 a les 17.29, Jan Beulich ha escrit: >>>>>> On 29.09.15 at 16:01, wrote: >>>> Ok thanks, so we seem to have a consensus. Before posting a new >>>> revision, does the following vcpu_hvm_context look fine to both of you: >>>> >>>> struct vcpu_hvm_x86_32 { >>>> uint32_t eax; >>>> uint32_t ecx; >>>> uint32_t edx; >>>> uint32_t ebx; >>>> uint32_t esp; >>>> uint32_t ebp; >>>> uint32_t esi; >>>> uint32_t edi; >>>> uint32_t eip; >>>> uint32_t eflags; >>>> >>>> uint32_t cr0; >>>> uint32_t cr3; >>>> uint32_t cr4; >>>> >>>> /* >>>> * EFER should only be used to set the NXE bit (if required) >>>> * when starting a vCPU in 32bit mode with paging enabled. >>>> */ >>>> uint64_t efer; >>>> >>>> uint32_t cs_base; >>>> uint32_t ds_base; >>>> uint32_t ss_base; >>>> uint32_t es_base; >>>> uint32_t tr_base; >>>> uint32_t cs_limit; >>>> uint32_t ds_limit; >>>> uint32_t ss_limit; >>>> uint32_t es_limit; >>>> uint32_t tr_limit; >>>> uint16_t cs_ar; >>>> uint16_t ds_ar; >>>> uint16_t ss_ar; >>>> uint16_t es_ar; >>>> uint16_t tr_ar; >>>> }; >>>> >>>> struct vcpu_hvm_x86_64 { >>>> uint64_t rax; >>>> uint64_t rcx; >>>> uint64_t rdx; >>>> uint64_t rbx; >>>> uint64_t rsp; >>>> uint64_t rbp; >>>> uint64_t rsi; >>>> uint64_t rdi; >>>> uint64_t rip; >>>> uint64_t rflags; >>>> >>>> uint64_t cr0; >>>> uint64_t cr3; >>>> uint64_t cr4; >>>> uint64_t efer; >>>> >>>> /* >>>> * Setting the CS attributes field is allowed in order to >>>> * start in compatibility mode. >>>> */ >>> >>> Hmm, as said before it would seem to me that this would better >>> (or also) be allowed to work by specifying a suitable 32-bit register >>> state. Remember that in compatibility mode base and limit matter >>> again, and I think you also can't start with a nul SS. >> >> Yes, I should add back all the registers here, so it looks like: >> >> uint32_t cs_base; >> uint32_t ds_base; >> uint32_t ss_base; >> uint32_t es_base; >> uint32_t cs_limit; >> uint32_t ds_limit; >> uint32_t ss_limit; >> uint32_t es_limit; >> uint16_t cs_ar; >> uint16_t ds_ar; >> uint16_t ss_ar; >> uint16_t es_ar; > > Or specify that compat mode entry is via using 32-bit register state. Yes, that should also work. If that's the preference then I guess we can remove all the selectors stuff from the 64bit structure. >>>> uint16_t cs_ar; >>>> }; >>> >>> No tr_* here? >> >> Is it necessary? for long or compatibility mode tr is always going to be: >> >> tr_base = 0; >> tr_limit = 0x67; >> tr_ar = 0x8b; >> >> The attributes field is always going to be 0x8b for compat or long mode, >> the base and the limit might be different, but the guest should change >> that by itself. > > Please explain why you have it in the 32-bit register state, but not > in the 64-bit one. Because the 32bit register state can be used to start a CPU in real mode, and the TS type in real mode is different from the ones in protected mode and long mode. > I said before that we should aim at being as > consistent as possible. (And btw I do not think that tr_base being > zero makes any sense.) We don't actually have a TSS anywhere, does it really matter if base is set to 0? In fact allowing the user to set tr_base and tr_limit is kind of pointless, it's impossible to load a TSS from this interface. So AFAICT what really matters is tr_ar only. >>> Also, what are the selector values going to be? Do you intend to >>> pass zero there? >> >> Do you mean the visible part of the selectors? With the current >> implementation the value of the "sel" field in the segment_register >> struct is left uninitialized, so it's undefined. I could set them to 0, >> but what's the point in doing it? > > We should never leave things uninitialized; leaving things > unspecified may be okay, but I don't see why we shouldn't > spell out what we intend to do, unless we foresee it to change > later on. Ack, I don't see a problem in setting them to 0, but anyway there's no GDT loaded, so the selector value itself is pointless (the cached part is what really matters). Roger.