[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v14 11/17] pvh: Set up more PV stuff in set_info_guest
On 07/11/13 16:10, Jan Beulich wrote: On 07.11.13 at 16:51, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:On 04/11/13 16:53, Jan Beulich wrote:On 04.11.13 at 13:15, George Dunlap <george.dunlap@xxxxxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |