[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 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]);
        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?

@@ -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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.