From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v14 11/17] pvh: Set up more PV stuff in set_info_guest Date: Thu, 7 Nov 2013 16:33:17 +0000 Message-ID: <527BC0CD.6030409@eu.citrix.com> References: <1383567306-6636-1-git-send-email-george.dunlap@eu.citrix.com> <1383567306-6636-12-git-send-email-george.dunlap@eu.citrix.com> <5277DF0C02000078000FF396@nat28.tlf.novell.com> <527BB71D.8050103@eu.citrix.com> <527BC9A30200007800100D16@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeSWP-0000za-9w for xen-devel@lists.xenproject.org; Thu, 07 Nov 2013 16:33:29 +0000 In-Reply-To: <527BC9A30200007800100D16@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: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 07/11/13 16:10, Jan Beulich wrote: >>>> On 07.11.13 at 16:51, George Dunlap wrote: >> On 04/11/13 16:53, Jan Beulich wrote: >>>>>> On 04.11.13 at 13:15, George Dunlap wrote: >>>> @@ -728,8 +740,21 @@ int arch_set_info_guest( >>>> >>>> if ( has_hvm_container_vcpu(v) ) >>>> { >>>> - hvm_set_info_guest(v); >>>> - goto out; >>>> + hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel); >>> I'm afraid this isn't correct - so far gs_base_kernel didn't get used >>> for HVM guests, i.e. you're changing behavior here (even if only >>> in a - presumably - benign way). >>> >>>> + >>>> + if ( is_hvm_vcpu(v) || v->is_initialised ) >>>> + goto out; >>>> + >>>> + cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]); >>> I'd recommend against using this PV construct - the 32-bit >>> counterpart won't be correct to be used here once 32-bit >>> support gets added. >> So the plan would be that once we support 32-bit, I'd just copy the code >> from below: >> >> if ( !compat ) >> cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]); >> else >> cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]); >> >> But since we know that compat is false here, it seems a bit silly to >> have the if() statement. >> >> But there should be a "PVH 32bitfixme" here -- is that enough for now? > Sure, but that wasn't my point. I was recommending against > *_cr3_to_pfn() altogether here. Just use the control register > value shifted right by 12 bits. Oh, right, I see. > >>>> @@ -1426,6 +1426,11 @@ static void vmx_set_info_guest(struct vcpu *v) >>>> __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow); >>>> } >>>> >>>> + /* PVH 32bitfixme */ >>>> + if ( is_pvh_vcpu(v) ) >>>> + __vmwrite(GUEST_GS_BASE, gs_base_kernel); >>> Oh, I see, you suppress this here. I'd really suggest adjusting the >>> caller, then you don't need to do anything here afaict. >> What do you mean "adjusting the caller"? What we want for HVM guests is >> for this field to be entirely left alone, isn't it? >> >> If we set GUEST_GS_BASE unconditionally here, the only way to effect "no >> change" is to read it and pass in the existing value, which seems kind >> of pointless. > Oh, right, I didn't pay attention to the calling path also being > used for HVM, and us not wanting to write zero here in that > case. Or maybe I did, but concluded that the code can be used > only for initial state setup, in which case writing zero would be > benign. I guess we could do that... but since we're talking about changing the interface here anyway, it's kind of nitpicking at this point. -George